Issue 7003 - Look up whitelist filter only if URL is blocked

lib/matcher.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 315 matching lines @@
                      specificOnly)
   {
     let candidates = location.toLowerCase().match(/[a-z0-9%]{3,}/g);
     if (candidates === null)
       candidates = [];
     candidates.push("");
 
     let whitelistHit = null;
     let blacklistHit = null;
 
+    // If the type mask includes no types other than whitelist-only types, we
+    // can skip the blacklist.
     if ((typeMask & ~WHITELIST_ONLY_TYPES) != 0)

[Sebastian Noack, 2018/10/21 22:41:39]

I wonder if this logic is necessary. Shouldn't calling code that is specifically
interested in whitelisting filters rather use matcher.whitelist.matchesAny()?

[Manish Jethani, 2018/10/22 11:05:16]

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?

[Sebastian Noack, 2018/10/22 15:04:31]

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?

[Manish Jethani, 2018/10/22 20:39:30]

We will be doing this as part of this:

https://codereview.adblockplus.org/29897555/
It's not repetition because the two checks are different.

[Manish Jethani, 2018/10/22 21:16:19]

I've added comments to make it clearer.

[Sebastian Noack, 2018/10/22 22:12:35]

I don't see any changes in the current patch there that will make
defaultMatcher.blacklist and defaultMatcher.whitelist private.
Acknowledged.

[Manish Jethani, 2018/10/22 22:57:56]

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).

     {
       for (let i = 0, l = candidates.length; !blacklistHit && i < l; i++)
       {
         blacklistHit = this.blacklist._checkEntryMatch(candidates[i], location,
                                                        typeMask, docDomain,
                                                        thirdParty, sitekey,
                                                        specificOnly);
       }
     }
 
+    // If the type mask includes any whitelist-only types, we need to check the
+    // whitelist.
     if (blacklistHit || (typeMask & WHITELIST_ONLY_TYPES) != 0)
     {
       for (let i = 0, l = candidates.length; !whitelistHit && i < l; i++)
       {
         whitelistHit = this.whitelist._checkEntryMatch(candidates[i], location,
                                                        typeMask, docDomain,
                                                        thirdParty, sitekey);
       }
     }
 
@@ skipping 27 matching lines @@
 
 exports.CombinedMatcher = CombinedMatcher;
 
 /**
  * Shared {@link CombinedMatcher} instance that should usually be used.
  * @type {CombinedMatcher}
  */
 let defaultMatcher = new CombinedMatcher();
 
 exports.defaultMatcher = defaultMatcher;