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

include.preload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 176 matching lines @@
     return element.contentDocument;
   }
   catch (e)
   {
     return null;
   }
 }
 
 function ElementHidingTracer(selectors)
 {
-  this.selectors = selectors;
+  this.selectors = selectors || [];

[Sebastian Noack, 2017/01/11 16:20:40]

It seems you removed the only code that initialized the ElementHidingTracer
without arguments. So unless I missed something, why not just removing support
for the "selectors" argument?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

 
   this.changedNodes = [];
   this.timeout = null;
 
   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)
   {
     var matchedSelectors = [];
 
     // 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];
 
       for (var j = 0; j < nodes.length; j++)
       {
-        var elements = nodes[j].querySelectorAll(selector);
+        var elements = nodes[j].querySelectorAll(selector.selector);
         var matched = false;
 
         for (var k = 0; k < elements.length; k++)
         {
           // 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")
           {
-            matchedSelectors.push(selector);
+            if (selector.filter)
+              matchedSelectors.push(selector.filter.replace(/^.*?##/, ""));
+            else
+              matchedSelectors.push(selector.selector);
             matched = true;
             break;
           }
         }
 
         if (matched)
           break;
       }
     }
 
@@ skipping 240 matching lines @@
             var shadow = shadowRoot(this);
             return shadow == ourShadowRoot ? null : shadow;
           }
         });
       }, null);
     }
 
     return shadow;
   },
 
-  addSelectors: function(selectors)
+  addSelectors: function(selectors, filters)
   {
     if (selectors.length == 0)
       return;
 
+    if (this.tracer != null)
+    {
+      // The selector is beeing used to identify the filter that is responsible
+      // for hiding a particular element in the devtools panel. The
+      // ElementHidingTracer uses the selector to figure out if a filter is
+      // used on the current document. In case of an ElemHideEmulationFilter
+      // the selector doesn't match the selector of the filter
+      // ([-abp-properties=...] vs actual css selector depending on the sites
+      // css). The To still allow the devtools panel to find the correct filter
+      // we pass both the selector and the filter string to the
+      // ElementHidingTracer.
+      for (var i = 0; i < selectors.length; i++)
+      {
+        this.tracer.selectors.push({

[Sebastian Noack, 2017/01/11 16:20:40]

Is it guaranteed that the order of selectors match the order of filters? I have
some doubts from looking at the code in elemHideEmulation.js:

  addSelectors: function(stylesheets)
  {
    var selectors = [];
    var filters = {};
    for (var i = 0; i < stylesheets.length; i++)
      this.findSelectors(stylesheets[i], selectors, filters);
    this.addSelectorsFunc(selectors, Object.keys(filters));
  },

Note that objects in JavaScript don't guarantee properties to be retrieved in
insertion order (like dicts in Python). However, we could simply use a Set in
elemHideEmulation.js which preserves the insertion order and seems more
appropriate anyway, but wasn't supported by all supported browsers we run that
code on back then.

[Sebastian Noack, 2017/01/11 16:20:40]

Creating a new object for each selector might potentially add a significant
memory footprint. A possibly better solution would be to use two arrays, one for
the selectors and one for the filters. Perhaps you could benchmark the memory
usage with the current approach and compare it against the one I'm suggesting.

[wspee, 2017/01/12 13:39:58]

Indeed ... if you look at findSelectors there can even be more selectors than
filters.

findSelectors: function(stylesheet, selectors, filters)
{
  ...
        var subSelectors = splitSelector(rule.selectorText);
        for (var k = 0; k < subSelectors.length; k++)
          selectors.push(pattern.prefix + subSelectors[k] + pattern.suffix);
        filters[pattern.text] = true;
  ...
},

So I have switched the whole approach to something else entirely ....

ElemHideEmulationFilter are only created if there is a style that matches the
filter and document changes are also handled by ElemHideEmulation so it wasn't
really necessary to use the ElementHidingTracer for ElemHideEmulationFilter.

Instead I let addSelectors fire devtools.traceElemHide directly similar to how
it works in firefox
(adblockplus/lib/child/elemHideEmulation.js:initElemHideEmulation).

Only drawback is that devtools.traceElemHide gets fired regardless of whether
the devtools are opened or not. I could add a flag to ElemHide but that whould
be somewhat redundant with ElemHide.tracer but I cannot test ElemHide.tracer as
it gets assigned after addSelectors is called by ElemHideEmulation. But
logHiddenElements does (should have?!) check if the panel is opened anyways so
it's only a slight penalty. Let me now if you think otherwise.

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

According to the chrome profiler an array with 10000 objects uses less memory
than two arrays with 10000 entries, so I kept that approach.

[wspee, 2017/01/19 16:20:14]

Done.

+          selector: selectors[i],
+          filter: filters ? filters[i] : null
+        });
+      }
+    }
+
     if (!this.style)
     {
       // Create <style> element lazily, only if we add styles. Add it to
       // the shadow DOM if possible. Otherwise fallback to the <head> or
       // <html> element. If we have injected a style element before that
       // has been removed (the sheet property is null), create a new one.
       this.style = document.createElement("style");
       (this.shadow || document.head
                    || document.documentElement).appendChild(this.style);
 
@@ skipping 29 matching lines @@
     {
       var selector = selectors.slice(i, i + this.selectorGroupSize).join(", ");
       this.style.sheet.insertRule(selector + "{display: none !important;}",
                                   this.style.sheet.cssRules.length);
     }
   },
 
   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.selectors.length)

[Sebastian Noack, 2017/01/11 16:20:40]

Any particular reason (except for a hypothetical micro-optimization) to check
for this.tracer.selectors.length?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:15]

Done.

+        this.tracer.checkNodes([document]);
     }.bind(this);
 
-    ext.backgroundPage.sendMessage({type: "get-selectors"}, function(response)
+    ext.backgroundPage.sendMessage({type: "get-selectors"}, (response) =>

[Sebastian Noack, 2017/01/11 16:20:40]

The parentheses around the "response" argument are redundant.

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

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

[Sebastian Noack, 2017/01/11 16:20:40]

The arrow function seems redundant. Why not just passing checkLoaded without
wrapping to the load() method?

[wspee, 2017/01/12 13:39:58]

No longer relevant.

[wspee, 2017/01/19 16:20:14]

Done.

     });
   }
 };
 
 if (document instanceof HTMLDocument)
 {
   checkSitekey();
   wrapWebSocket();
 
   var elemhide = new ElemHide();
   elemhide.apply();
 
   document.addEventListener("error", function(event)
   {
     checkCollapse(event.target);
   }, true);
 
   document.addEventListener("load", function(event)
   {
     var element = event.target;
     if (/^i?frame$/.test(element.localName))
       checkCollapse(element);
   }, true);
 }