Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#431 closed change (fixed)

Remove special handling for the $sitekey option

Reported by: trev Assignee: greiner
Priority: P2 Milestone: Adblock-Plus-2.6.5-for-Firefox
Module: Core Keywords:
Cc: Blocked By:
Blocking: #406 Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4559243822759936/

Description (last modified by trev)

Background

Currently the $sitekey option is handled separately from any other filter options. The background here is that it was originally meant to whitelist everything on particular websites - so I implemented this special solution in order to avoid impacting the performance negatively. In addition to requiring more code, distinguishing between individual requests isn't possible and only one filter is respected for each key.

What to change

Remove the special handling for sitekeys in lib/matcher.js - they should be handled in exactly the same way as the domain option. That means that the filters are matched by the pattern part first, other options are checked later. This means that the sitekey needs to be added as a parameter for Mather.matchesAny() and RegExpFilter.matches(). The latter can check sitekey matches trivially.

The complication here is: a valid sitekey should apply to the current page and any pages below it in the frame hierarchy. E.g. consider the following structure:

top
|- frame1 (sitekey)
   |- frame2

We should consider the sitekey both when checking whether frame1 and frame2 are whitelisted. Also, any request triggered by frame1 or frame2 should be checked with the sitekey considered. top on the other hand is above the sitekey, so there it shouldn't be considered.

There is also the unlikely scenario that there are multiple valid sitekeys in the hierarchy. IMHO we don't need to overcomplicate things here - whichever sitekey is closer should be used:

top (sitekey1)
|- frame1 (sitekey2)
   |- frame2

Here sitekey1 should be checked when determining whether top is whitelisted, also for any requests made by top. For frame1 and frame2 it is being overwritten with sitekey2 however, so sitekey2 should be used for these frames instead.

Change History (7)

comment:1 Changed 6 years ago by trev

  • Description modified (diff)

comment:2 Changed 6 years ago by fhd

  • Priority changed from P1 to P2

comment:3 Changed 6 years ago by greiner

  • Owner set to greiner

comment:4 Changed 5 years ago by greiner

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

comment:5 Changed 5 years ago by trev

  • Description modified (diff)

comment:6 Changed 5 years ago by greiner

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 4 years ago by trev

  • Milestone set to Adblock-Plus-2.6.5-for-Firefox
  • Tester set to Unknown
Note: See TracTickets for help on using tickets.