[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..655556b692cbafd059879e472f6afcdccdee1a2b 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,35 +209,40 @@ function ElementHidingTracer(selectors)
     this.trace();
 }
 ElementHidingTracer.prototype = {
-  checkNodes: function(nodes)
+  addSelectors: function(selectors, filters)
   {
-    var matchedSelectors = [];
+    if (!filters)
+      filters = new Array(selectors.length);

[Sebastian Noack, 2017/02/20 12:19:55]

In the end, it turns out the logic would be somewhat simpler when we just pass
the selectors as filters, instead of creating a new array here, so that we also
only need to consider the filters array when extracting the raw selector, like
you originally did.

I was initially against that approach because that practice exposes a slightly
leaky abstraction. (You would assume that the filters array stores filters,
while most of the time it will be just the selector part, potentially raising
wrong assumptions.) But seeing the result of this change, I notice that we
merely moved the ambiguous logic from one location to another, while adding
additional complexity. Hence I'm preferring your original approach now.

I'm very sorry for changing my mind. Something that seems fishy on first glance,
might turn out to be still the best compromise in the end, which is hard to tell
before seeing the changed code. So please never take mine (or anybody's)
feedback on a review for granted. Often, it's just a first impression.

[wspee, 2017/02/23 10:29:39]

NP, Done.

 
-    // 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 matchedSelectors = [];
 
-      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];
+            matchedSelectors.push(filter.replace(/^.*?##/, ""));
+            break nodes;
           }
         }
-
-        if (matched)
-          break;
       }
     }
 
@@ -249,7 +255,7 @@ ElementHidingTracer.prototype = {
 
   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,7 +496,7 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
     if (selectors.length == 0)
       return;
@@ -516,16 +522,19 @@ ElemHide.prototype = {
     // If using shadow DOM, we have to add the ::content pseudo-element
     // before each selector, in order to match elements within the
     // insertion point.
+    let preparedSelectors = [];
     if (this.shadow)
     {
-      var preparedSelectors = [];
       for (var i = 0; i < selectors.length; i++)
       {
         var subSelectors = splitSelector(selectors[i]);
         for (var j = 0; j < subSelectors.length; j++)
           preparedSelectors.push("::content " + subSelectors[j]);
       }
-      selectors = preparedSelectors;
+    }
+    else
+    {
+      preparedSelectors = selectors;
     }
 
     // Safari only allows 8192 primitive selectors to be injected at once[1], we
@@ -533,24 +542,21 @@ 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)
     {
-      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;
-
-    var checkLoaded = function()
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, response =>
     {
-      if (!selectors || !elemHideEmulationLoaded)
-        return;
-
       if (this.tracer)
         this.tracer.disconnect();
       this.tracer = null;
@@ -559,23 +565,12 @@ ElemHide.prototype = {
         this.style.parentElement.removeChild(this.style);
       this.style = null;
 
-      this.addSelectors(selectors.selectors);
-      this.elemHideEmulation.apply();
-
-      if (selectors.trace)
-        this.tracer = new ElementHidingTracer(selectors.selectors);
-    }.bind(this);
+      if (response.trace)
+        this.tracer = new ElementHidingTracer();
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
-    {
-      selectors = response;
-      checkLoaded();
-    });
+      this.addSelectors(response.selectors);
 

[Sebastian Noack, 2017/02/18 13:11:01]

Nit: I'd remove the blank line here (as it was before, see above).

-    this.elemHideEmulation.load(function()
-    {
-      elemHideEmulationLoaded = true;
-      checkLoaded();
+      this.elemHideEmulation.apply();
     });
   }
 };