Noissue - Remove unnecessary checks for specific-only matches

lib/matcher.js

Index: lib/matcher.js
===================================================================
--- a/lib/matcher.js
+++ b/lib/matcher.js
@@ -307,28 +307,25 @@
    * @returns {?Filter}
    * @protected
    */
   checkEntryMatch(keyword, location, typeMask, docDomain, thirdParty, sitekey,
                   specificOnly, collection)
   {
     // We need to skip the simple (location-only) filters if the type mask does
     // not contain any default content types.
-    if ((typeMask & DEFAULT_TYPES) != 0)
+    if (!specificOnly && (typeMask & DEFAULT_TYPES) != 0)

[Manish Jethani, 2019/02/06 11:18:20]

These are all generic filters so the code should not enter here if specificOnly
is true

     {
       let simpleSet = this._simpleFiltersByKeyword.get(keyword);
       if (simpleSet)
       {
         let lowerCaseLocation = location.toLowerCase();
 
         for (let filter of simpleSet)
         {
-          if (specificOnly && !(filter instanceof WhitelistFilter))
-            continue;
-
           if (filter.matchesLocation(location, lowerCaseLocation))
           {
             if (!collection)
               return filter;
 
             collection.push(filter);
           }
         }
@@ -352,18 +349,17 @@
     {
       complexSet = this._complexFiltersByKeyword.get(keyword);
     }
 
     if (complexSet)
     {
       for (let filter of complexSet)
       {
-        if (specificOnly && filter.isGeneric() &&
-            !(filter instanceof WhitelistFilter))

[Manish Jethani, 2019/02/06 11:18:20]

It's up to the caller to not pass the specific-only flag for whitelist filters.

[hub, 2019/02/06 15:46:44]

I tend to think we should protect ourselves from the caller.

[Manish Jethani, 2019/02/06 16:16:00]

Yeah, but my point was that we should not expose the Matcher class directly. If
the caller wants the correct ad blocking behavior, they should use
CombinedMatcher. If they want "raw" filter matching, then they can use Matcher
but then it's just a dumb object and follows a fixed script. The nuance is in
CombinedMatcher.

[hub, 2019/02/06 17:47:56]

Fair enough.

+        if (specificOnly && filter.isGeneric())
           continue;
 
         if (filter.matches(location, typeMask, docDomain, thirdParty, sitekey))
         {
           if (!collection)
             return filter;
 
           collection.push(filter);
@@ -669,21 +665,20 @@
   }
 
   /**
    * Tests whether the URL is whitelisted
    * @see Matcher#matchesAny
    * @inheritdoc
    * @returns {boolean}
    */
-  isWhitelisted(location, typeMask, docDomain, thirdParty, sitekey,
-                specificOnly)
+  isWhitelisted(location, typeMask, docDomain, thirdParty, sitekey)
   {
     return !!this._whitelist.matchesAny(location, typeMask, docDomain,
-                                        thirdParty, sitekey, specificOnly);
+                                        thirdParty, sitekey);
   }
 }
 
 exports.CombinedMatcher = CombinedMatcher;
 
 /**
  * Shared {@link CombinedMatcher} instance that should usually be used.
  * @type {CombinedMatcher}