Issue 5090 - Use user stylesheets for element hiding if possible

include.preload.js

Index: include.preload.js
diff --git a/include.preload.js b/include.preload.js
index 92a6212f9929bddc8654ffdb7a80b136cc5b1f41..db67b224adf1cb783cfa0db63a7cdb1ac196d866 100644
--- a/include.preload.js
+++ b/include.preload.js
@@ -534,6 +534,7 @@ function ElemHide()
   this.shadow = this.createShadowTree();
   this.style = null;
   this.tracer = null;
+  this.inject = true;
 
   this.elemHideEmulation = new ElemHideEmulation(
     window,
@@ -596,11 +597,33 @@ ElemHide.prototype = {
     return shadow;
   },
 
-  addSelectors(selectors, filters)
+  addSelectors(selectors, filters, {inject, traceOnly} = {})

[Sebastian Noack, 2017/05/24 08:31:41]

Any reason you wrapped these two new options into an object?

[Manish Jethani, 2017/05/24 17:13:34]

The calling code is usually easier to read when option arguments are in an
options object.

e.g.

  addSelectors(selectors, filters, {trace: !someValue});

If these were positional arguments, assuming inject came first, the call would
look like this:

  addSelectors(selectors, filters, null, !someValue);

Now there's an additional null in between, you don't know what the options are
exactly, and you can never get rid of the options without breaking a lot of code
(if it's an options object then the function can simply ignore the option even
if old calling code is passing it in).

I use this pattern a lot (especially with ES6 destructuring now it performs well
too), I think it's great for public APIs, but in this particular case I don't
have a strong opinion either way. If you think it doesn't agree with the coding
practices then I'll change it.

[Sebastian Noack, 2017/05/24 18:14:05]

Yeah, named arguments would be useful here. Not sure whether the workaround
using an object is justified in the case here though. As far as I've seen, this
is usually only done when there is a large list of options of which many are
optional, or in public APIs that are expected to add new options in the future.

[Manish Jethani, 2017/05/25 02:52:25]

I've changed it now.

   {
-    if (selectors.length == 0)
+    if (!selectors || selectors.length == 0)
+      return;
+
+    if (traceOnly)
+    {
+      if (this.tracer)

[Sebastian Noack, 2017/05/24 08:31:41]

Couldn't we just do..

if (trace || inject)
  this.tracer.addSelectors(selectors, filters);

below, rather than duplicating this logic?

[Manish Jethani, 2017/05/24 17:13:34]

The code that follows is not supposed to run if traceOnly is true. Either we
have to return here, or wrap the rest of the code in "if (!traceOnly) { ... }".
I think returning is probably better, even if it means two lines are duplicated.

[Sebastian Noack, 2017/05/24 18:14:05]

I see, I guess it makes sense then. On the other hand, any reason you introduced
the traceOnly state in the first place, rather than just having "trace" and
"inject" as independent states? This would avoid redundant/invalid states,
and is more consistent with the abstraction in the calling code, potentially
simplifying the logic? Not sure though.

[Manish Jethani, 2017/05/25 02:52:25]

The problem is that inject can't be simply true or false, we need a third state
where we don't insert the CSS at all. This is independent of whether we trace or
not. traceOnly is basically "Don't insert CSS, only trace".

Now I've changed it so that it takes three values: "inline" (inject), "user"
(use "hide-elements"), and "none" (trace only).

By the way, we got into this situation because we tried to avoid passing an
addSelectors wrapper to the ElemHideEmulation constructor. In hindsight, that
was way more straightforward.

[Sebastian Noack, 2017/05/30 15:50:30]

Sorry, I still don't get it. If we just use two flags, "inject" and "trace" just
like the details sent by the background page, we have following possible states:

* inject=true,  trace=true  Inject an author stylesheet through the
                            content script and setup tracing. This will be
                            the case on Chrome with active devtools panel.
* inject=true,  trace=false Just inject an author stylesheet through the
                            content script. This is the case on Chrome and older
                            Firefox versions, with no active devtools panel.
* inject=false, trace=true  User stylesheet is injected through the
                            background page and tracing is done by the
                            content script. This will be the case in the future,
                            when either Firefox adds support for the devtools
                            panel or Chrome adds support for user stylesheets.
* inject=false, trace=false User stylesheet is injected through the
                            background page. This is the case on newer
                            Firefox versions.

So why do you translate these states into a different abstraction here?

[Manish Jethani, 2017/05/31 06:21:38]

As I said earlier, inject cannot be just true or false, it needs to take three
values, because there are three different scenarios we're trying to handle: (1)
inject the styles into the page, (2) insert a user stylesheet, and (3) do
nothing.

Let me explain when each of these happens:

 1.  When "get-selectors" gives us inject=true (Chrome) and when
ElemHideEmulation calls us again
 2.  When ElemHideEmulation calls us and this.inject is false (Firefox)
 3.  When "get-selectors" gives us inject=false (Firefox)

[Sebastian Noack, 2017/05/31 11:13:29]

Couldn't we detect #3 by selectors being an empty array?

[Manish Jethani, 2017/05/31 14:41:14]

selectors isn't an empty array when trace is true. This is the reason why we
call addSelectors function in the first place, even though inject is false.

We could move the tracing code outside of the addSelectors function, but then
we'd be duplicating. Maybe that's OK.

+        this.tracer.addSelectors(selectors, filters);
+
       return;
+    }
 
+    inject = this.inject && inject == null || !!inject;
+
+    // Insert the style rules inline if we have been instructed by the
+    // background page to do so. This is usually the case, except on platforms
+    // that do support user stylesheets via the chrome.tabs.insertCSS API
+    // (Firefox 53 onwards for now and possibly Chrome in the near future).
+    // Once all supported platforms have implemented this API, we can remove
+    // the code below.
+    // See issue #5090.
+    // Related Chrome and Firefox issues:
+    // https://bugs.chromium.org/p/chromium/issues/detail?id=632009
+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1310026
+    if (inject)
+    {
       if (!this.style)
       {
         // Create <style> element lazily, only if we add styles. Add it to
@@ -637,10 +660,11 @@ ElemHide.prototype = {
         preparedSelectors = selectors;
       }
 
-    // Safari only allows 8192 primitive selectors to be injected at once[1], we
-    // therefore chunk the inserted selectors into groups of 200 to be safe.
-    // (Chrome also has a limit, larger... but we're not certain exactly what it
-    //  is! Edge apparently has no such limit.)
+      // Safari only allows 8192 primitive selectors to be injected at once[1],
+      // we therefore chunk the inserted selectors into groups of 200 to be
+      // safe.
+      // (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 (let i = 0; i < preparedSelectors.length; i += this.selectorGroupSize)
       {
@@ -653,6 +677,22 @@ ElemHide.prototype = {
 
       if (this.tracer)
         this.tracer.addSelectors(selectors, filters);
+    }
+    else
+    {
+      ext.backgroundPage.sendMessage({type: "hide-elements", selectors},
+      response =>
+      {
+        let options = {};
+
+        if (!response.success)
+          options.inject = true;
+        else
+          options.traceOnly = true;
+
+        this.addSelectors(selectors, filters, options);
+      });
+    }
   },
 
   apply()
@@ -670,7 +710,9 @@ ElemHide.prototype = {
       if (response.trace)
         this.tracer = new ElementHidingTracer();
 
-      this.addSelectors(response.selectors);
+      this.inject = response.inject;
+
+      this.addSelectors(response.selectors, null, {traceOnly: !this.inject});
       this.elemHideEmulation.apply();
     });
   }