Noissue - Remove unnecessary checks for specific-only matches

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 294 matching lines @@
    *   matching filters to the list; if omitted, the function directly returns
    *   the first matching filter.
    * @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);
           }
         }
       }
     }
@@ skipping 13 matching lines @@
     }
     else
     {
       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);
         }
       }
@@ skipping 295 matching lines @@
 
     return result;
   }
 
   /**
    * 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}
  */
 let defaultMatcher = new CombinedMatcher();
 
 exports.defaultMatcher = defaultMatcher;