Issue 3710 - Unify hostname logic

lib/abp2blocklist.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 74 matching lines @@
   let canSafelyMatchAsLowercase = false;
 
   for (let i = 0; i < text.length; i++)
   {
     let c = text[i];
 
     // If we're currently inside the hostname we have to be careful not to
     // escape any characters until after we have converted it to punycode.
     if (hostnameStart != null && !hostnameFinished)
     {
-      if (c == "*" || c == "^" || c == "?" || c == "/" || i == lastIndex)

[Sebastian Noack, 2016/02/27 20:30:38]

If you turn the logic here the other way around, you don't need an else block.

  if (c != "*" && c != "^" && c != "?" && c != "/" && i < lastIndex)
    continue;

[Sebastian Noack, 2016/02/27 20:30:38]

I'm not entirely sure if the case of last index is handled correctly. So if I
get the logic right you'd consider just "example.co" part of the hostname in
"||example.com".

[kzar, 2016/02/27 21:28:53]

Done.

[kzar, 2016/02/27 21:28:53]

Good point but it's even more complicated, what if we're at the last index but
also the current character is one that would have ended a hostname?! I've done
my best to keep the logic simple, but complexity crept in a bit.

-      {
-        hostnameFinished = true;
-        let hostname = text.substring(hostnameStart, i);
-        result.push(escapeRegExp(punycode.toASCII(hostname)));
-      }
-      else
+      let endingChar = (c == "*" || c == "^" ||
+                        c == "?" || c == "/" || c == "|");
+      if (!endingChar && i != lastIndex)
         continue;
+
+      let hostname = text.substring(hostnameStart, endingChar ? i : i + 1);
+      hostnameFinished = true;
+      result.push(escapeRegExp(punycode.toASCII(hostname)));
+      if (!endingChar)
+        break;
     }
 
     switch (c)
     {
       case "*":
         if (result.length > 0 && i < lastIndex && text[i + 1] != "*")
           result.push(".*");
         break;
       case "^":
         if (i < lastIndex)
           result.push(".");
         break;
       case "|":
         if (i == 0)
         {
           result.push("^");
           break;
         }
         if (i == lastIndex)
         {
           result.push("$");
           break;
         }
         if (i == 1 && text[0] == "|")
         {
-          result.push("https?://");
           hostnameStart = i + 1;
           canSafelyMatchAsLowercase = true;
+          result.push("https?://");
           break;
         }
         result.push("\\|");
         break;
       case "/":
-        result.push("/");

[Sebastian Noack, 2016/02/27 20:30:38]

Nit: Mind moving that line to just above the break, for consistency?

[kzar, 2016/02/27 21:28:53]

Done.

         if (!hostnameFinished &&

[Sebastian Noack, 2016/02/27 20:30:38]

Nit: It doesn't matter, but I personally find that it reads slightly better if
you have similar code aligned when wrapping:

  if (!hostnameFinished && text.charAt(i-2) == ":"
                        && text.charAt(i-1) == "/")

[kzar, 2016/02/27 21:28:53]

I'd rather leave this one as it is.

             text.charAt(i-2) == ":" && text.charAt(i-1) == "/")
         {
           hostnameStart = i + 1;
           canSafelyMatchAsLowercase = true;
         }
-        break;
-      case ".": case "+": case "$": case "{": case "}": case "?":

[Sebastian Noack, 2016/02/27 20:30:38]

Nit: I think the way this block was originally wrapped was a little nicer:

  case ".": case "+": case "$": case "?":
  case "{": case "}": case "(": case ")":
  case "[": case "]": case "\\":

But I guess it's personal preference, so I leave it up you.

[kzar, 2016/02/27 21:28:53]

Done.

-      case "(": case ")": case "[": case "]": case "\\":
+        result.push("/");
+        break;
+      case ".": case "+": case "$": case "?":
+      case "{": case "}": case "(": case ")":
+      case "[": case "]": case "\\":
         result.push("\\", c);
         break;
       default:
         if (hostnameFinished && (c >= "a" && c <= "z" ||
                                  c >= "A" && c <= "Z"))
           canSafelyMatchAsLowercase = false;
         result.push(c);
     }
   }
 
@@ skipping 278 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;
 };