Issue 6741 - Use ES2015 classes in lib/matcher.js

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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 /**
  * @fileOverview Matcher class implementing matching addresses against
  *               a list of filters.
  */
 
-const {Filter, WhitelistFilter} = require("./filterClasses");
+const {WhitelistFilter} = require("./filterClasses");
 
 /**
  * Blacklist/whitelist filter matching
  */
 class Matcher
 {
   constructor()
   {
     /**
      * Lookup table for filters by their associated keyword
@@ skipping 66 matching lines @@
   }
 
   /**
    * Chooses a keyword to be associated with the filter
    * @param {Filter} filter
    * @returns {string} keyword or an empty string if no keyword could be found
    */
   findKeyword(filter)
   {
     let result = "";
-    let {text} = filter;
-    if (Filter.regexpRegExp.test(text))
+    let {pattern} = filter;
+    if (pattern == null)
       return result;
 
-    // Remove options
-    let match = Filter.optionsRegExp.exec(text);
-    if (match)
-      text = match.input.substr(0, match.index);
-
-    // Remove whitelist marker
-    if (text[0] == "@" && text[1] == "@")
-      text = text.substr(2);
-
-    let candidates = text.toLowerCase().match(
+    let candidates = pattern.toLowerCase().match(
       /[^a-z0-9%*][a-z0-9%]{3,}(?=[^a-z0-9%*])/g
     );
     if (!candidates)
       return result;
 
     let hash = this.filterByKeyword;
     let resultCount = 0xFFFFFF;
     let resultLength = 0;
     for (let i = 0, l = candidates.length; i < l; i++)
     {
@@ skipping 15 matching lines @@
    * Checks whether a particular filter is being matched against.
    * @param {RegExpFilter} filter
    * @returns {boolean}
    */
   hasFilter(filter)
   {
     return this.keywordByFilter.has(filter);
   }
 
   /**
-   * Returns the keyword used for a filter, null for unknown filters.

[Manish Jethani, 2018/09/03 18:46:49]

Let's make this `<code>null</code>` as well while we're at it.

[Jon Sonesen, 2018/09/05 14:05:27]

Done.

+   * Returns the keyword used for a filter, <code>null</code>
+   * for unknown filters.
    * @param {RegExpFilter} filter
    * @returns {?string}
    */
   getKeywordForFilter(filter)
   {
     let keyword = this.keywordByFilter.get(filter);
     return typeof keyword != "undefined" ? keyword : null;
   }
 
   /**
@@ skipping 104 matching lines @@
    */
   clear()
   {
     this.blacklist.clear();
     this.whitelist.clear();
     this.resultCache.clear();
   }
 
   /**
    * @see Matcher#add
    * @param {Filter} filter

[Manish Jethani, 2018/09/03 18:46:49]

When we use @see, we're not being consistent in adding @param tags. Here we're
adding them, but not for matchesAny for example. Let's just leave the @param
tags out here when using @see.

Also see my comment about using @implements in the long run.

[Jon Sonesen, 2018/09/05 14:05:27]

The reason for this is that matchesAny does not have a filter parameter, so
removing the @param tag causes eslint to complain since the definitions don't
match.

[Manish Jethani, 2018/09/05 19:33:57]

Oh damn. OK, no problem then.

    */
   add(filter)
   {
     if (filter instanceof WhitelistFilter)
       this.whitelist.add(filter);
     else
       this.blacklist.add(filter);
 
     this.resultCache.clear();
   }
@@ skipping 61 matching lines @@
     if (matcher.hasFilter(filter))
       return !matcher.getKeywordForFilter(filter);
     return !matcher.findKeyword(filter);
   }
 
   /**
    * Optimized filter matching testing both whitelist and blacklist matchers
    * simultaneously. For parameters see
      {@link Matcher#matchesAny Matcher.matchesAny()}.
    * @see Matcher#matchesAny
    * @inheritdoc

[Manish Jethani, 2018/09/03 18:46:49]

I don't think @inheritdoc makes sense here (also below), because CombinedMatches
does not in fact extend Matcher. It does nothing for the generated
documentation.

In the long run we should probably consider adding a Matcher interface and the
use @implements [1] to automatically inherit the documentation, but this would
be separate thing.

[1]: http://usejsdoc.org/tags-implements.html

[Jon Sonesen, 2018/09/05 14:05:27]

Similarly to above, eslint complains here without @inherit doc

    */
   matchesAnyInternal(location, typeMask, docDomain, thirdParty, sitekey,
                      specificOnly)
   {
     let candidates = location.toLowerCase().match(/[a-z0-9%]{3,}/g);
     if (candidates === null)
       candidates = [];
     candidates.push("");
 
     let blacklistHit = null;
@@ skipping 40 matching lines @@
     return result;
   }
 }
 
 exports.CombinedMatcher = CombinedMatcher;
 
 /**
  * Shared {@link CombinedMatcher} instance that should usually be used.
  * @type {CombinedMatcher}
  */
-exports.defaultMatcher = new CombinedMatcher();

[Manish Jethani, 2018/09/03 18:46:49]

I think we should follow the convention of first declaring something and then
exporting it. We're doing this at the end of lib/filterListener.js now.

  let defaultMatcher = new CombinedMatcher();

  exports.defaultMatcher = defaultMatcher;

[Jon Sonesen, 2018/09/05 14:05:27]

Done.

+let defaultMatcher = new CombinedMatcher();
+
+exports.defaultMatcher = defaultMatcher;