[on hold] Issue 6455 - Use tabs.removeCSS on Chromium

lib/cssInjection.js

Index: lib/cssInjection.js
===================================================================
--- a/lib/cssInjection.js
+++ b/lib/cssInjection.js
@@ -23,21 +23,17 @@
 const {ElemHide} = require("elemHide");
 const {ElemHideEmulation} = require("elemHideEmulation");
 const {checkWhitelisted} = require("whitelisting");
 const {extractHostFromFrame} = require("url");
 const {port} = require("messaging");
 const devtools = require("devtools");
 const info = require("info");
 
-// Chromium's support for tabs.removeCSS is still a work in progress and the
-// API is likely to be different from Firefox's; for now we just don't use it
-// at all, even if it's available.
-// See https://crbug.com/608854
-const styleSheetRemovalSupported = info.platform == "gecko";
+const styleSheetRemovalSupported = "removeCSS" in browser.tabs;
 
 const selectorGroupSize = 1024;
 
 let userStyleSheetsSupported = true;
 
 function* splitSelectors(selectors)
 {
   // Chromium's Blink engine supports only up to 8,192 simple selectors, and
@@ -112,31 +108,74 @@
   if (!frame)
     return false;
 
   if (!frame.injectedStyleSheets)
     frame.injectedStyleSheets = new Map();
 
   let oldStyleSheet = frame.injectedStyleSheets.get(groupName);
 
+  // The behavior of tabs.insertCSS and tabs.removeCSS is different between

[kzar, 2018/03/19 18:42:49]

Thanks for taking the time to write such a good comment.

+  // Firefox and Chromium. On Firefox, it is an error to inject a duplicate
+  // style sheet, whereas Chromium will happily inject the duplicate as a copy.
+  // tabs.removeCSS on Chromium will remove only the last injected copy.
+  //
   // Ideally we would compare the old and new style sheets and skip this code
   // if they're the same, but the old style sheet can be a leftover from a
   // previous instance of the frame. We must add the new style sheet
   // regardless.
-
-  // Add the new style sheet first to keep previously hidden elements from
-  // reappearing momentarily.
-  if (styleSheet && !addStyleSheet(tabId, frameId, styleSheet))
-    return false;
+  //
+  // We could simply remove the old style sheet first before we add the new
+  // one, but this may cause previously hidden elements to reappear
+  // momentarily. On the other hand, we cannot remove the old style sheet after
+  // we add the new one if they're exactly the same, because this would cause
+  // Firefox to remove its only copy; if we skip the removal, Chromium would
+  // end up with a duplicate copy!
+  //
+  // So: (1) we do not want to remove first to avoid flicker; (2) if the old

[kzar, 2018/03/19 18:42:49]

I think this stanza can be removed.

+  // and new style sheets are identical, we do not want to remove on Firefox,
+  // but we do want to remove on Chromium.
+  //
+  // In order to resolve this conundrum without resorting to browser-specific
+  // code, we treat the different types of style sheets differently.
+  if (groupName == "standard")

[kzar, 2018/03/19 18:42:49]

Perhaps the logic would be clearer something like this?

  let removeOld = styleSheet == oldStyleSheet;

  if (removeOld && groupName == "standard")
  {
    removeStyleSheet(tabId, frameId, oldStyleSheet);
    frame.injectedStyleSheets.delete(groupName);
    removeOld = false;
  }

  if (styleSheet && !addStyleSheet(tabId, frameId, styleSheet))
    return false;

  if (removeOld)
    removeStyleSheet(tabId, frameId, oldStyleSheet);

+  {
+    // Style sheets in the "standard" group are typically only added once and

[kzar, 2018/03/19 18:42:49]

I think instead of saying 'the "standard" group' and 'other style sheets' you
should explain what they are. I can see you're checking for the standard group
in the code, but it's not immediately obvious what a standard or not-standard
filter is.

+    // never removed. In this case, it's OK to remove the old style sheet first
+    // and accept the flicker.
+    if (oldStyleSheet)
+    {
+      removeStyleSheet(tabId, frameId, oldStyleSheet);
 
-  // Sometimes the old and new style sheets can be exactly the same. In such a
-  // case, do not remove the "old" style sheet, because it is in fact the new
-  // style sheet now.
-  if (oldStyleSheet && oldStyleSheet != styleSheet)
-    removeStyleSheet(tabId, frameId, oldStyleSheet);
+      // Delete the entry first, just in case style sheet injection below
+      // fails.
+      frame.injectedStyleSheets.delete(groupName);
+    }
+
+    if (styleSheet && !addStyleSheet(tabId, frameId, styleSheet))
+      return false;
+  }
+  else if (styleSheet != oldStyleSheet)
+  {
+    // Note that it is possible that the old and new style sheets are the same
+    // but the old one is really a leftover from a previous instance of the
+    // frame (i.e. it doesn't exist in the current instance of the frame). In
+    // this case we'll skip the addition of the new style sheet and ads will
+    // not get blocked. This would not be acceptable for style sheets in the
+    // "standard" group, but it's OK for other style sheets. Given the problems
+    // described above, this is the best tradeoff.
+
+    // Add the new style sheet first to keep previously hidden elements from
+    // reappearing momentarily.
+    if (styleSheet && !addStyleSheet(tabId, frameId, styleSheet))
+      return false;
+
+    if (oldStyleSheet)
+      removeStyleSheet(tabId, frameId, oldStyleSheet);
+  }
 
   frame.injectedStyleSheets.set(groupName, styleSheet);
   return true;
 }
 
 port.on("elemhide.getSelectors", (message, sender) =>
 {
   let selectors = [];