Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5434053851348992: Issue 1282 - Let user-defined filters override filter lists (Closed)

Created:
Dec. 9, 2014, 10:24 a.m. by Sebastian Noack
Modified:
Dec. 13, 2014, 6:11 p.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

This is just a proof of concept for now. Do you think this change would be justified or do you have any concerns regarding the performance? See the discussion in the issue: https://issues.adblockplus.org/ticket/1282#comment:11

Patch Set 1 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -35 lines) Patch
M lib/elemHide.js View 1 chunk +5 lines, -2 lines 0 comments Download
M lib/filterClasses.js View 1 chunk +13 lines, -0 lines 0 comments Download
M lib/matcher.js View 12 chunks +53 lines, -33 lines 0 comments Download

Messages

Total messages: 2
Sebastian Noack
Dec. 9, 2014, 11:21 a.m. (2014-12-09 11:21:08 UTC) #1
Wladimir Palant
Dec. 9, 2014, 10:44 p.m. (2014-12-09 22:44:55 UTC) #2
I didn't get into the details of the implementation yet, merely discussing the
general approach. Also, I'd rather discuss matcher first before we look at
element hiding.

Performance-wise it seems wrong to add even more data structures that need to be
considered. A better approach should be getting all potential filter hits, then
selecting the one with the highest priority. Compared to the current approach we
drop a shortcut (blacklist doesn't need to be considered if whitelist already
produced a hit) but that scenario is comparably uncommon anyway. On the other
hand, we could merge the data structures for blacklist and whitelist - that's a
rather significant potential performance improvement.

Note that the matcher really shouldn't know the details of how filters are being
prioritized. So it seems that ideally we will simply specify a numerical
priority for each filter when it is being added. Alternatively, that priority
can be a property of the filter itself (pro: the value is retrieved on demand,
so it is guaranteed to be current; contra: the code behind the property will
likely run too often). A solution combining the advantages of both approaches
should also be possible, likely involving FilterListener setting priority
property on the filters, only when actually necessary.

If the matcher can go by priority and doesn't need to distinguish between
whitelist and blacklist, then the code can be simplified rather drastically:
CombinedMatcher can go, the "basic" Matcher class is sufficient. Some logic will
have to move however, especially caching.

Powered by Google App Engine
This is Rietveld