[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..8eb89e9c6ede8860b5ae67dd8f8b94bf3dbda582 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -192,64 +192,80 @@ function getContentDocument(element)
   }
 }
 
-function ElementHidingTracer(selectors)
+function ElementHidingTracer()
 {
-  this.selectors = selectors;
+  this.selectors = [];
+  this.filters = [];
 
   this.changedNodes = [];
   this.timeout = null;
+  this.started = false;
 
   this.observer = new MutationObserver(this.observe.bind(this));
   this.trace = this.trace.bind(this);
-
-  if (document.readyState == "loading")
-    document.addEventListener("DOMContentLoaded", this.trace);
-  else
-    this.trace();
 }
 ElementHidingTracer.prototype = {
-  checkNodes: function(nodes)
+  start: function()
+  {
+    let _start = () => {
+      this.trace();

[Sebastian Noack, 2017/02/13 15:09:17]

Please correct me if I'm wrong, but it seems this is the only place where
trace() is called. So if you just set the started property inside the trace()
function, we wouldn't need this wrapper.

[wspee, 2017/02/14 09:28:40]

Done.

+      this.started = true;
+    };
+
+    if (document.readyState == "loading")
+      document.addEventListener("DOMContentLoaded", _start);
+    else
+      _start();
+  },
+
+ addFilters: function(selectors, filters)
   {
-    var matchedSelectors = [];
+    if (this.started)
+      window.setTimeout(() => {
+        this.checkNodes([document], selectors, filters);
+      }, 0);
 
-    // Find all selectors that match any hidden element inside the given nodes.
-    for (var i = 0; i < this.selectors.length; i++)
+    Array.prototype.push.apply(this.selectors, selectors);

[Sebastian Noack, 2017/02/13 15:09:17]

As mentioned before, please use the rest operator instead of .apply():

  this.selectors.push(...selectors);
  this.filters.push(...filters);

[wspee, 2017/02/14 09:28:40]

Done.

+    Array.prototype.push.apply(this.filters, filters);
+  },
+
+  checkNodes: function(nodes, selectors, filters)
+  {
+    let matchedFilters = [];
+
+    for (let i = 0; i < selectors.length; i++)
     {
-      var selector = this.selectors[i];
+      let selector = selectors[i];

[Sebastian Noack, 2017/02/13 15:09:17]

It seems "selector" and "filter" are only used once each. So these temporary
variables seem unnecessary. Instead you could inline selectors[i]/filters[i].

[wspee, 2017/02/14 09:28:40]

Done.

+      let filter = filters[i];
 
-      for (var j = 0; j < nodes.length; j++)
+      nodes: for (let node of nodes)
       {
-        var elements = nodes[j].querySelectorAll(selector);
-        var matched = false;
+        let elements = node.querySelectorAll(selector);
 
-        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;
+            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 +328,7 @@ ElementHidingTracer.prototype = {
 
   trace: function()
   {
-    this.checkNodes([document]);
+    this.checkNodes([document], this.selectors, this.filters);
 
     this.observer.observe(
       document,
@@ -490,11 +506,21 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
-    if (selectors.length == 0)
+    if (!selectors.length)
       return;
 
+    if (this.tracer)
+    {
+      if (!filters)
+      {
+        filters = [];
+        Array.prototype.push.apply(filters, selectors);

[Sebastian Noack, 2017/02/13 15:09:17]

As indicated before, I'm not sure if it is a good idea to redundantly store the
selectors as filters, when we don't know the actual filters. This is a leaky
abstraction. While it works for the code above, it raises wrong assumptions. Not
to mention the redundancy in the data structure.

I'd rather create an array which holds the value of undefined for each selector
we don't know the filter of (i.e. `new Array(selectors.length)`), and handle
that case explicitly, when extracting the original selectors.

Moreover, I feel that this logic belongs into the addFilter() method.

[wspee, 2017/02/14 09:28:40]

Done.

+      }
+      this.tracer.addFilters(selectors, filters);
+    }
+
     if (!this.style)
     {
       // Create <style> element lazily, only if we add styles. Add it to
@@ -544,38 +570,33 @@ ElemHide.prototype = {
   apply: function()
   {
     var selectors = null;
-    var elemHideEmulationLoaded = false;
 
     var checkLoaded = function()
     {
-      if (!selectors || !elemHideEmulationLoaded)
-        return;
-
       if (this.tracer)
         this.tracer.disconnect();
       this.tracer = null;
 
+      if (selectors.trace)
+        this.tracer = new ElementHidingTracer();
+
       if (this.style && this.style.parentElement)
         this.style.parentElement.removeChild(this.style);
       this.style = null;
 
-      this.addSelectors(selectors.selectors);
+      if (selectors.selectors)
+        this.addSelectors(selectors.selectors);
+
       this.elemHideEmulation.apply();
 
-      if (selectors.trace)
-        this.tracer = new ElementHidingTracer(selectors.selectors);
+      if (this.tracer)
+        this.tracer.start();
     }.bind(this);
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, response =>
     {
       selectors = response;
-      checkLoaded();
-    });
-
-    this.elemHideEmulation.load(function()
-    {
-      elemHideEmulationLoaded = true;
-      checkLoaded();
+      this.elemHideEmulation.load(checkLoaded);
     });
   }
 };