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 738 matching lines @@
     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);
       }
 
       let inverse = option[0] == "~";

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

I'm just calling it "inverse" because "option" is implied (this is the options
loop).

[kzar, 2018/06/08 09:30:32]

Acknowledged.

       if (inverse)
         option = option.substr(1);
 
       let type = RegExpFilter.typeMap[option.replace(/-/, "_").toUpperCase()];

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

Lookup happens only once in the RegExpFilter.typeMap object.

[kzar, 2018/06/08 09:30:32]

Nice.

       if (type)
       {
         if (inverse)
         {
           if (contentType == null)
             ({contentType} = RegExpFilter.prototype);
           contentType &= ~type;
         }
         else
         {
           contentType |= type;
 
           if (type == RegExpFilter.typeMap.CSP && value)

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

Integer comparison.

[Manish Jethani, 2018/06/08 14:21:14]

By the way in the case of csp we quietly ignore the option if it has no value,
so we are not being consistent. Anyway we should address these things
separately, not as part of this patch.

[kzar, 2018/06/15 10:44:03]

Yea, agreed. By the way Rosie is reimplementing this stuff in Python ATM, so if
you change the behaviour ping her in IRC.

             csp = value;
         }
       }
       else
       {
         switch (option.toLowerCase())

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

Only lowercase if we get this far.

switch is cleaner than if-else in my opinion, let me know if you disagree.

         {
           case "match-case":
             matchCase = !inverse;

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

!inverse

[kzar, 2018/06/08 09:30:31]

(Heh, it does make you wonder if storing !inverse as a variable would be better,
but I can't think of a good name.)

             break;
           case "domain":
-            if (value)

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

Note: This has a good side effect. Previously foo$domain would throw an error, I
don't see why it should. It should just apply to any domain in that case. I mean
the option should be ignored (it's not an "unknown option", it's a known option
with no value).

[kzar, 2018/06/08 09:30:32]

Seems reasonable to me, but I wonder if this was intentional. Do you have an
opinion Segei?

[sergei, 2018/06/08 13:28:11]

Formally, if the option is present and there are no values then the filter is
domain specific with no domains and simply put it should be never applied. BTW,
previously the code returned an instance of InvalidFilter, not thrown an
exception.

If it's a special case then it deserves a test IMO.

I would like to prefer to fail earlier in terms of rather mark the filter as
invalid, however if it conflicts with the idea to allow empty options, then it's
fine for me but could you please add a test for it?

[Manish Jethani, 2018/06/08 14:12:39]

For known options that must have a value but none is given, what do you think
about returning an invalid filter with a different reason like
"filter_missing_value" or "filter_invalid_value"? Previously the reason was
"filter_unknown_option" was not exactly correct.

Anyway, let's try to stay compatible here and not change the behavior, so I'll
try to keep it the same for now. Let me update the patch.

-              domains = value.toLowerCase();
+            if (!value)
+              return new InvalidFilter(origText, "filter_unknown_option");
+            domains = value.toLowerCase();
             break;
           case "third-party":
             thirdParty = !inverse;
             break;
           case "collapse":
             collapse = !inverse;
             break;
           case "sitekey":
-            if (value)
-              sitekeys = value.toUpperCase();
+            if (!value)

[Manish Jethani, 2018/06/08 14:19:42]

There's a bit of code repetition here but I'm OK with it.

[kzar, 2018/06/15 10:44:03]

Yea, I think it's fine.

+              return new InvalidFilter(origText, "filter_unknown_option");
+            sitekeys = value.toUpperCase();
             break;
           case "rewrite":
-            if (value)

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

Note that this also simplifies how we can accept the blank value for the rewrite
option in the other patch.

[kzar, 2018/06/08 09:30:31]

Acknowledged.

-              rewrite = value;
+            if (!value)

[Manish Jethani, 2018/06/08 14:19:42]

Note that this will get updated for the rewrite option in the other patch.

+              return new InvalidFilter(origText, "filter_unknown_option");
+            rewrite = value;
             break;
           default:
             return new InvalidFilter(origText, "filter_unknown_option");
         }
       }
     }
   }
 
   // For security reasons, never match $rewrite filters
   // against requests that might load any code to be executed.
@@ skipping 283 matching lines @@
  */
 function ElemHideEmulationFilter(text, domains, selector)
 {
   ElemHideBase.call(this, text, domains, selector);
 }
 exports.ElemHideEmulationFilter = ElemHideEmulationFilter;
 
 ElemHideEmulationFilter.prototype = extend(ElemHideBase, {
   type: "elemhideemulation"
 });