[adblockpluschrome] Issue 3596 - Added support for CSS property filters to devtools panel

include.preload.js

Index: include.preload.js
diff --git a/include.preload.js b/include.preload.js
index 86e52c32afceb894f6591a84b0eca3b1842383ce..a2840bc6a4a66b56476d4b94a78ebd15c8da589a 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -192,9 +192,10 @@ function getContentDocument(element)
   }
 }
 
-function ElementHidingTracer(selectors)
+function ElementHidingTracer()
 {
-  this.selectors = selectors;
+  this.selectors = [];
+  this.filters = [];
 
   this.changedNodes = [];
   this.timeout = null;
@@ -208,48 +209,53 @@ function ElementHidingTracer(selectors)
     this.trace();
 }
 ElementHidingTracer.prototype = {
-  checkNodes: function(nodes)
+ addSelectors: function(selectors, filters)

[Sebastian Noack, 2017/02/17 13:55:31]

Nit: The indentation is one space off here.

[wspee, 2017/02/18 11:20:23]

Done.

   {
-    var matchedSelectors = [];
+    if (!filters)
+      filters = new Array(selectors.length);
 
-    // Find all selectors that match any hidden element inside the given nodes.
-    for (var i = 0; i < this.selectors.length; i++)
-    {
-      var selector = this.selectors[i];
+    if (document.readyState != "loading")
+      this.checkNodes([document], selectors, filters);
+
+    this.selectors.push(...selectors);
+    this.filters.push(...filters);
+  },
+
+  checkNodes: function(nodes, selectors, filters)
+  {
+    let matchedFilters = [];
 
-      for (var j = 0; j < nodes.length; j++)
+    for (let i = 0; i < selectors.length; i++)
+    {
+      nodes: for (let node of nodes)
       {
-        var elements = nodes[j].querySelectorAll(selector);
-        var matched = false;
+        let elements = node.querySelectorAll(selectors[i]);
 
-        for (var k = 0; k < elements.length; k++)
+        for (let element of elements)
         {
           // Only consider selectors that actually have an effect on the
           // computed styles, and aren't overridden by rules with higher
           // priority, or haven't been circumvented in a different way.
-          if (getComputedStyle(elements[k]).display == "none")
+          if (getComputedStyle(element).display == "none")
           {
-            matchedSelectors.push(selector);
-            matched = true;
-            break;
+            let filter = filters[i] || selectors[i];
+            matchedFilters.push(filter.replace(/^.*?##/, ""));
+            break nodes;
           }
         }
-
-        if (matched)
-          break;
       }
     }
 
-    if (matchedSelectors.length > 0)
+    if (matchedFilters.length > 0)
       ext.backgroundPage.sendMessage({
         type: "devtools.traceElemHide",
-        selectors: matchedSelectors
+        selectors: matchedFilters
       });
   },
 
   onTimeout: function()
   {
-    this.checkNodes(this.changedNodes);
+    this.checkNodes(this.changedNodes, this.selectors, this.filters);
     this.changedNodes = [];
     this.timeout = null;
   },
@@ -312,7 +318,7 @@ ElementHidingTracer.prototype = {
 
   trace: function()
   {
-    this.checkNodes([document]);
+    this.checkNodes([document], this.selectors, this.filters);
 
     this.observer.observe(
       document,
@@ -490,9 +496,9 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
-    if (selectors.length == 0)
+    if (!selectors.length)

[Sebastian Noack, 2017/02/17 13:55:31]

I find the purpose of this check more clear when comparing the length explicitly
against zero (as this code initially did), rather than checking whether the
length evaluates to false. It's also more consistent with our existing code.

[wspee, 2017/02/18 11:20:23]

Done.

       return;
 
     if (!this.style)
@@ -525,7 +531,6 @@ ElemHide.prototype = {
         for (var j = 0; j < subSelectors.length; j++)
           preparedSelectors.push("::content " + subSelectors[j]);
       }
-      selectors = preparedSelectors;
     }
 
     // Safari only allows 8192 primitive selectors to be injected at once[1], we
@@ -533,49 +538,37 @@ ElemHide.prototype = {
     // (Chrome also has a limit, larger... but we're not certain exactly what it
     //  is! Edge apparently has no such limit.)
     // [1] - https://github.com/WebKit/webkit/blob/1cb2227f6b2a1035f7bdc46e5ab69debb75fc1de/Source/WebCore/css/RuleSet.h#L68
-    for (var i = 0; i < selectors.length; i += this.selectorGroupSize)
+    for (var i = 0; i < preparedSelectors.length; i += this.selectorGroupSize)

[Sebastian Noack, 2017/02/17 13:55:31]

Wouldn't preparedSelectors be undefined, on browsers where ShadowDOM does not
exist, and therefore this.shadow is null, and therefore the conditional block in
which preparedSelectors is defined won't be executed?

[wspee, 2017/02/18 11:20:23]

Done.

     {
-      var selector = selectors.slice(i, i + this.selectorGroupSize).join(", ");
+      var selector = preparedSelectors.slice(i, i + this.selectorGroupSize).join(", ");
       this.style.sheet.insertRule(selector + "{display: none !important;}",
                                   this.style.sheet.cssRules.length);
     }
+
+    if (this.tracer)
+      this.tracer.addSelectors(selectors, filters);
   },
 
   apply: function()
   {
-    var selectors = null;
-    var elemHideEmulationLoaded = false;
+    if (this.tracer)

[Sebastian Noack, 2017/02/17 13:55:31]

We should still wait until we have a response for "get-selectors" at least,
before removing the stylesheet and tracer, in order to minimize the side effects
when element hiding is reapplied. So please move this logic inside the callback.

[wspee, 2017/02/18 11:20:23]

Done.

+      this.tracer.disconnect();
+    this.tracer = null;
 
-    var checkLoaded = function()
-    {
-      if (!selectors || !elemHideEmulationLoaded)
-        return;
-
-      if (this.tracer)
-        this.tracer.disconnect();
-      this.tracer = null;
-
-      if (this.style && this.style.parentElement)
-        this.style.parentElement.removeChild(this.style);
-      this.style = null;
-
-      this.addSelectors(selectors.selectors);
-      this.elemHideEmulation.apply();
+    if (this.style && this.style.parentElement)
+      this.style.parentElement.removeChild(this.style);
+    this.style = null;
 
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, selectors =>
+    {
       if (selectors.trace)
-        this.tracer = new ElementHidingTracer(selectors.selectors);
-    }.bind(this);
+        this.tracer = new ElementHidingTracer();
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
-    {
-      selectors = response;
-      checkLoaded();
-    });
+      if (selectors.selectors)
+        this.addSelectors(selectors.selectors);
 
-    this.elemHideEmulation.load(function()
-    {
-      elemHideEmulationLoaded = true;
-      checkLoaded();
+      this.elemHideEmulation.load(

[Sebastian Noack, 2017/02/17 13:55:31]

The of the load() and apply() method did only exist in the first place, for the
synchronization which we just got rid off. So while changing
elemHideEmulation.js in adblockpluscore anyway, mind merging those two
functions, to simplify the logic here (as well as in Adblock Plus for Firefox,
where we do the same).

[wspee, 2017/02/18 11:20:24]

Done, see:

https://codereview.adblockplus.org/29375888/
https://codereview.adblockplus.org/29375891/

+        this.elemHideEmulation.apply.bind(this.elemHideEmulation));
     });
   }
 };