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

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -382,32 +382,63 @@
 
   /**
    * 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;
   },
 
   /**
@@ -455,12 +486,13 @@
  * @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;
@@ -468,6 +500,8 @@
     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] == "/")
   {
@@ -549,19 +583,80 @@
   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;
     }
@@ -593,7 +688,7 @@
   let contentType = null;
   let matchCase = null;
   let domains = null;
-  let siteKeys = null;
+  let sitekeys = null;
   let thirdParty = null;
   let collapse = null;
   let options;
@@ -639,7 +734,7 @@
       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());
     }
@@ -653,15 +748,13 @@
       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)
   {
@@ -705,13 +798,14 @@
  * @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;
 }
@@ -736,28 +830,19 @@
  * @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
 }
 
 /**