Opened 5 years ago

Closed 3 months ago

Last modified 2 months ago

#1162 closed change (rejected)

Cache notification URL matcher

Reported by: greiner Assignee: greiner
Priority: P2 Milestone:
Module: Core Keywords: closed-in-favor-of-gitlab
Cc: Blocked By: #7432
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5330039625220096/

Description (last modified by greiner)

Background

We introduced notifications that can be triggered when matching a given set of filters for the anti-adblock notification. In the current implementation we are instantiating the matcher each time when we check for a notification based on the assumption that the performance impact would be insignificant.

However, since this is not the case due to this happening on each request we should only instantiate it once and reuse it after that.

What to change

In lib/notification.js:

  • Instantiate the matcher on notification creation and attach it to the notification as a property _matcher
  • Notifications should remain serializable (replace JSON.serialize() to drop all properties starting with "_" if necessary)
  • Remove the instantiation in getNextToShow() and instead use the matcher associated with the notification

Hints for testers

  • Check that there are no errors after installing the extension
  • Check that anti-adblock notification shows up on domains listed in the filter list and doesn't show up for any other domains

Change History (18)

comment:1 Changed 5 years ago by greiner

  • Cc trev added

comment:2 Changed 5 years ago by greiner

  • Owner set to greiner

comment:3 Changed 5 years ago by greiner

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 2 years ago by fhd

  • Cc trev removed

comment:5 Changed 23 months ago by greiner

  • Description modified (diff)
  • Tester set to Unknown

Added hints for testers section.

comment:6 Changed 9 months ago by umairaslam

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:7 Changed 8 months ago by greiner

  • Blocked By 7432 added

comment:8 Changed 8 months ago by Antoniacummins

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:9 Changed 7 months ago by mitchellsroka01

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:10 Changed 7 months ago by Arvind098

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:11 Changed 6 months ago by Angus O'Driscoll

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:12 Changed 6 months ago by Archie Radcliffe

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:13 Changed 6 months ago by Archie Radcliffe

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:14 Changed 5 months ago by Ora Kessler

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:15 Changed 5 months ago by Sperry56

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:16 Changed 4 months ago by Kaitlyn

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:17 Changed 4 months ago by steve39

spam

Last edited 2 months ago by kzar (previous) (diff)

comment:18 Changed 3 months ago by sebastian

  • Keywords closed-in-favor-of-gitlab added
  • Resolution set to rejected
  • Status changed from reviewing to closed

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the new issue tracker.

Note: See TracTickets for help on using tickets.