Issue 3670 - Make rules case sensitive where possible

lib/contentBlockerList.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 47 matching lines @@
   let included = [];
   let excluded = [];
   let rules = [];
 
   parseDomains(filter.domains, included, excluded);
 
   if (excluded.length == 0 && !(filter.selector in elemhideSelectorExceptions))
     return {matchDomains: included.map(matchDomain), selector: filter.selector};
 }
 
-// Convert the "regexpSource" part of a filter's text to a regular expression,
-// also deciding if the expression can safely be converted to and matched as
-// lowercase.
+/**
+ * Convert the given filter "regexpSource" string into a regular expression.
+ * (Also deciding if the regular expression can be safely converted to and
+ *  matched as lower case or not.)
+ *
+ * @param   {string} text regexpSource property of a filter
+ * @returns {object} An object containing a regular expression string and a bool
+ *                   indicating if the filter can be safely matched as lower
+ *                   case: {regexp: "...", caseSenstive: true/false}
+ */
 function toRegExp(text)
 {
   let result = [];
   let lastIndex = text.length - 1;
   let hostnameStarted = false;
   let hostnameFinished = false;
   let caseSensitive = false;
 
   for (let i = 0; i < text.length; i++)
   {
@@ skipping 19 matching lines @@
           result.push("^");
           break;
         }
         if (i == lastIndex)
         {
           result.push("$");
           break;
         }
         if (i == 1 && text[0] == "|")
         {
+          hostnameStarted = caseSensitive = true;
           result.push("https?://");
-          hostnameStarted = caseSensitive = true;

[Sebastian Noack, 2016/02/24 22:53:49]

Nit: Sometimes you have the regular expression conversion logic first, sometimes
you have case sensitivity logic first. Mind doing it consistently?

[kzar, 2016/02/24 23:15:20]

Done.

           break;
         }
+        result.push("\\", c);
+        break;
+      case "?":
+        if (hostnameStarted)
+          hostnameFinished = true;
+      case ".": case "+": case "$": case "{": case "}":
+      case "(": case ")": case "[": case "]": case "\\":
         result.push("\\", c);
         break;
       case "/":
         if (hostnameStarted)
           hostnameFinished = true;
         else if (text.charAt(i-2) == ":" && text.charAt(i-1) == "/")
           hostnameStarted = caseSensitive = true;
-        result.push("/");

[Sebastian Noack, 2016/02/24 22:53:49]

If we move this case just above the default case, we could simply pass through,
and wouldn't need |result.push("/");break;| here.

[kzar, 2016/02/24 23:15:20]

Done.

-        break;
-      case "?":
-        if (hostnameStarted)
-          hostnameFinished = true;
-      case ".": case "+": case "$": case "{":

[Sebastian Noack, 2016/02/24 22:53:48]

Nit: It doesn't really matter, but you wrap after the 5th (instead 4th) case, we
have an equal number of cases per line, and the parantheses/braces/etc are on
the same line as their corresponding counter part. ;)

[kzar, 2016/02/24 23:15:20]

Done.

-      case "}": case "(": case ")": case "[":
-      case "]": case "\\":
-        result.push("\\", c);
-        break;
       default:
         if (hostnameFinished && (c >= "a" && c <= "z" ||
                                  c >= "A" && c <= "Z"))
           caseSensitive = false;
         result.push(c);
     }
   }
 
   return {regexp: result.join(""), caseSensitive: caseSensitive};
 }
 
 function getRegExpTrigger(filter)
 {
   let result = toRegExp(filter.regexpSource.replace(
     // Safari expects punycode, filter lists use unicode
     /^(\|\||\|?https?:\/\/)([\w\-.*\u0080-\uFFFF]+)/i,
     function (match, prefix, domain)
     {
       return prefix + punycode.toASCII(domain);
     }
   ));
 
   let trigger = {"url-filter": result.regexp};
 
   // Limit rules to to HTTP(S) URLs
   if (!/^(\^|http)/i.test(trigger["url-filter"]))
     trigger["url-filter"] = "^https?://.*" + trigger["url-filter"];
 
   // For rules containing only a hostname we know that we're matching against
-  // a lowercase string and can therefore enable case sensitive matching.
-  if (result.caseSensitive)

[Sebastian Noack, 2016/02/24 22:53:49]

I think the logic here would be a little more straight forward if you put it
like that:

  if (result.caseSensitive)
    trigger["url-filter"] = trigger["url-filter"].toLowerCase();

  if (result.caseSensitive || filter.matchCase)
    trigger["url-filter-is-case-sensitive"] = true;

Note that this also simplifies the calling code.

(Moreover, I'd personally use a variable for the regexp rather than duplicating
the object lookup, but I leave that up to you.)

[Sebastian Noack, 2016/02/24 23:07:37]

Ah wait, the logic is incorrect anyway, with either approach of us. If
$match_case is requested, we shouldn't convert the filter to lower case. It's
more a theoretical case as a filter like ||EXAMPLE.COM^$match_case would never
match anything, but we should behave consistent with Adblock Plus here. So the
logic is supposed to be:

  if (result.caseSensitive && !filter.matchCase)
    trigger["url-filter"] = trigger["url-filter"].toLowerCase();

  if (result.caseSensitive || filter.matchCase)
    trigger["url-filter-is-case-sensitive"] = true;

[kzar, 2016/02/24 23:15:20]

Done.

-  {
+  // a lowercase string unless the matchCase option was passed.
+  if (result.caseSensitive && !filter.matchCase)
     trigger["url-filter"] = trigger["url-filter"].toLowerCase();
+
+  if (result.caseSensitive || filter.matchCase)
     trigger["url-filter-is-case-sensitive"] = true;
-  }
 
   return trigger;
 }
 
 function getResourceTypes(filter)
 {
   let types = [];
 
   if (filter.contentType & typeMap.IMAGE)
     types.push("image");
@@ skipping 32 matching lines @@
 
   return result;
 }
 
 function convertFilter(filter, action, withResourceTypes)
 {
   let trigger = getRegExpTrigger(filter);
   let included = [];
   let excluded = [];
 
-  if (filter.matchCase)
-    trigger["url-filter-is-case-sensitive"] = true;
-
- parseDomains(filter.domains, included, excluded);
+  parseDomains(filter.domains, included, excluded);
 
   if (withResourceTypes)
     trigger["resource-type"] = getResourceTypes(filter);
   if (filter.thirdParty != null)
     trigger["load-type"] = [filter.thirdParty ? "third-party" : "first-party"];
 
   if (included.length > 0)
     trigger["if-domain"] = addDomainPrefix(included);
   else if (excluded.length > 0)
     trigger["unless-domain"] = addDomainPrefix(excluded);
@@ skipping 194 matching lines @@
 
   for (let filter of this.elemhideExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", false));
   for (let filter of this.requestFilters)
     addRule(convertFilter(filter, "block", true));
   for (let filter of this.requestExceptions)
     addRule(convertFilter(filter, "ignore-previous-rules", true));
 
   return rules;
 };