[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..45ccb31fc17a23ca3395ee798033f65e2df77f4d 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -192,64 +192,76 @@ 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()

[Sebastian Noack, 2017/02/14 10:58:16]

Do we even need the start method (and started flag)? What if we just put
following back into the constructor:

  if (document.readyState == "loading")
    document.addEventListener("DOMContentLoaded", this.trace);

But without the else branch. And then in addFilters(), instead of checking for
started (which we'd no longer need then), we check for document.readyState:

  if (document.readyState != "loading")
    this.checkNodes([document], selectors, filters);

Note that I also removed the timeout, which I think we can get rid of, if we
just make sure to call addFilters() after adding the rules to the stylesheet.

[Sebastian Noack, 2017/02/14 11:27:58]

I just noticed that currently we have to call trace(), to setup the
MutationObserver. But even if we separate that logic, it still seems simpler
than the current approach.

[wspee, 2017/02/15 14:09:57]

If we implement your suggestions wouldn't that mean that the tracer wouldn't
trace anything if the dom is still loading if trace is called?
We have to make sure that trace is called not only to set up the observer but
also to call checkNodes for the whole document?

[Sebastian Noack, 2017/02/15 18:49:03]

We should defer tracing (i.e. traversing through the whole document and start
observing further mutations) until when the DOM has been loaded. That is what
the document.readyState check and the DOMContentLoaded listener has been here
for in the first place.

This is necessary because the MutationObserver doesn't reliably observe nodes
added while the DOM is still being parsed. This depends a little bit on the
browser (and its version) though. But even if the MutationObserver is fired for
DOM fragments as they are parsed, it's still way faster to just wait until the
DOM is ready and then check all nodes at once.

With the suggested approach, we would still make sure everything is eventually
traced, as addFilters() will keep adding selectors/filters to
this.selectors/this.filters, which are then processed on DOMContentLoaded. Or if
the DOM has already been loaded, we process these selectors/filters immediately.

[wspee, 2017/02/17 10:51:38]

Done.

   {
-    var matchedSelectors = [];
+    if (document.readyState == "loading")
+      document.addEventListener("DOMContentLoaded", this.trace);
+    else
+      this.trace();
+  },
 
-    // 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];
+ addFilters: function(selectors, filters)

[Sebastian Noack, 2017/02/14 10:58:16]

I think this method should rather be called addSelectors() now, where it always
takes selectors, and the corresponding filter are optional.

[wspee, 2017/02/15 14:09:57]

Done.

+  {
+    if (this.started)
+      window.setTimeout(() => {
+        this.checkNodes([document], selectors, filters);
+      }, 0);
+
+    if (!filters)

[Sebastian Noack, 2017/02/14 10:58:16]

I'd move this to the top of the function. If we get rid of the timeout, we even
have to. But regardless, I think it makes the code flow clearer.

[wspee, 2017/02/15 14:09:56]

Done.

+      filters = new Array(selectors.length);
+
+    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);

[Sebastian Noack, 2017/02/14 11:27:58]

Renaming this variable to matchedFilters seems inappropriate, since we still
extract the selector part of the filter, and only send that part.

[wspee, 2017/02/15 14:09:57]

In case of an ElementHidingEmulationFilter we want to send the filter not the
selector. The selector array contains the constructed selector used to identify
the element to hide and not the selector part of the filter. If we sent only the
selector the panel is unable to identify which filter is responsible for a
hidden element.

So we send the filter and fallback to the selector if the filter is not
available and therefore the name should be matchedFilters and not
matchedSelectors, right?

[Sebastian Noack, 2017/02/15 18:49:03]

We never send the actual filter, as we remove the part before including the ##
if any. In fact, the string we are sending is the value of the ElemHideFilter or
ElemHideEmulationFilter object's "selector" property. In case of element hiding
emulation this value is still different from the resulting selector that is
injected in the end. But calling it a filter isn't accurate either. If we'd need
distinct terms for these things, I'd call them "raw/emulated selectors" and
"generated selectors". But that doesn't matter much for the code here.

[wspee, 2017/02/17 10:51:38]

No, that's the change I made, for ElemHideEmulationFilter we send the actual
filter not the selector and neither the generated selector.

http://i.imgur.com/9pj1UOJ.png

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

And what do you do in the line below? ;)

  matchedFilters.push(filter.replace(/^.*?##/, ""));

You only store the (raw) selector part, and eventually send it.

[Sebastian Noack, 2017/02/18 11:41:18]

This comment hasn't been resolved yet.

[wspee, 2017/02/18 13:00:14]

Done, you were right after all ;).

-            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 +324,8 @@ ElementHidingTracer.prototype = {
 
   trace: function()
   {
-    this.checkNodes([document]);
+    this.started = true;
+    this.checkNodes([document], this.selectors, this.filters);
 
     this.observer.observe(
       document,
@@ -490,11 +503,14 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
-    if (selectors.length == 0)
+    if (!selectors.length)
       return;
 
+    if (this.tracer)
+      this.tracer.addFilters(selectors, filters);
+
     if (!this.style)
     {
       // Create <style> element lazily, only if we add styles. Add it to
@@ -544,38 +560,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);

[Sebastian Noack, 2017/02/14 10:58:16]

Why did you move this inside this callback? That way "get-selectors" and
elemHideEmulation.load() are no longer requested simultaneously, adding a delay.

[wspee, 2017/02/15 14:09:56]

Hmm ... because this way checkLoaded is only called once and is stateless ...
but I didn't take into account that sendMessage could be asynchronous.

But I'm not sure if there is a way to take advantage of the async execution.
Whether or not to trace elements depends on the response of get-selectors
(response.trace) and there is not much you could do without knowing whether or
not to activate the tracer?

[Sebastian Noack, 2017/02/15 18:49:03]

Well, the whole point of having checkLoaded(), as opposed to passing an
anonymous function here, or naming it "onLoaded" at least, is to synchronize
sendMessage({type: "get-selectors"}, ...) and elemHideEmulation.load(...).

First of all, the synchronization is necessary, as you already noticed, 
so that we know whether to trace element hiding, in order to keep track of the
selectors/filters if necessary, before injecting any selectors. But there is
also another benefit of the synchronization here. That is if element hiding gets
reapplied (e.g. when using the "Block element" dialog from the icon menu),
replacing the stylesheet happens in one go before the page is redrawn, so that
you don't see any elements showing up shortly before they are hidden again.

The same could be achieved by sending the messages sequentially, like you did.
But this will cause a (small) delay, since the background page, after responding
to the first message, has to wait for us to send the second message. But if we
send the second message already while the first message hasn't been processed
yet, like this code originally did, there is no delay in between both messages
being processed by the background page.

While I don't consider your approach an improvement, I might see some potential
for improvement here. First of all, the synchronization adds some complexity,
that would be great to get rid of, as you noticed. Moreover, while it makes sure
that emulation filters are ready as soon as possible, it adds some delay for
regular element hiding filter (which is the common case), with either approach
(but even more with yours). So how about this: We setup the stylesheet and
tracer, and start injecting selectors, as soon as we got a response for
"get-selectors". And then we asynchronously retrieve and apply the emulation
filters. This will get rid of the synchronization and minimizes the delay for
regular element hiding filters as much as possible. The only downside then is
that we have a visible delay in between regular element hiding filters, and
emulation filters, being applied. However, in the common case this is
preferable, if that means regular element hiding filters become applied earlier.
And for the case where the "Block element" dialog causes element hiding to be
reapplied, this would be still acceptable, considering the performance
improvement in the common case and that this only effect emulation filters.

[wspee, 2017/02/17 10:51:38]

Like so?

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

Almost. ;)

     });
   }
 };