Issue 431/432 - Remove special handling for the $sitekey option

lib/filterClasses.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 197 matching lines @@
 
   /**
    * See Filter.serialize()
    */
   serialize: function(buffer) {}
 };
 
 /**
  * Abstract base class for filters that can get hits
  * @param {String} text see Filter()
- * @param {String} domains  (optional) Domains that the filter is restricted to separated by domainSeparator e.g. "foo.com|bar.com|~baz.com"
+ * @param {String} [domains] Domains that the filter is restricted to separated by domainSeparator e.g. "foo.com|bar.com|~baz.com"
  * @constructor
  * @augments Filter
  */
 function ActiveFilter(text, domains)
 {
   Filter.call(this, text);
 
   this.domainSource = domains;
 }
 exports.ActiveFilter = ActiveFilter;
@@ skipping 88 matching lines @@
    * @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 Object
    */
   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);
@@ skipping 37 matching lines @@
       }
 
       this.domainSource = null;
     }
 
     Object.defineProperty(this, "domains", {value: domains, enumerable: true});
     return this.domains;
   },
 
   /**
+   * Array containing public keys of websites that this filter should apply to
+   * @type Array of String
+   */
+  sitekeys: null,
+
+  /**
    * Checks whether this filter is active on a domain.
    * @param {String} docDomain domain name of the document that loads the URL
-   * @param {String} sitekey (optional) public key provided by the document

[Wladimir Palant, 2014/08/14 19:58:15]

Nit: Please indicate the optional parameter with square brackets.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+   * @param {String} [sitekey] public key provided by the document
    * @return {Boolean} true in case of the filter being active
    */
   isActiveOnDomain: function(docDomain, sitekey)
   {
-    // If no domains or sitekeys are set the rule matches everywhere
-    if (!this.domains && !this.sitekeys)

[Wladimir Palant, 2014/08/14 19:58:15]

I think there should be a |sitekey: null| property on ActiveFilter just to make
sure that you don't access an undefined property here (it will only be set
properly for RegExpFilter still).

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+    // Sitekeys are case-sensitive so we shouldn't convert them to upper-case to avoid false
+    // positives here. Instead we need to change the way filter options are parsed.
+    if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey.toUpperCase()) < 0))
+      return false;
+
+    // If no domains are set the rule matches everywhere
+    if (!this.domains)
       return true;
 
-    let activeDomain = true;

[Wladimir Palant, 2014/08/14 19:58:15]

