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
@@ skipping 24 matching lines @@
     code: selectors.join(", ") + "{display: none !important;}",
     cssOrigin: "user",
     frameId,
     matchAboutBlank: true,
     runAt: "document_start"
   });
 }
 
 port.on("elemhide.getSelectors", (msg, sender) =>
 {
-  let selectors = null;
+  let selectors = [];
   let trace = devtools && devtools.hasPanel(sender.page);
   let inject = !userStyleSheetsSupported;

[Manish Jethani, 2017/09/17 13:51:38]

The trace and inject values are pretty much constant. We decide early if we want
to inject or not.

 
   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
     );
-
-    if (!inject && selectors.length > 0)

[Manish Jethani, 2017/09/17 13:51:38]

This has been moved inside the if block. If we're not going to inject and we do
have selectors, do the tabs.insertCSS thing here.

-      hideElements(sender.page.id, sender.frame.id, selectors);
   }
 
+  if (!inject && selectors.length > 0)
+    hideElements(sender.page.id, sender.frame.id, selectors);
+
   let response = {trace, inject};

[Manish Jethani, 2017/09/17 13:51:38]

trace and inject pretty much have to be passed as they are as the values cannot
be changed for any reason.

-
-  if ((trace || inject) && selectors && selectors.length > 0)

[Manish Jethani, 2017/09/17 13:51:38]

Include a selectors property only if either trace or inject are true and if
there are any selectors at all.

We could skip the last two checks and then just include the selectors property
even if it's null or an empty array, but I don't see why we should. It would
certainly simplify the code though.

For example, only one return statement:

  let selectors = [];
  ...
  ...
  ...
  return {selectors: trace || inject ? selectors : [], trace, inject};

i.e. always return an array, even if empty.

[Manish Jethani, 2017/09/17 15:09:19]

Or:

  let selectors = null;
  ...
  ...
  ...
  return {selectors: (trace || inject) && selectors || [], trace, inject};

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

How about this?

  let selectors = [];
  ...
  if (trace || inject)
    response.selectors = selectors;

Or like before:

  let selectors;
  if (...)
  {
    ...
    selectors = ...
  }
  else
    selectors = [];

Then you also wouldn't need the check for presence of response.selectors in the
content script, and you wouldn't have to nest the logic above for calling
hideElements().

[Manish Jethani, 2017/09/19 05:47:26]

OK, I like this.

Done.
We still have to check if the selectors array is empty before calling
hideElements, otherwise it'll bomb. Then it makes more sense to keep that check
inside the if block, right?

[Sebastian Noack, 2017/09/19 23:09:17]

Yes, you have to check for selectors.length regardless whether you nest in the
block above or not, but the reason you moved it inside the block, I suppose, was
to not also have to check for selectors being null, which it never is now. So it
doesn't need to be nested in that block anymore.

[Manish Jethani, 2017/09/20 01:36:29]

With the latest patch there's no check for selectors.length outside of the if
block.

[Sebastian Noack, 2017/09/20 01:51:52]

Sure, but why don't you just flatten the logic by moving

  if (!inject && selectors.length > 0)
    hideElements(sender.page.id, sender.frame.id, selectors);

outside of the

  if (!checkWhitelisted(...)
  {
    ...
  }

block, since `selectors = []` is now defined at the top of the function.

[Manish Jethani, 2017/09/20 02:23:59]

Done.

selectors starting out as null or an empty array is no longer relevant here
though, but I suppose it makes the code more readable.

+  if (trace || inject)
     response.selectors = selectors;
 
   return response;
 });
 
 port.on("elemhide.injectSelectors", (msg, sender) =>
 {
   hideElements(sender.page.id, sender.frame.id, msg.selectors);
 });