Issue 6727 - Use string rather than map for single-domain filters

lib/filterClasses.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 382 matching lines @@
   ignoreTrailingDot: true,
 
   /**
    * Determines whether domainSource is already upper-case,
    * can be overridden by subclasses.
    * @type {boolean}
    */
   domainSourceIsUpperCase: false,
 
   /**
-   * Map containing domains that this filter should match on/not match
-   * on or null if the filter should match on all domains
-   * @type {?Map.<string,boolean>}
+   * String specifying the domain that this filter should match on, or map
+   * containing domains that this filter should match on/not match on, or null
+   * if the filter should match on all domains
+   * @type {?Map.<string,boolean>|string}
    */
   get domains()
   {
     // Despite this property being cached, the getter is called
     // several times on Safari, due to WebKit bug 132872
     let prop = Object.getOwnPropertyDescriptor(this, "domains");
     if (prop)
       return prop.value;
 
     let domains = null;
 
     if (this.domainSource)
     {
       let source = this.domainSource;
       if (!this.domainSourceIsUpperCase)
       {
         // RegExpFilter already have uppercase domains
         source = source.toUpperCase();
       }
       let list = source.split(this.domainSeparator);
       if (list.length == 1 && list[0][0] != "~")
       {
         // Fast track for the common one-domain scenario
         if (this.ignoreTrailingDot)
           list[0] = list[0].replace(/\.+$/, "");
-        domains = new Map([["", false], [list[0], true]]);
+        domains = list[0];
       }
       else
       {
         let hasIncludes = false;
         for (let i = 0; i < list.length; i++)
         {
           let domain = list[i];
           if (this.ignoreTrailingDot)
             domain = domain.replace(/\.+$/, "");
           if (domain == "")
@@ skipping 50 matching lines @@
       return false;
     }
 
     // If no domains are set the rule matches everywhere
     if (!this.domains)
       return true;
 
     // If the document has no host name, match only if the filter
     // isn't restricted to specific domains
     if (!docDomain)
-      return this.domains.get("");
+      return typeof this.domains != "string" && this.domains.get("");
 
     if (this.ignoreTrailingDot)
       docDomain = docDomain.replace(/\.+$/, "");
     docDomain = docDomain.toUpperCase();
 
+    if (typeof this.domains == "string")
+    {
+      return docDomain == this.domains ||
+             docDomain.endsWith("." + this.domains);
+    }
+
     while (true)
     {
       let isDomainIncluded = this.domains.get(docDomain);
       if (typeof isDomainIncluded != "undefined")
         return isDomainIncluded;
 
       let nextDot = docDomain.indexOf(".");
       if (nextDot < 0)
         break;
       docDomain = docDomain.substr(nextDot + 1);
     }
     return this.domains.get("");
   },
 
   /**
    * Checks whether this filter is active only on a domain and its subdomains.
    * @param {string} docDomain
    * @return {boolean}
    */
   isActiveOnlyOnDomain(docDomain)
   {
-    if (!docDomain || !this.domains || this.domains.get(""))
+    if (!docDomain || !this.domains ||
+        typeof this.domains != "string" && this.domains.get(""))

[sergei, 2018/05/28 09:22:07]

IMO, perhaps it would be safer to rather check that `this.domains instanceof
Map` instead of typeof.

BTW, despite it's correct, wouldn't it be better to wrap the logical AND into
the braces () in order to improve readability because I'm not sure how many
people actually remember that && has a higher priority than || in JS?

[kzar, 2018/06/05 16:51:30]

Disagree on this one, people that don't understand JavaScript's logical
operators shouldn't really be messing with this code IMO.

[Manish Jethani, 2018/06/06 11:16:31]

This is one of those things that can lead to an endless debate, so in the
interest of consistency I'll leave it as it is. It seems in the rest of the code
we assume knowledge of logical operator precedence. I personally do prefer to
group things logically though.

[Manish Jethani, 2018/06/06 12:31:18]

typeof is typically faster than instanceof, so where there's a choice I think it
makes sense to use the former.

+    {
       return false;
+    }
 
     if (this.ignoreTrailingDot)
       docDomain = docDomain.replace(/\.+$/, "");
     docDomain = docDomain.toUpperCase();
 
+    if (typeof this.domains == "string")
+    {
+      return docDomain == this.domains ||
+             this.domains.endsWith("." + docDomain);
+    }
+
     for (let [domain, isIncluded] of this.domains)
     {
       if (isIncluded && domain != docDomain)
       {
         if (domain.length <= docDomain.length)
           return false;
 
         if (!domain.endsWith("." + docDomain))
           return false;
       }
     }
 
     return true;
   },
 
   /**
    * Checks whether this filter is generic or specific
    * @return {boolean}
    */
   isGeneric()
   {
     return !(this.sitekeys && this.sitekeys.length) &&
-            (!this.domains || this.domains.get(""));
+            (!this.domains ||
+             typeof this.domains != "string" && this.domains.get(""));
   },
 
   /**
    * See Filter.serialize()
    * @inheritdoc
    */
   serialize(buffer)
   {
     if (this._disabled || this._hitCount || this._lastHit)
     {
@@ skipping 545 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });