Issue 6735 - Store domains in lower case

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 369 matching lines @@
   domainSource: null,
 
   /**
    * Separator character used in domainSource property, must be
    * overridden by subclasses
    * @type {string}
    */
   domainSeparator: null,
 
   /**
-   * Determines whether domainSource is already upper-case,
+   * Determines whether domainSource is already lower-case,
    * can be overridden by subclasses.
    * @type {boolean}
    */
-  domainSourceIsUpperCase: false,
+  domainSourceIsLowerCase: 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>}
    */
   get domains()
   {
     let domains = null;
 
     if (this.domainSource)
     {
       let source = this.domainSource;
-      if (!this.domainSourceIsUpperCase)
+      if (!this.domainSourceIsLowerCase)
       {
-        // RegExpFilter already have uppercase domains
-        source = source.toUpperCase();
+        // RegExpFilter already have lowercase domains
+        source = source.toLowerCase();
       }
       let list = source.split(this.domainSeparator);
       if (list.length == 1 && list[0][0] != "~")
       {
         // Fast track for the common one-domain scenario
         domains = new Map([["", false], [list[0], true]]);
       }
       else
       {
         let hasIncludes = false;
@@ skipping 56 matching lines @@
 
     // 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("");
 
-    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
+    docDomain = docDomain.replace(/\.+$/, "").toLowerCase();
 
     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(""))
       return false;
 
-    docDomain = docDomain.replace(/\.+$/, "").toUpperCase();
+    docDomain = docDomain.replace(/\.+$/, "").toLowerCase();
 
     for (let [domain, isIncluded] of this.domains)
     {
       if (isIncluded && domain != docDomain)
       {
         if (domain.length <= docDomain.length)
           return false;
 
         if (!domain.endsWith("." + docDomain))
           return false;
@@ skipping 80 matching lines @@
   else
   {
     // No need to convert this filter to regular expression yet, do it on demand
     this.regexpSource = regexpSource;
   }
 }
 exports.RegExpFilter = RegExpFilter;
 
 RegExpFilter.prototype = extend(ActiveFilter, {
   /**
-   * @see ActiveFilter.domainSourceIsUpperCase
+   * @see ActiveFilter.domainSourceIsLowerCase
    */
-  domainSourceIsUpperCase: true,
+  domainSourceIsLowerCase: true,
 
   /**
    * Number of filters contained, will always be 1 (required to
    * optimize Matcher).
    * @type {number}
    */
   length: 1,
 
   /**
    * @see ActiveFilter.domainSeparator
@@ skipping 120 matching lines @@
     text = match.input.substr(0, match.index);
     for (let option of options)
     {
       let value = null;
       let separatorIndex = option.indexOf("=");
       if (separatorIndex >= 0)
       {
         value = option.substr(separatorIndex + 1);
         option = option.substr(0, separatorIndex);
       }
-      option = option.replace(/-/, "_").toUpperCase();
-      if (option in RegExpFilter.typeMap)
+      let type = (option[0] == "~" ? option.substr(1) : option)

[kzar, 2018/06/07 13:16:02]

Not sure it matters especially but it seems kinda wasteful that we check
`option[0] == "~"` twice. If we assigned it to a variable it would also be an
opportunity to give it a useful name like `inverseOption` or similar which would
help document the code a little bit.

[Manish Jethani, 2018/06/08 05:00:40]

Done.

+                 .replace(/-/, "_").toUpperCase();
+      option = option.toLowerCase();
+      if (type in RegExpFilter.typeMap)
       {
-        if (contentType == null)
-          contentType = 0;
-        contentType |= RegExpFilter.typeMap[option];
+        if (option[0] == "~")
+        {
+          if (contentType == null)
+            ({contentType} = RegExpFilter.prototype);
+          contentType &= ~RegExpFilter.typeMap[type];
+        }
+        else
+        {
+          if (contentType == null)
+            contentType = 0;
+          contentType |= RegExpFilter.typeMap[type];
 
-        if (option == "CSP" && value)
-          csp = value;
+          if (type == "CSP" && value)

[kzar, 2018/06/07 13:16:02]

I found the variable names `type` vs `contentType` kinda confusing initially. I
understand the logic now that I'm part way through a second coffee but I wonder
if we could just get rid of the `type` variable? If I got it right we only use
it for looking up the content type and to check if it's "CSP" here.

How about we avoid saving the type string, instead saving the content type right
away and then here checking if the content type is RegExpFilter.typeMap.CSP? It
would avoid a string comparison too.

[Manish Jethani, 2018/06/08 05:00:41]

Done.

+            csp = value;
+        }
       }
-      else if (option[0] == "~" && option.substr(1) in RegExpFilter.typeMap)
-      {
-        if (contentType == null)
-          ({contentType} = RegExpFilter.prototype);
-        contentType &= ~RegExpFilter.typeMap[option.substr(1)];

[kzar, 2018/06/07 13:16:02]

You know reading your new logic I was trying to figure out how the old logic
actually worked. I think your version is an improvement for sure, it sucked how
we looked up the content type in all these different places.

-      }
-      else if (option == "MATCH_CASE")
+      else if (option == "match-case")
         matchCase = true;
-      else if (option == "~MATCH_CASE")
+      else if (option == "~match-case")

[kzar, 2018/06/07 13:16:02]

If we added the `inverseOption` variable we could simplify this logic (same
below):

else if (type == "MATCH_CASE")
  matchCase = !inverseOption;

(The down side is that contradicts what I said above about getting rid of the
`type` variable.)

[Manish Jethani, 2018/06/08 05:00:41]

Done.
I've managed to do both, good suggestions!

         matchCase = false;
-      else if (option == "DOMAIN" && value)
-        domains = value.toUpperCase();
-      else if (option == "THIRD_PARTY")
+      else if (option == "domain" && value)
+        domains = value.toLowerCase();
+      else if (option == "third-party")
         thirdParty = true;
-      else if (option == "~THIRD_PARTY")
+      else if (option == "~third-party")
         thirdParty = false;
-      else if (option == "COLLAPSE")
+      else if (option == "collapse")
         collapse = true;
-      else if (option == "~COLLAPSE")
+      else if (option == "~collapse")
         collapse = false;
-      else if (option == "SITEKEY" && value)
+      else if (option == "sitekey" && value)
         sitekeys = value.toUpperCase();
-      else if (option == "REWRITE" && value)
+      else if (option == "rewrite" && value)
         rewrite = value;
       else
         return new InvalidFilter(origText, "filter_unknown_option");
     }
   }
 
   // For security reasons, never match $rewrite filters
   // against requests that might load any code to be executed.
   if (rewrite != null)
   {
@@ skipping 281 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });