Issue 29896562: Issue 7003 - Look up whitelist filter only if URL is blocked (Closed)
|
Created:
Sept. 30, 2018, 8:55 a.m. by Manish Jethani Modified:
Oct. 23, 2018, 12:03 a.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #Patch Set 3 : Check whitelist for whitelist-only types #
Total comments: 2
Patch Set 4 : Refactor a bit #Patch Set 5 : Inline access to blacklist and whitelist #
Total comments: 7
Patch Set 6 : Add comments #MessagesTotal messages: 13
https://codereview.adblockplus.org/29896562/diff/29896563/lib/matcher.js File lib/matcher.js (left): https://codereview.adblockplus.org/29896562/diff/29896563/lib/matcher.js#oldc... lib/matcher.js:371: for (let i = 0, l = candidates.length; i < l; i++) Any reason to not use for..of here?
I am more convinced about making this change now. Let me rebase this patch and make some minor adjustments. https://codereview.adblockplus.org/29896562/diff/29896563/lib/matcher.js File lib/matcher.js (left): https://codereview.adblockplus.org/29896562/diff/29896563/lib/matcher.js#oldc... lib/matcher.js:371: for (let i = 0, l = candidates.length; i < l; i++) On 2018/09/30 13:39:23, Sebastian Noack wrote: > Any reason to not use for..of here? for..of is quite a bit slower in this case.
Patch Set 2-4 Ready for review. https://codereview.adblockplus.org/29896562/diff/29918560/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918560/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) If the type mask includes no types other than DOCUMENT, ELEMHIDE, GENERICHIDE, and GENERICBLOCK, we can skip the blacklist. https://codereview.adblockplus.org/29896562/diff/29918560/lib/matcher.js#newc... lib/matcher.js:350: if (blacklistHit || (typeMask & WHITELIST_ONLY_TYPES) != 0) If the type mask includes DOCUMENT, ELEMHIDE, GENERICHIDE, or GENERICBLOCK, we need to check the whitelist.
Patch Set 5 Also ran the test [1] again and got similar results [1]: https://issues.adblockplus.org/ticket/7003#comment:12
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) I wonder if this logic is necessary. Shouldn't calling code that is specifically interested in whitelisting filters rather use matcher.whitelist.matchesAny()?
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/21 22:41:39, Sebastian Noack wrote: > I wonder if this logic is necessary. Shouldn't calling code that is specifically > interested in whitelisting filters rather use matcher.whitelist.matchesAny()? I like the idea, but we would lose out on the caching of the result (which should be valuable for these types of lookups). I see that lib/whitelisting.js and lib/filterComposer.js already call defaultMatcher.whitelist.matchesAny directly. We could just introduce new individual caches in both blacklist and whitelist. I think though that I am leaning towards the current patch, which optimizes for whitelist-only types as well. If we go with this, we can change the code in adblockpluschrome to call defaultMatcher.matchesAny in all cases. What do you think?
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/22 11:05:16, Manish Jethani wrote: > On 2018/10/21 22:41:39, Sebastian Noack wrote: > > I wonder if this logic is necessary. Shouldn't calling code that is > specifically > > interested in whitelisting filters rather use matcher.whitelist.matchesAny()? > > I like the idea, but we would lose out on the caching of the result (which > should be valuable for these types of lookups). > > I see that lib/whitelisting.js and lib/filterComposer.js already call > defaultMatcher.whitelist.matchesAny directly. We could just introduce new > individual caches in both blacklist and whitelist. > > I think though that I am leaning towards the current patch, which optimizes for > whitelist-only types as well. If we go with this, we can change the code in > adblockpluschrome to call defaultMatcher.matchesAny in all cases. > > What do you think? No strong opinion either way, but in case we go with your approach, we should probably hide defaultMatcher.whitelist and defaultMatcher.blacklist from the public API. Also mind putting the result of `(typeMask & ~WHITELIST_ONLY_TYPES) != 0` into a variable rather than repeating yourself?
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/22 15:04:31, Sebastian Noack wrote: > On 2018/10/22 11:05:16, Manish Jethani wrote: > > On 2018/10/21 22:41:39, Sebastian Noack wrote: > > > I wonder if this logic is necessary. Shouldn't calling code that is > > specifically > > > interested in whitelisting filters rather use > matcher.whitelist.matchesAny()? > > > > I like the idea, but we would lose out on the caching of the result (which > > should be valuable for these types of lookups). > > > > I see that lib/whitelisting.js and lib/filterComposer.js already call > > defaultMatcher.whitelist.matchesAny directly. We could just introduce new > > individual caches in both blacklist and whitelist. > > > > I think though that I am leaning towards the current patch, which optimizes > for > > whitelist-only types as well. If we go with this, we can change the code in > > adblockpluschrome to call defaultMatcher.matchesAny in all cases. > > > > What do you think? > > No strong opinion either way, but in case we go with your approach, we should > probably hide defaultMatcher.whitelist and defaultMatcher.blacklist from the > public API. We will be doing this as part of this: https://codereview.adblockplus.org/29897555/ > Also mind putting the result of `(typeMask & ~WHITELIST_ONLY_TYPES) != 0` into a > variable rather than repeating yourself? It's not repetition because the two checks are different.
Patch Set 6: Add comments https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/22 20:39:30, Manish Jethani wrote: > On 2018/10/22 15:04:31, Sebastian Noack wrote: > > > Also mind putting the result of `(typeMask & ~WHITELIST_ONLY_TYPES) != 0` into > a > > variable rather than repeating yourself? > > It's not repetition because the two checks are different. I've added comments to make it clearer.
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/22 20:39:30, Manish Jethani wrote: > On 2018/10/22 15:04:31, Sebastian Noack wrote: > > No strong opinion either way, but in case we go with your approach, we should > > probably hide defaultMatcher.whitelist and defaultMatcher.blacklist from the > > public API. > > We will be doing this as part of this: > > https://codereview.adblockplus.org/29897555/ I don't see any changes in the current patch there that will make defaultMatcher.blacklist and defaultMatcher.whitelist private. > > > Also mind putting the result of `(typeMask & ~WHITELIST_ONLY_TYPES) != 0` into > a > > variable rather than repeating yourself? > > It's not repetition because the two checks are different. Acknowledged.
https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29896562/diff/29918564/lib/matcher.js#newc... lib/matcher.js:336: if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0) On 2018/10/22 22:12:35, Sebastian Noack wrote: > On 2018/10/22 20:39:30, Manish Jethani wrote: > > On 2018/10/22 15:04:31, Sebastian Noack wrote: > > > No strong opinion either way, but in case we go with your approach, we > should > > > probably hide defaultMatcher.whitelist and defaultMatcher.blacklist from the > > > public API. > > > > We will be doing this as part of this: > > > > https://codereview.adblockplus.org/29897555/ > > I don't see any changes in the current patch there that will make > defaultMatcher.blacklist and defaultMatcher.whitelist private. Yes, I'll suggest that change there if we agree on this one here. Regardless, this change does not depend on that one (i.e. whitelist could still be public and this change would still make sense).
LGTM 
|