This made the code significantly more complicated and quite frankly I don't even
want to validate that it is still correct. How about you simply deal with the
sitekeys at the very beginning of the function?

  if (this.sitekeys && (!sitekey || this.sitekeys.indexOf(sitekey) < 0)
    return false;

And then you can check the domains exactly the same way it was done before -
starting with |if (!domains)|.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

-    if (this.domains)
+    // If the document has no host name, match only if the filter isn't restricted to specific domains
+    if (!docDomain)
+      return this.domains[""];
+
+    if (this.ignoreTrailingDot)
+      docDomain = docDomain.replace(/\.+$/, "");
+    docDomain = docDomain.toUpperCase();
+
+    while (true)
     {
-      if (!docDomain)
-        // If the document has no host name, match only if the filter isn't restricted to specific domains
-        activeDomain = this.domains[""];
-      else
-      {
-        if (this.ignoreTrailingDot)
-          docDomain = docDomain.replace(/\.+$/, "");
-        docDomain = docDomain.toUpperCase();
-
-        let domain = "";
-        while (true)
-        {
-          if (docDomain in this.domains)
-          {
-            domain = docDomain;
-            break;
-          }
-
-          let nextDot = docDomain.indexOf(".");
-          if (nextDot < 0)
-            break;
-          docDomain = docDomain.substr(nextDot + 1);
-        }
-        activeDomain = this.domains[domain];
-      }
-    }
-
-    let activeSitekey = true;
-    if (this.sitekeys)
-    {
-      if (!sitekey)
-        // If the document has no sitekey, match only if the filter isn't restricted by sitekeys
-        activeSitekey = this.sitekeys[""];
-      else
-      {
-        let [key, signature] = sitekey.split("_", 2);

[Wladimir Palant, 2014/08/14 19:58:15]

There should be no signature at this point.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

-        let formattedKey = key.replace(/=/g, "").toUpperCase();
-        if (formattedKey in this.sitekeys)
-          activeSitekey = this.sitekeys[formattedKey];
-        else
-          activeSitekey = this.sitekeys[""];
-      }
-    }
-    return activeDomain && activeSitekey;
+      if (docDomain in this.domains)
+        return this.domains[docDomain];
+
+      let nextDot = docDomain.indexOf(".");
+      if (nextDot < 0)
+        break;
+      docDomain = docDomain.substr(nextDot + 1);
+    }
+    return this.domains[""];
   },
 
   /**
    * Checks whether this filter is active only on a domain and its subdomains.
    */
   isActiveOnlyOnDomain: function(/**String*/ docDomain) /**Boolean*/
   {
     if (!docDomain || !this.domains || this.domains[""])
       return false;
 
@@ skipping 23 matching lines @@
       if (this._lastHit)
         buffer.push("lastHit=" + this._lastHit);
     }
   }
 };
 
 /**
  * Abstract base class for RegExp-based filters
  * @param {String} text see Filter()
  * @param {String} regexpSource filter part that the regular expression should be build from
- * @param {Number} contentType  (optional) Content types the filter applies to, combination of values from RegExpFilter.typeMap
- * @param {Boolean} matchCase   (optional) Defines whether the filter should distinguish between lower and upper case letters
- * @param {String} domains      (optional) Domains that the filter is restricted to, e.g. "foo.com|bar.com|~baz.com"
- * @param {Boolean} thirdParty  (optional) Defines whether the filter should apply to third-party or first-party content only
- * @param {String} sitekeys     (optional) Public keys of websites that this filter should apply to, e.g. "foo|bar|~baz"

[Wladimir Palant, 2014/08/14 19:58:15]

Please remove the exceptions from the comment here.

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

+ * @param {Number} [contentType] Content types the filter applies to, combination of values from RegExpFilter.typeMap
+ * @param {Boolean} [matchCase] Defines whether the filter should distinguish between lower and upper case letters
+ * @param {String} [domains] Domains that the filter is restricted to, e.g. "foo.com|bar.com|~baz.com"
+ * @param {Boolean} [thirdParty] Defines whether the filter should apply to third-party or first-party content only
+ * @param {String} [sitekeys] Public keys of websites that this filter should apply to
  * @constructor
  * @augments ActiveFilter
  */
 function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys)
 {
   ActiveFilter.call(this, text, domains, sitekeys);
 
   if (contentType != null)
     this.contentType = contentType;
   if (matchCase)
@@ skipping 41 matching lines @@
    * Expression from which a regular expression should be generated - for delayed creation of the regexp property
    * @type String
    */
   regexpSource: null,
   /**
    * Regular expression to be used when testing against this filter
    * @type RegExp
    */
   get regexp()
   {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "regexp");
+    if (prop)
+      return prop.value;
+
     // Remove multiple wildcards
     let source = this.regexpSource
       .replace(/\*+/g, "*")        // remove multiple wildcards
       .replace(/\^\|$/, "^")       // remove anchors following separator placeholder
       .replace(/\W/g, "\\$&")      // escape special symbols
       .replace(/\\\*/g, ".*")      // replace wildcards by .*
       // process separator placeholders (all ANSI characters but alphanumeric characters and _%.-)
       .replace(/\\\^/g, "(?:[\\x00-\\x24\\x26-\\x2C\\x2F\\x3A-\\x40\\x5B-\\x5E\\x60\\x7B-\\x7F]|$)")
       .replace(/^\\\|\\\|/, "^[\\w\\-]+:\\/+(?!\\/)(?:[^\\/]+\\.)?") // process extended anchor at expression start
       .replace(/^\\\|/, "^")       // process anchor at expression start
@@ skipping 21 matching lines @@
    */
   thirdParty: null,
 
   /**
    * String that the sitekey property should be generated from
    * @type String
    */
   sitekeySource: null,
 
   /**
-   * Map containing public keys of websites that this filter should apply to
-   * @type Object
+   * Array containing public keys of websites that this filter should apply to
+   * @type Array of String
    */
   get sitekeys()
   {
+    // Despite this property being cached, the getter is called
+    // several times on Safari, due to WebKit bug 132872
+    let prop = Object.getOwnPropertyDescriptor(this, "sitekeys");
+    if (prop)
+      return prop.value;
+
     let sitekeys = null;
 
     if (this.sitekeySource)
     {
-      let source = this.sitekeySource;
-      let list = source.split("|");
-      if (list.length == 1 && list[0][0] != "~")
-      {
-        // Fast track for the common one-sitekey scenario
-        sitekeys = {__proto__: null, "": false};
-        sitekeys[list[0]] = true;
-      }
-      else
-      {
-        let hasIncludes = false;
-        for (let i = 0; i < list.length; i++)
-        {
-          let sitekey = list[i];
-          if (sitekey == "")
-            continue;
-
-          let include;
-          if (sitekey[0] == "~")
-          {
-            include = false;
-            sitekey = sitekey.substr(1);
-          }
-          else
-          {
-            include = true;
-            hasIncludes = true;
-          }
-
-          if (!sitekeys)
-            sitekeys = Object.create(null);
-
-          sitekeys[sitekey] = include;
-        }
-        sitekeys[""] = !hasIncludes;
-      }

[Wladimir Palant, 2014/08/14 19:58:15]

You don't need this complicated data structure if you don't have exceptions:

  sitekeys = this.sitekeySource.split("|");

[Thomas Greiner, 2014/08/29 15:57:29]

Done.

-
+      sitekeys = this.sitekeySource.split("|");
       this.sitekeySource = null;
     }
 
     Object.defineProperty(this, "sitekeys", {value: sitekeys, enumerable: true});
     return this.sitekeys;
   },
 
   /**
    * Tests whether the URL matches this filter
    * @param {String} location URL to be tested
@@ skipping 193 matching lines @@
 exports.WhitelistFilter = WhitelistFilter;
 
 WhitelistFilter.prototype =
 {
   __proto__: RegExpFilter.prototype
 }
 
 /**
  * Base class for element hiding filters
  * @param {String} text see Filter()
- * @param {String} domains    (optional) Host names or domains the filter should be restricted to
+ * @param {String} [domains] Host names or domains the filter should be restricted to
  * @param {String} selector   CSS selector for the HTML elements that should be hidden
  * @constructor
  * @augments ActiveFilter
  */
 function ElemHideBase(text, domains, selector)
 {
   ActiveFilter.call(this, text, domains || null);
 
   if (domains)
     this.selectorDomain = domains.replace(/,~[^,]+/g, "").replace(/^~[^,]+,?/, "").toLowerCase();
@@ skipping 113 matching lines @@
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideException = ElemHideException;
 
 ElemHideException.prototype =
 {
   __proto__: ElemHideBase.prototype
 };