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

lib/cssInjection.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module cssInjection */
 
 "use strict";
 
 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 = "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;
+const userStyleSheetsSupported = "extensionTypes" in chrome &&
+                                 "CSSOrigin" in chrome.extensionTypes;
 
 function hideElements(tabId, frameId, selectors)
 {
-  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"
-    }
-  );
+  chrome.tabs.insertCSS(tabId, {
+    code: selectors.join(", ") + "{display: none !important;}",
+    cssOrigin: "user",
+    frameId,
+    matchAboutBlank: true,
+    runAt: "document_start"
+  });
 }
 
 port.on("elemhide.getSelectors", (msg, sender) =>
 {
-  let selectors;
+  let selectors = [];
   let trace = devtools && devtools.hasPanel(sender.page);
+  let inject = !userStyleSheetsSupported;
 
   if (!checkWhitelisted(sender.page, sender.frame,
                         RegExpFilter.typeMap.DOCUMENT |
                         RegExpFilter.typeMap.ELEMHIDE))
   {
     let specificOnly = checkWhitelisted(sender.page, sender.frame,
                                         RegExpFilter.typeMap.GENERICHIDE);
     selectors = ElemHide.getSelectorsForDomain(
       extractHostFromFrame(sender.frame),
       specificOnly ? ElemHide.SPECIFIC_ONLY : ElemHide.ALL_MATCHING
     );
   }
-  else
-  {
-    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 (!inject && selectors.length > 0)
+    hideElements(sender.page.id, sender.frame.id, selectors);
 
-    if (trace)
-      return {selectors, trace: true, inject: false};
+  let response = {trace, inject};
+  if (trace || inject)
+    response.selectors = selectors;
 
-    return {trace: false, inject: false};
-  }
-
-  return {selectors, trace, inject: true};
+  return response;
 });
 
 port.on("elemhide.injectSelectors", (msg, sender) =>
 {
-  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);
+  hideElements(sender.page.id, sender.frame.id, msg.selectors);
 });