Issue 1853 - Moved filter generation into background page

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -24,30 +24,6 @@
 var lastRightClickEvent = null;
 var lastRightClickEventValid = false;
 
-function escapeChar(chr)
-{
-  var code = chr.charCodeAt(0);
-
-  // Control characters and leading digits must be escaped based on
-  // their char code in CSS. Moreover, curly brackets aren't allowed
-  // in elemhide filters, and therefore must be escaped based on their
-  // char code as well.
-  if (code <= 0x1F || code == 0x7F || /[\d\{\}]/.test(chr))
-    return "\\" + code.toString(16) + " ";
-
-  return "\\" + chr;
-}
-
-function quote(value)
-{
-  return '"' + value.replace(/["\\\{\}\x00-\x1F\x7F]/g, escapeChar) + '"';
-}
-
-function escapeCSS(s)
-{
-  return s.replace(/^[\d\-]|[^\w\-\u0080-\uFFFF]/g, escapeChar);
-}
-
 function highlightElement(element, shadowColor, backgroundColor)
 {
   unhighlightElement(element);
@@ -139,7 +115,7 @@
 {
   var url = element.getAttribute("data");
   if (url)
-    return [resolveURL(url)];
+    return [url];
 
   for (var i = 0; i < element.children.length; i++)
   {
@@ -158,7 +134,7 @@
     if (!value)
       continue;
 
-    return [resolveURL(value)];
+    return [value];
   }
 
   return [];
@@ -178,7 +154,7 @@
     {
       var url = candidates[i].trim().replace(/\s+\S+$/, "");
       if (url)
-        urls.push(resolveURL(url));
+        urls.push(url);
     }
   }
 
@@ -463,6 +439,26 @@
   }
 }
 
+function getFiltersForElement(element, callback)
+{
+  ext.backgroundPage.sendMessage(
+    {
+      type: "compose-filters",
+      tagName: element.localName,
+      id: element.id,
+      src: element.getAttribute("src"),
+      style: element.getAttribute("style"),
+      classes: [].slice.call(element.classList),
+      urls: getURLsFromElement(element),
+      baseURL: document.location.href
+    },
+    function(response)
+    {
+      callback(response.filters, response.selectors);
+    }
+  );
+}
+
 // When the user clicks, the currentElement is the one we want.
 // We should have ABP rules ready for when the
 // popup asks for them.
@@ -475,62 +471,8 @@
   if (currentElement.classList.contains("__adblockplus__overlay"))
     elt = currentElement.prisoner;
 
-  var clickHideFilters = [];
-  var selectorList = [];
-
-  var addSelector = function(selector)
+  getFiltersForElement(elt, function(filters, selectors)
   {
-    if (selectorList.indexOf(selector) != -1)
-      return;
-
-    clickHideFilters.push(document.domain + "##" + selector);
-    selectorList.push(selector);
-  };
-
-  if (elt.id)
-    addSelector("#" + escapeCSS(elt.id));
-
-  if (elt.classList.length > 0)
-  {
-    var selector = "";
-
-    for (var i = 0; i < elt.classList.length; i++)
-      selector += "." + escapeCSS(elt.classList[i]);
-
-    addSelector(selector);
-  }
-
-  var urls = getURLsFromElement(elt);
-  for (var i = 0; i < urls.length; i++)
-  {
-    var url = urls[i];
-
-    if (/^https?:/i.test(url))
-    {
-      var filter = url.replace(/^[\w\-]+:\/+(?:www\.)?/, "||");
-
-      if (clickHideFilters.indexOf(filter) == -1)
-        clickHideFilters.push(filter);
-
-      continue;
-    }
-
-    if (url == elt.src)
-      addSelector(escapeCSS(elt.localName) + '[src=' + quote(elt.getAttribute("src")) + ']');
-  }
-
-  // as last resort, create a filter based on inline styles
-  if (clickHideFilters.length == 0)
-  {
-    var style = elt.getAttribute("style");
-    if (style)
-      addSelector(escapeCSS(elt.localName) + '[style=' + quote(style) + ']');
-  }
-
-  // Show popup, or if inside frame tell the parent to do it
-  if (window.self == window.top)
-    clickHide_showDialog(e.clientX, e.clientY, clickHideFilters);
-  else
     ext.backgroundPage.sendMessage(
     {
       type: "forward",
@@ -539,15 +481,15 @@
         type: "clickhide-show-dialog",
         screenX: e.screenX,
         screenY: e.screenY,
-        clickHideFilters: clickHideFilters
+        clickHideFilters: filters
       }
     });
 

[kzar, 2015/01/26 10:35:16]

Why remove the comments explaining that we're highlighting the elements yellow /
red? I found those useful when first trying to understand the code last time.

[Sebastian Noack, 2015/01/26 10:44:07]

A while ago we agreed to don't spam code with comments that explain what's
already obvious. Calling functions called highlightElement(s) should be clear
enough that those elements are highlighted here. And the color doesn't matter to
understand the logic. Also the color is given in the code below as well.

[kzar, 2015/01/26 10:49:55]

These comments were useful and directly helped me to understand the code when I
was doing the block elements in iFrames ticket.

[Sebastian Noack, 2015/01/26 11:08:14]

Those comments doesn't add any information that aren't already given in those
two lines of code.

[kzar, 2015/01/26 14:21:28]

Yes they do. These two lines do not explain that anything is being highlighted
yellow and they only explain something is being highlighted red if you go and
look up the hex code #fd1708.

Also I found they gave a useful clue / implied some context that currentElement
was the thing that the user had last clicked on - before you understand the code
"current element" could be the element you last hovered over or pretty much
anything.

But either way, why isn't the fact that I personally found these comments useful
- just the other day - enough? Having a couple of comments here costs us
nothing. What's the point of arguing about them?

[Sebastian Noack, 2015/01/26 15:11:34]

Actually nothing is highlighted yellow here. highlightElements() uses just
another shade of red. This comment is outdated, another reason to get rid of it.
And if you tell me that the comment helped you to understand the code, that
merely means that you didn't understand the code but were misguided by the
comment, since the code is doing something different.
It's quite obvious, since the red octet is significantly larger than the green
and blue. So if you understand the RGB format you don't have to lookup anything
to know that this color is almost pure red. But as I said the color shouldn't
matter to understand the logic. It's actually a hack to use hard-coded colors.
No, the line in clickHide_mouseOver() where currentElement is set to
event.target gives you the clue that currentElement is the element the user
clicked on. It's irrelevant for the code we have here.

-  // Highlight the elements specified by selector in yellow
-  if (selectorList.length > 0)
-    highlightElements(selectorList.join(","));
-  // Now, actually highlight the element the user clicked on in red
-  highlightElement(currentElement, "#fd1708", "#f6a1b5");
+    if (selectors.length > 0)
+      highlightElements(selectors.join(","));
+
+    highlightElement(currentElement, "#fd1708", "#f6a1b5");
+  });
 
   // Make sure the browser doesn't handle this click
   e.preventDefault();