Opened on 05/12/2015 at 09:37:26 AM

Closed on 05/18/2015 at 03:29:14 PM

Last modified on 05/18/2015 at 03:29:41 PM

#2505 closed change (fixed)

Check for notifications and dispatch filter.hitCount event asynchronously

Reported by: greiner Assignee: sebastian
Priority: P4 Milestone: Adblock-Plus-1.9-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

Description (last modified by sebastian)


On each request we're checking whether or not to show a notification on Safari, because some notifications are only supposed to be shown if a certain domain is encountered (e.g. anti-adblock notification). Moreover, the the filter.hitCount event is processed with every request. This, however, is potentially causing huge performance issues (see test results below) and can be significantly improved by making it run asynchronously.

Speed measurements of webRequest.js~onBeforeRequest:

  • (homepage + article page):
    • before: 223ms
    • after: 32ms
  • (homepage + article page)
    • before: 88ms
    • after: 11ms
    • before: 81ms
    • after: 20ms

What to change

Wrap the code calling NotificationStorage.getNextToShow() and FilterNotifier.triggerListeners("filter.hitCount", ..) into a function called delayed with setTimeout().

What to test

  1. Create a new browser profile or reset the notificationdata preference.
  2. Navigate to and you should see a notification pop up that asks you to hide targeted messages
  3. Click the "x" to close the notification
  4. Navigate to and you should not see a notification pop up

Note that the pop up will no longer appear if you click on "Yes" or "No" unless you reset the notificationdata preference again.

Also make sure that the ad counter still works as expected, i.e. that the number of blocked requests show up in the ABP icon.

Attachments (0)

Change History (9)

comment:1 Changed on 05/15/2015 at 01:33:16 PM by sebastian

  • Cc sebastian added
  • Priority changed from Unknown to P4
  • Ready set

Nice idea! But please still add a "What to test" section.

comment:2 Changed on 05/15/2015 at 05:54:47 PM by greiner

  • Description modified (diff)

Good point, added test instructions to ensure that notifications continue working as expected.

comment:3 Changed on 05/18/2015 at 08:48:58 AM by sebastian

I think we should probably move FilterNotifier.triggerListeners() into the asynchronous callback as well.

comment:4 in reply to: ↑ description Changed on 05/18/2015 at 08:59:13 AM by sebastian

Replying to greiner:

  • Make sure that the request is either of type "main_frame" or "sub_frame" before calling NotificationStorage.getNextToShow()

Note that NotificationStorage.getNextToShow() is currently only called for subframes loaded on Safari.

comment:5 Changed on 05/18/2015 at 11:19:28 AM by sebastian

  • Owner set to sebastian

comment:6 Changed on 05/18/2015 at 11:19:38 AM by sebastian

  • Status changed from new to reviewing

comment:7 Changed on 05/18/2015 at 11:19:48 AM by sebastian

  • Review URL(s) modified (diff)

comment:8 Changed on 05/18/2015 at 03:29:14 PM by sebastian

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 Changed on 05/18/2015 at 03:29:41 PM by sebastian

  • Summary changed from Don't block web request blocking on notification checks to Check for notifications and dispatch filter.hitCount event asynchronously

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from sebastian.
Note: See TracTickets for help on using tickets.