Issue 5695 - Use tabs.insertCSS if extensionTypes.CSSOrigin exists

lib/cssInjection.js

Index: lib/cssInjection.js
===================================================================
--- a/lib/cssInjection.js
+++ b/lib/cssInjection.js
@@ -21,43 +21,30 @@
 
 const {RegExpFilter} = require("filterClasses");
 const {ElemHide} = require("elemHide");
 const {checkWhitelisted} = require("whitelisting");
 const {extractHostFromFrame} = require("url");
 const {port} = require("messaging");
 const devtools = require("devtools");
 
-let userStylesheetsSupported = true;
+let userStyleSheetsSupported = "extensionTypes" in chrome &&

[Manish Jethani, 2017/09/15 13:02:42]

Note: "style sheet" is actually two words, I've changed the name of this
variable accordingly.

[Sebastian Noack, 2017/09/15 18:46:37]

In which case, does chrome.extensionTypes does not exist? According to MDN, it
even exists on Miccrosoft Edge:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extensionTypes

Ollie, can you confirm?

[Manish Jethani, 2017/09/15 21:26:08]

It doesn't exist on Chrome.

[Sebastian Noack, 2017/09/15 22:52:44]

Indeed. The MDN compatibility information are misleading here.

+                               "CSSOrigin" in chrome.extensionTypes;
 
 function hideElements(tabId, frameId, selectors)
 {
-  let code = selectors.join(", ") + "{display: none !important;}";
-
-  try
-  {
-    chrome.tabs.insertCSS(tabId,
-      {
-        code,
-        cssOrigin: "user",
-        frameId,
-        matchAboutBlank: true,
-        runAt: "document_start"
-      }
-    );
-    return true;
-  }
-  catch (error)
-  {
-    if (/\bError processing cssOrigin\b/.test(error.message) == -1)
-      throw error;
-
-    userStylesheetsSupported = false;
-    return false;
-  }
+  chrome.tabs.insertCSS(tabId,

[Sebastian Noack, 2017/09/15 18:46:37]

Nit: The additional indentation (and one additional line) seems unnecessary, if
you wrap it like this (which also seems consistent with most existing code):

  chrome.tabs.insertCSS(tabId, {
    ...
  });

[Manish Jethani, 2017/09/15 21:26:09]

Done.

+    {
+      code: selectors.join(", ") + "{display: none !important;}",
+      cssOrigin: "user",
+      frameId,
+      matchAboutBlank: true,
+      runAt: "document_start"
+    }
+  );
 }
 
 port.on("elemhide.getSelectors", (msg, sender) =>
 {
   let selectors;
   let trace = devtools && devtools.hasPanel(sender.page);
 
   if (!checkWhitelisted(sender.page, sender.frame,
@@ -71,24 +58,27 @@
       specificOnly ? ElemHide.SPECIFIC_ONLY : ElemHide.ALL_MATCHING
     );
   }
   else
   {
     selectors = [];
   }
 
-  if (selectors.length == 0 || userStylesheetsSupported &&
-      hideElements(sender.page.id, sender.frame.id, selectors))
+  if (selectors.length == 0 || userStyleSheetsSupported)

[Sebastian Noack, 2017/09/15 18:46:37]

I had a go before. But you were quicker, getting your patch under review.
However, there are some more simplifications, here, in my patch:
https://gist.github.com/snoack/253a5302b933933e19c5cfb95725cc78

What do you think?

[Manish Jethani, 2017/09/15 21:26:08]

I've incorporated the simplifications and made some more. We don't need to
include a property if the value is false.

   {
+    if (userStyleSheetsSupported && selectors.length > 0)
+      hideElements(sender.page.id, sender.frame.id, selectors);

[Manish Jethani, 2017/09/15 13:02:42]

If userStyleSheetsSupported is true then a call to hideElements is always
expected to succeed (rather never for lack of cssOrigin support).

+
     if (trace)
       return {selectors, trace: true, inject: false};
 
     return {trace: false, inject: false};
   }
 
   return {selectors, trace, inject: true};
 });
 
 port.on("elemhide.injectSelectors", (msg, sender) =>
 {
-  return hideElements(sender.page.id, sender.frame.id, msg.selectors);
+  if (userStyleSheetsSupported)

[Manish Jethani, 2017/09/15 13:02:42]

Since there's no try...catch in hideElements now, we need to do a check here
explicitly before calling the function.

[Sebastian Noack, 2017/09/15 18:46:37]

It seems no message of this type is send by the content script if cssOrigin is
not supported, hence inject is true.

[Manish Jethani, 2017/09/15 21:26:09]

Done.

+    hideElements(sender.page.id, sender.frame.id, msg.selectors);
 });