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)
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
- Create a new browser profile or reset the notificationdata preference.
- Navigate to wwwok.at and you should see a notification pop up that asks you to hide targeted messages
- Click the "x" to close the notification
- 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.
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
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: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
Nice idea! But please still add a "What to test" section.