Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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):

http://codereview.adblockplus.org/5897276308324352

Description (last modified by sebastian)

Background

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:

  • bild.de (homepage + article page):
    • before: 223ms
    • after: 32ms
  • recode.net (homepage + article page)
    • before: 88ms
    • after: 11ms
  • wwwok.at
    • 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 wwwok.at 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 w3.org 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.

Change History (9)

comment:1 Changed 4 years ago 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 4 years ago by greiner

  • Description modified (diff)

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

comment:3 Changed 4 years ago by sebastian

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

comment:4 in reply to: ↑ description Changed 4 years ago 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 4 years ago by sebastian

  • Owner set to sebastian

comment:6 Changed 4 years ago by sebastian

  • Status changed from new to reviewing

comment:7 Changed 4 years ago by sebastian

  • Review URL(s) modified (diff)

comment:8 Changed 4 years ago 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 4 years ago by sebastian

  • Summary changed from Don't block web request blocking on notification checks to Check for notifications and dispatch filter.hitCount event asynchronously
Note: See TracTickets for help on using tickets.