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 364 matching lines @@
 
       this.domainSource = null;
     }
 
     Object.defineProperty(this, "domains", {value: domains, enumerable: true});
     return this.domains;
   },
 
   /**
    * 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.

+   * @return {Boolean} true in case of the filter being active
    */
-  isActiveOnDomain: function(/**String*/ docDomain) /**Boolean*/
+  isActiveOnDomain: function(docDomain, sitekey)
   {
-    // If no domains are set the rule matches everywhere
-    if (!this.domains)
+    // 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.

       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[""];
+    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 (!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();
 
-    if (this.ignoreTrailingDot)
-      docDomain = docDomain.replace(/\.+$/, "");
-    docDomain = docDomain.toUpperCase();
+        let domain = "";
+        while (true)
+        {
+          if (docDomain in this.domains)
+          {
+            domain = docDomain;
+            break;
+          }
 
-    while (true)
+          let nextDot = docDomain.indexOf(".");
+          if (nextDot < 0)
+            break;
+          docDomain = docDomain.substr(nextDot + 1);
+        }
+        activeDomain = this.domains[domain];
+      }
+    }
+
+    let activeSitekey = true;
+    if (this.sitekeys)
     {
-      if (docDomain in this.domains)
-        return this.domains[docDomain];
-
-      let nextDot = docDomain.indexOf(".");
-      if (nextDot < 0)
-        break;
-      docDomain = docDomain.substr(nextDot + 1);
+      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 this.domains[""];
+    return activeDomain && activeSitekey;
   },
 
   /**
    * 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 27 matching lines @@
 };
 
 /**
  * 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.

  * @constructor
  * @augments ActiveFilter
  */
-function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty)
+function RegExpFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys)
 {
-  ActiveFilter.call(this, text, domains);
+  ActiveFilter.call(this, text, domains, sitekeys);
 
   if (contentType != null)
     this.contentType = contentType;
   if (matchCase)
     this.matchCase = matchCase;
   if (thirdParty != null)
     this.thirdParty = thirdParty;
+  if (sitekeys != null)
+    this.sitekeySource = sitekeys;
 
   if (regexpSource.length >= 2 && regexpSource[0] == "/" && regexpSource[regexpSource.length - 1] == "/")
   {
     // The filter is a regular expression - convert it immediately to catch syntax errors
     let regexp = new RegExp(regexpSource.substr(1, regexpSource.length - 2), this.matchCase ? "" : "i");
     Object.defineProperty(this, "regexp", {value: regexp});
   }
   else
   {
     // No need to convert this filter to regular expression yet, do it on demand
@@ skipping 61 matching lines @@
    * @type Boolean
    */
   matchCase: false,
   /**
    * Defines whether the filter should apply to third-party or first-party content only. Can be null (apply to all content).
    * @type Boolean
    */
   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
+   */
+  get sitekeys()
+  {
+    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.

+
+      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
    * @param {String} contentType content type identifier of the URL
    * @param {String} docDomain domain name of the document that loads the URL
    * @param {Boolean} thirdParty should be true if the URL is a third-party request
+   * @param {String} sitekey public key provided by the document
    * @return {Boolean} true in case of a match
    */
-  matches: function(location, contentType, docDomain, thirdParty)
+  matches: function(location, contentType, docDomain, thirdParty, sitekey)
   {
     if (this.regexp.test(location) &&
         (RegExpFilter.typeMap[contentType] & this.contentType) != 0 &&
         (this.thirdParty == null || this.thirdParty == thirdParty) &&
-        this.isActiveOnDomain(docDomain))
+        this.isActiveOnDomain(docDomain, sitekey))
     {
       return true;
     }
 
     return false;
   }
 };
 
 // Required to optimize Matcher, see also RegExpFilter.prototype.length
 Object.defineProperty(RegExpFilter.prototype, "0",
@@ skipping 11 matching lines @@
   let origText = text;
   if (text.indexOf("@@") == 0)
   {
     blocking = false;
     text = text.substr(2);
   }
 
   let contentType = null;
   let matchCase = null;
   let domains = null;
-  let siteKeys = null;
+  let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
   let options;
   let match = (text.indexOf("$") >= 0 ? Filter.optionsRegExp.exec(text) : null);
   if (match)
   {
     options = match[1].toUpperCase().split(",");
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
@@ skipping 25 matching lines @@
         domains = value;
       else if (option == "THIRD_PARTY")
         thirdParty = true;
       else if (option == "~THIRD_PARTY")
         thirdParty = false;
       else if (option == "COLLAPSE")
         collapse = true;
       else if (option == "~COLLAPSE")
         collapse = false;
       else if (option == "SITEKEY" && typeof value != "undefined")
-        siteKeys = value.split(/\|/);
+        sitekeys = value;
       else
         return new InvalidFilter(origText, "Unknown option " + option.toLowerCase());
     }
   }
 
   if (!blocking && (contentType == null || (contentType & RegExpFilter.typeMap.DOCUMENT)) &&
       (!options || options.indexOf("DOCUMENT") < 0) && !/^\|?[\w\-]+:/.test(text))
   {
     // Exception filters shouldn't apply to pages by default unless they start with a protocol name
     if (contentType == null)
       contentType = RegExpFilter.prototype.contentType;
     contentType &= ~RegExpFilter.typeMap.DOCUMENT;
   }
-  if (!blocking && siteKeys)
-    contentType = RegExpFilter.typeMap.DOCUMENT;
 
   try
   {
     if (blocking)
-      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, collapse);
+      return new BlockingFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys, collapse);
     else
-      return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, siteKeys);
+      return new WhitelistFilter(origText, text, contentType, matchCase, domains, thirdParty, sitekeys);
   }
   catch (e)
   {
     return new InvalidFilter(origText, e);
   }
 }
 
 /**
  * Maps type strings like "SCRIPT" or "OBJECT" to bit masks
  */
@@ skipping 23 matching lines @@
 RegExpFilter.prototype.contentType &= ~(RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP);
 
 /**
  * Class for blocking filters
  * @param {String} text see Filter()
  * @param {String} regexpSource see RegExpFilter()
  * @param {Number} contentType see RegExpFilter()
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
+ * @param {String} sitekeys see RegExpFilter()
  * @param {Boolean} collapse  defines whether the filter should collapse blocked content, can be null
  * @constructor
  * @augments RegExpFilter
  */
-function BlockingFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, collapse)
+function BlockingFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys, collapse)
 {
-  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty);
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys);
 
   this.collapse = collapse;
 }
 exports.BlockingFilter = BlockingFilter;
 
 BlockingFilter.prototype =
 {
   __proto__: RegExpFilter.prototype,
 
   /**
    * Defines whether the filter should collapse blocked content. Can be null (use the global preference).
    * @type Boolean
    */
   collapse: null
 };
 
 /**
  * Class for whitelist filters
  * @param {String} text see Filter()
  * @param {String} regexpSource see RegExpFilter()
  * @param {Number} contentType see RegExpFilter()
  * @param {Boolean} matchCase see RegExpFilter()
  * @param {String} domains see RegExpFilter()
  * @param {Boolean} thirdParty see RegExpFilter()
- * @param {String[]} siteKeys public keys of websites that this filter should apply to
+ * @param {String} sitekeys see RegExpFilter()
  * @constructor
  * @augments RegExpFilter
  */
-function WhitelistFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, siteKeys)
+function WhitelistFilter(text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys)
 {
-  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty);
-
-  if (siteKeys != null)
-    this.siteKeys = siteKeys;
+  RegExpFilter.call(this, text, regexpSource, contentType, matchCase, domains, thirdParty, sitekeys);
 }
 exports.WhitelistFilter = WhitelistFilter;
 
 WhitelistFilter.prototype =
 {
-  __proto__: RegExpFilter.prototype,
-
-  /**
-   * List of public keys of websites that this filter should apply to
-   * @type String[]
-   */
-  siteKeys: null
+  __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} selector   CSS selector for the HTML elements that should be hidden
  * @constructor
  * @augments ActiveFilter
  */
@@ skipping 119 matching lines @@
 function ElemHideException(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideException = ElemHideException;
 
 ElemHideException.prototype =
 {
   __proto__: ElemHideBase.prototype
 };