Issue 5090 - Use user stylesheets for element hiding if possible

If the chrome.tabs.insertCSS API is available and supports "user" stylesheets (Firefox 53), we use it instead of injecting the CSS directly into the page.

[Manish Jethani, 2017-04-12 11:03:49]

Patch Set 1

If the browser.tabs.insertCSS API is available and supports "user" stylesheets
(Firefox 53), we use it instead of injecting the CSS directly into the page.

[Sebastian Noack, 2017-04-12 12:21:19]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:51: if (!tryInsertCSS)
Chrome and Firefox should use the same code path. The hard-coded browser check
is unnecessary. Just call chrome.tabs.insertCSS({..., cssOrigin: "user"}) and if
it fails send the selectors as response (as outlined in the issue description).

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:71: port.on("hide-elements", (msg, sender) =>
This is out of scope of what is defined in the issue. I'm not sure if the
addional complexity (and messaging overhead) is worth it, just to have
css-property filters injected via user stylesheets (where possible).

[Wladimir Palant, 2017-04-12 12:25:56]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:71: port.on("hide-elements", (msg, sender) =>
On 2017/04/12 12:21:18, Sebastian Noack wrote:
> This is out of scope of what is defined in the issue. I'm not sure if the
> addional complexity (and messaging overhead) is worth it, just to have
> css-property filters injected via user stylesheets (where possible).

Yes, this is an aspect we missed when describing the issue. Manish asked me
about that, and I think that we should definitely stay away from the DOM if we
can - also for the CSS property filters. This should mean slightly more complex
code but no runtime penalty otherwise - the content script already knows from
the get-selectors response whether it should message the background page.

[Manish Jethani, 2017-04-12 12:32:30]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:51: if (!tryInsertCSS)
On 2017/04/12 12:21:18, Sebastian Noack wrote:
> Chrome and Firefox should use the same code path. The hard-coded browser check
> is unnecessary. Just call chrome.tabs.insertCSS({..., cssOrigin: "user"}) and
if
> it fails send the selectors as response (as outlined in the issue
description).

We have to remember whether it failed so we don't try again (as per the issue),
hence the extra variable tryInsertCSS.

Unfortunately this API is available in Firefox only via browser.tabs. It doesn't
work via chrome.tabs. Since we're not trying to do this on Chrome anyway, I
figured we'd check for the existence of the browser object.

Should we try to target Chrome as well so it works automatically in a future
version?

[Sebastian Noack, 2017-04-12 12:54:43]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:51: if (!tryInsertCSS)
On 2017/04/12 12:32:30, Manish Jethani wrote:
> On 2017/04/12 12:21:18, Sebastian Noack wrote:
> > Chrome and Firefox should use the same code path. The hard-coded browser
check
> > is unnecessary. Just call chrome.tabs.insertCSS({..., cssOrigin: "user"})
and
> if
> > it fails send the selectors as response (as outlined in the issue
> description).
> 
> We have to remember whether it failed so we don't try again (as per the
issue),
> hence the extra variable tryInsertCSS.
> 
> Unfortunately this API is available in Firefox only via browser.tabs. It
doesn't
> work via chrome.tabs. Since we're not trying to do this on Chrome anyway, I
> figured we'd check for the existence of the browser object.
> 
> Should we try to target Chrome as well so it works automatically in a future
> version?

Chrome is also going to add support for user stylesheets to
chrome.tabs.insertCSS(), though most likely with different semantics (they don't
want to add the cssOrigin parameter therefore).

Either way, as less diverge we have as better. Does using the browser namespace
requires using promises? If not, perhaps we could first implement
https://issues.adblockplus.org/ticket/5028, in order to minimize the diverge
here, and potentially in other places (in the future).

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:71: port.on("hide-elements", (msg, sender) =>
On 2017/04/12 12:25:56, Wladimir Palant wrote:
> On 2017/04/12 12:21:18, Sebastian Noack wrote:
> > This is out of scope of what is defined in the issue. I'm not sure if the
> > addional complexity (and messaging overhead) is worth it, just to have
> > css-property filters injected via user stylesheets (where possible).
> 
> Yes, this is an aspect we missed when describing the issue. Manish asked me
> about that, and I think that we should definitely stay away from the DOM if we
> can - also for the CSS property filters. This should mean slightly more
complex
> code but no runtime penalty otherwise - the content script already knows from
> the get-selectors response whether it should message the background page.

Keep, in mind that with -abp-selector/:has there will be no way around 
modifying the DOM, as it doesn't generate a new selector but must hide elements
directly. Will property filters be handled completely different from the new
syntax, so that we keep having a selector to inject?

[Wladimir Palant, 2017-04-12 13:37:57]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:51: if (!tryInsertCSS)
On 2017/04/12 12:32:30, Manish Jethani wrote:
> Unfortunately this API is available in Firefox only via browser.tabs. It
doesn't
> work via chrome.tabs

Are you certain? I am not aware of Firefox exposing different functionality via
browser.* than chrome.*. I also checked the schema and the implementation in the
Firefox source code, I cannot see anything special about that parameter, nothing
that would disallow it via chrome.*.

[Manish Jethani, 2017-04-12 14:27:01]

https://codereview.adblockplus.org/29410607/diff/29410608/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29410608/background.js#newco...
background.js:51: if (!tryInsertCSS)
On 2017/04/12 13:37:57, Wladimir Palant wrote:
> On 2017/04/12 12:32:30, Manish Jethani wrote:
> > Unfortunately this API is available in Firefox only via browser.tabs. It
> doesn't
> > work via chrome.tabs
> 
> Are you certain? I am not aware of Firefox exposing different functionality
via
> browser.* than chrome.*. I also checked the schema and the implementation in
the
> Firefox source code, I cannot see anything special about that parameter,
nothing
> that would disallow it via chrome.*.

My bad, it is implemented but it takes a callback instead of returning a
promise. I'll update the code so it uses chrome.tabs.insertCSS instead.

[Manish Jethani, 2017-04-12 14:43:29]

Patch Set 2: Use chrome.tabs.insertCSS instead

[kzar, 2017-04-13 07:56:25]

Your changes so far look really good, glad to have you. Sorry my slowness, but
it looks like Sebastian and Wladimir have been on top of the reviews at least.

 - I guess the commit message (review title) needs to be updated now to mention
the Chrome API name instead?
 - I'm still unable to view the proper diff for include.preload.js, I'm not sure
what's happening there. I guess another Rietveld problem.
 - Generally if you can see how to write unit tests while making changes I'm
sure that would be appreciated by everyone, our tests are lacking at the moment.

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js
File include.preload.js (left):

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js#...
include.preload.js:534: this.shadow = this.createShadowTree();
I guess the shadowTree and related hacks aren't necessary if we can use
insertCSS? Just be careful if you're messing with this code, since the wrapping
code (see the runInPageContext call) needs to run immediately if it's being
used, otherwise the page's scripts might get a chance to mess with the API
before we wrap it!

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js#...
include.preload.js:627: +    if (this.inject)
I think there should be a comment here to explain why we sometimes don't need to
inject the stylesheets. Or perhaps why sometimes we _do_. I think it should be
specific about platforms and if possible versions, to help the next person know
when the code can be simplified in the future.

[Manish Jethani, 2017-04-18 09:25:05]

Patch Set 3: Add comment explaining when we inject styles

> - I guess the commit message (review title) needs to be updated now to mention
the Chrome API name instead?

Done.

> - I'm still unable to view the proper diff for include.preload.js, I'm not
sure what's happening there. I guess another Rietveld problem.

Sorry about this. I've been using the upload.py script. I'll try the other
option next time.

> - Generally if you can see how to write unit tests while making changes I'm
sure that would be appreciated by everyone, our tests are lacking at the moment.

Thanks, I'll take a look.

The only change I've made in this patch set is that I've added a comment
explaining when we inject styles.

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js
File include.preload.js (left):

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js#...
include.preload.js:534: this.shadow = this.createShadowTree();
On 2017/04/13 07:56:24, kzar wrote:
> I guess the shadowTree and related hacks aren't necessary if we can use
> insertCSS? Just be careful if you're messing with this code, since the
wrapping
> code (see the runInPageContext call) needs to run immediately if it's being
> used, otherwise the page's scripts might get a chance to mess with the API
> before we wrap it!

I don't think we can avoid creating the shadow tree here because it's only later
that we know if insertCSS is supported. This information is cached in the
background page, but it would require a roundtrip.

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29410624/include.preload.js#...
include.preload.js:627: +    if (this.inject)
On 2017/04/13 07:56:24, kzar wrote:
> I think there should be a comment here to explain why we sometimes don't need
to
> inject the stylesheets. Or perhaps why sometimes we _do_. I think it should be
> specific about platforms and if possible versions, to help the next person
know
> when the code can be simplified in the future.

I've added a comment.

[Manish Jethani, 2017-04-18 09:34:58]

On 2017/04/12 12:54:43, Sebastian Noack wrote:

> Keep, in mind that with -abp-selector/:has there will be no way around 
> modifying the DOM, as it doesn't generate a new selector but must hide
elements
> directly. Will property filters be handled completely different from the new
> syntax, so that we keep having a selector to inject?

I asked Hubert and he said that for :has we're not using selectors but instead
setting properties on the elements directly. I think it probably still makes
sense to stay out of the DOM wherever possible.

[kzar, 2017-04-20 06:17:15]

On 2017/04/18 09:25:05, Manish Jethani wrote:
> Patch Set 3: Add comment explaining when we inject styles
> 
> > - I guess the commit message (review title) needs to be updated now to
mention
> the Chrome API name instead?
> 
> Done.

Thanks but it still seems a little misleading to me since that API is always
available. Perhaps "Issue 5090 - Use user stylesheets for element hiding if
possible" or something like that?

> > - I'm still unable to view the proper diff for include.preload.js, I'm not
> sure what's happening there. I guess another Rietveld problem.
> 
> Sorry about this. I've been using the upload.py script. I'll try the other
> option next time.

Well upload.py is your only option, either you use it directly or Wladimir's
script that calls it in turn. There are different versions though, did you
download our one from https://codereview.adblockplus.org/static/upload.py ?

> I don't think we can avoid creating the shadow tree here because it's only
later
> that we know if insertCSS is supported. This information is cached in the
> background page, but it would require a roundtrip.

Yea, you could well be right. Shame but I guess not the end of the world.

https://codereview.adblockplus.org/29410607/diff/29416561/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29416561/background.js#newco...
background.js:58: if (error && /\bError processing
cssOrigin\b/.test(error.message) != -1)
From the discussion in the Chromium issue it seems like they expect us to just
check the user agent string to figure out if user stylesheets can be injected,
and the cssOrigin option won't be supported anyway.

So I wonder if we're better off just using the info module to check platform +
version in order to see if the feature is supported, rather than seeing if an
exception is thrown that matches this regexp?

We're just going to have to change the logic in the future otherwise. For
example we're going to have to check the platform and version here anyway and
we're going to have to at least check the platform in lib/css.js before passing
the cssOrigin option which Chrome doesn't like.

[Manish Jethani, 2017-04-20 12:49:32]

On 2017/04/20 06:17:15, kzar wrote:
> On 2017/04/18 09:25:05, Manish Jethani wrote:
> > Patch Set 3: Add comment explaining when we inject styles
> > 
> > > - I guess the commit message (review title) needs to be updated now to
> mention
> > the Chrome API name instead?
> > 
> > Done.
> 
> Thanks but it still seems a little misleading to me since that API is always
> available. Perhaps "Issue 5090 - Use user stylesheets for element hiding if
> possible" or something like that?

Makes sense, done.

> Well upload.py is your only option, either you use it directly or Wladimir's
> script that calls it in turn. There are different versions though, did you
> download our one from https://codereview.adblockplus.org/static/upload.py ?

I'm using the upload.py linked to from the Getting Started document. I've
decided to use the Mercurial extension for future issues. You'll have to bear
with me for this one though.

[Manish Jethani, 2017-04-20 12:50:32]

Patch Set 4: Check platform and platform version for user stylesheet support

https://codereview.adblockplus.org/29410607/diff/29416561/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29416561/background.js#newco...
background.js:58: if (error && /\bError processing
cssOrigin\b/.test(error.message) != -1)
On 2017/04/20 06:17:15, kzar wrote:

> So I wonder if we're better off just using the info module to check platform +
> version in order to see if the feature is supported, rather than seeing if an
> exception is thrown that matches this regexp?

Yeah, this makes sense. I've added a userStylesheetsSupported property to the
css module now. The background page checks the value before trying to use the
hideElements function.

[kzar, 2017-04-21 11:20:22]

https://codereview.adblockplus.org/29410607/diff/29416561/background.js
File background.js (right):

https://codereview.adblockplus.org/29410607/diff/29416561/background.js#newco...
background.js:58: if (error && /\bError processing
cssOrigin\b/.test(error.message) != -1)
On 2017/04/20 12:50:32, Manish Jethani wrote:
> On 2017/04/20 06:17:15, kzar wrote:
> 
> > So I wonder if we're better off just using the info module to check platform
+
> > version in order to see if the feature is supported, rather than seeing if
an
> > exception is thrown that matches this regexp?
> 
> Yeah, this makes sense. I've added a userStylesheetsSupported property to the
> css module now. The background page checks the value before trying to use the
> hideElements function.

Nice, looks good. Could you update the issue description to match?

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode48
lib/css.js:48: try
I guess we don't really need this try catch here any more.

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51
lib/css.js:51: {
Nit: We'd normally put the semicolon on the above line here. I'm curious, did it
pass linting?

[kzar, 2017-04-21 11:21:57]

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51
lib/css.js:51: {
On 2017/04/21 11:20:22, kzar wrote:
> Nit: We'd normally put the semicolon on the above line here. I'm curious, did
it
> pass linting?

Arg, of course I meant `{` not `;`! Like this

chrome.tabs.insertCSS(tabId, {
  ...

[Manish Jethani, 2017-04-21 13:35:52]

On 2017/04/21 11:20:22, kzar wrote:

> Nice, looks good. Could you update the issue description to match?

Updated Trac issue #5090 to match the current implementation.

[Manish Jethani, 2017-04-21 13:39:33]

Patch Set 5: Remove try/catch

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode48
lib/css.js:48: try
On 2017/04/21 11:20:22, kzar wrote:
> I guess we don't really need this try catch here any more.

Removed.

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51
lib/css.js:51: {
On 2017/04/21 11:21:57, kzar wrote:
> On 2017/04/21 11:20:22, kzar wrote:
> > Nit: We'd normally put the semicolon on the above line here. I'm curious,
did
> it
> > pass linting?
> 
> Arg, of course I meant `{` not `;`! Like this
> 
> chrome.tabs.insertCSS(tabId, {
>   ...

It did pass linting. I've seen this style in many other places in the code. In
fact, if I simply move the brace up, it'll fail linting, unless I also remove
one level of indentation from the subsequent lines. I don't feel strongly either
way, but I do have a preference for the style I've used. Let me know if I should
change it.

[kzar, 2017-04-21 14:36:51]

Otherwise LGTM

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29418604/lib/css.js#newcode51
lib/css.js:51: {
On 2017/04/21 13:39:32, Manish Jethani wrote:
> Let me know if I should change it.

I'll defer to Sebastian there.

[Sebastian Noack, 2017-04-29 22:11:04]

https://codereview.adblockplus.org/29410607/diff/29419615/background.js
File background.js (left):

https://codereview.adblockplus.org/29410607/diff/29419615/background.js#oldco...
background.js:48: return {selectors, trace};
How about moving this message handler (i.e. this function + the call that
registers it) to lib/css.js as well? For reference, the background.js file is
historic, and should eventually go away, while we move all new code to modules.

https://codereview.adblockplus.org/29410607/diff/29419615/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29419615/include.preload.js#...
include.preload.js:554: +      else
The duplication here isn't great. Couldn't we handle this diverge within
elemHide.addSelectors() in a more graceful way?

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode18
lib/css.js:18: /** @module css */
I feel "css" being a too generic name for what is done here. However, "elemHide"
is already taken. But we could still call it "elemHideHelper" which would be
consistent with "notificationHelper". Other suggestions are welcome.

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode30
lib/css.js:30: exports.userStylesheetsSupported = info.platform == "gecko" &&
Again, please avoid statically binding logic to the environment, use feature
detection instead. Initially, in the issue tracker we specified it this way, any
reason you changed it? (This really should have been justified with a comment in
the issue when you changed it.)

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode46
lib/css.js:46: "";
Is this function under any circumstances called with an empty array for the
selectors? If so, we shouldn't call insertCSS() either, not just fall back to an
empty string.

[Manish Jethani, 2017-05-01 23:18:06]

Patch Set 6: Move get-selectors and hide-elements to elemHideHelper

Sorry, you'll have to look at the unified diff, Rietveld has messed up again.

My comments inline.

https://codereview.adblockplus.org/29410607/diff/29419615/background.js
File background.js (left):

https://codereview.adblockplus.org/29410607/diff/29419615/background.js#oldco...
background.js:48: return {selectors, trace};
On 2017/04/29 22:11:03, Sebastian Noack wrote:
> How about moving this message handler (i.e. this function + the call that
> registers it) to lib/css.js as well? For reference, the background.js file is
> historic, and should eventually go away, while we move all new code to
modules.

Done, there is now a elemHideHelper module that handles this.

https://codereview.adblockplus.org/29410607/diff/29419615/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29419615/include.preload.js#...
include.preload.js:554: +      else
On 2017/04/29 22:11:03, Sebastian Noack wrote:
> The duplication here isn't great. Couldn't we handle this diverge within
> elemHide.addSelectors() in a more graceful way?

OK, I've added two new options to the addSelectors function.

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode18
lib/css.js:18: /** @module css */
On 2017/04/29 22:11:03, Sebastian Noack wrote:
> I feel "css" being a too generic name for what is done here. However,
"elemHide"
> is already taken. But we could still call it "elemHideHelper" which would be
> consistent with "notificationHelper". Other suggestions are welcome.

Done.

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode30
lib/css.js:30: exports.userStylesheetsSupported = info.platform == "gecko" &&
On 2017/04/29 22:11:04, Sebastian Noack wrote:
> Again, please avoid statically binding logic to the environment, use feature
> detection instead. Initially, in the issue tracker we specified it this way,
any
> reason you changed it? (This really should have been justified with a comment
in
> the issue when you changed it.)

I had used feature detection originally, but then Dave pointed out that we would
have to check the platform details anyway for Chrome, because they're not going
to support cssOrigin. I suppose we could still use feature detection, I'm not
sure it's worth it. It would not even be possible if Chrome starts to accept
cssOrigin but ignore its value, always inserting the CSS as an author stylesheet
until they decide to switch the default. I feel like checking for the platform
here is the safer option.

Yeah, I should have left a comment explaining the change. I'll bear this in mind
for next time.

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode46
lib/css.js:46: "";
On 2017/04/29 22:11:03, Sebastian Noack wrote:
> Is this function under any circumstances called with an empty array for the
> selectors? If so, we shouldn't call insertCSS() either, not just fall back to
an
> empty string.

Done.

[kzar, 2017-05-10 08:43:32]

LGTM

[Wladimir Palant, 2017-05-17 11:50:16]

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js
File lib/css.js (right):

https://codereview.adblockplus.org/29410607/diff/29419615/lib/css.js#newcode30
lib/css.js:30: exports.userStylesheetsSupported = info.platform == "gecko" &&
On 2017/05/01 23:18:05, Manish Jethani wrote:
> I had used feature detection originally, but then Dave pointed out that we
would
> have to check the platform details anyway for Chrome, because they're not
going
> to support cssOrigin. I suppose we could still use feature detection, I'm not
> sure it's worth it.

Given that the values provided by the info module are extracted from the user
agent and are hence inherently unreliable - yes, I think that it is still worth
it. And even if Chrome uses a different name, we could detect that one similarly
then.

[kzar, 2017-05-17 11:55:02]

On 2017/05/17 11:50:16, Wladimir Palant wrote:
> Given that the values provided by the info module are extracted from the user
> agent and are hence inherently unreliable - yes, I think that it is still
worth
> it. And even if Chrome uses a different name, we could detect that one
similarly
> then.

I don't think they plan to use any name for this option, rather insertCSS will
just start working a different way. See this discussion
https://bugs.chromium.org/p/chromium/issues/detail?id=632009&desc=2

[Sebastian Noack, 2017-05-17 12:07:44]

On 2017/05/17 11:55:02, kzar wrote:
> On 2017/05/17 11:50:16, Wladimir Palant wrote:
> > Given that the values provided by the info module are extracted from the
user
> > agent and are hence inherently unreliable - yes, I think that it is still
> worth
> > it. And even if Chrome uses a different name, we could detect that one
> similarly
> > then.
> 
> I don't think they plan to use any name for this option, rather insertCSS will
> just start working a different way. See this discussion
> https://bugs.chromium.org/p/chromium/issues/detail?id=632009&desc=2

They say there that they will consider how they can make this behavior
detectable. I definitely agree that we should use feature detection here, as
initially planned.

[Manish Jethani, 2017-05-18 01:55:54]

On 2017/05/17 11:50:16, Wladimir Palant wrote:

> Given that the values provided by the info module are extracted from the user
> agent and are hence inherently unreliable - yes, I think that it is still
worth
> it.

I tried overriding the value of the user-agent string and then checking the
value of navigator.userAgent. In both Firefox 53 and Chrome 57, the content
script reports the new value, but the background page continues to have the
original value. This is true even when the background page is loaded after the
override is put in effect. Since we're using the info module in the background
page, it seems like this at least shouldn't be a problem, but we'd have to go
back and check every supported version of Chrome to be sure.

Pros and cons, I'm OK with feature detection so let's go with that one. I'll
make the change.

[Manish Jethani, 2017-05-18 02:25:23]

Patch Set 7: Use feature detection

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:55: callback(error);
Doing this for any errors instead of just the one for unsupported cssOrigin. We
could also throw here if it's not an error we're interested in.

[Sebastian Noack, 2017-05-24 08:31:42]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:600: +  addSelectors(selectors, filters, {inject, traceOnly}
= {})
Any reason you wrapped these two new options into an object?

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
Couldn't we just do..

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

below, rather than duplicating this logic?

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (left):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:15: * along with Adblock Plus.  If not, see
<http://www.gnu.org/licenses/>.
This file is new. So how does it come that there are lines in this patch that
are not added, but already seem to exist? It looks like this patch has been
created on top of a previous iteration of this review. Please always upload
combined patches with all changes squashed. Otherwise, it's quite difficult to
review all changes together.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:31: function hideElements(tabId, frameId, selectors,
callback)
Perhaps we should make this function return a promise rather than taking a
callback, in particular considering that the calling code wraps it into a
proomise anyway?

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:55: callback(error);
On 2017/05/18 02:25:23, Manish Jethani wrote:
> Doing this for any errors instead of just the one for unsupported cssOrigin.
We
> could also throw here if it's not an error we're interested in.

Yeah, we definitely should make sure that any unexpected error will be noticed
during development and testing.

[Manish Jethani, 2017-05-24 17:13:35]

Patch Set 8: Make hideElement return a promise

Note: I've used hg review now, but the patch set is still broken. Unified diffs
are working file, for other diffs it gives an error. I don't know what to do
about this. You'll have to bear with this and use the unified diffs for some of
the files (I think it's only include.preload.js).

There's otherwise nothing wrong with the patch set, it will cleanly apply on the
revision that it's based on.

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:600: +  addSelectors(selectors, filters, {inject, traceOnly}
= {})
On 2017/05/24 08:31:41, Sebastian Noack wrote:
> Any reason you wrapped these two new options into an object?

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.

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/24 08:31:41, Sebastian Noack wrote:
> Couldn't we just do..
> 
> if (trace || inject)
>   this.tracer.addSelectors(selectors, filters);
> 
> below, rather than duplicating this logic?

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.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (left):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:15: * along with Adblock Plus.  If not, see
<http://www.gnu.org/licenses/>.
On 2017/05/24 08:31:41, Sebastian Noack wrote:
> This file is new. So how does it come that there are lines in this patch that
> are not added, but already seem to exist? It looks like this patch has been
> created on top of a previous iteration of this review. Please always upload
> combined patches with all changes squashed. Otherwise, it's quite difficult to
> review all changes together.

This is probably upload.py messing up. I'm trying hg review now.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:31: function hideElements(tabId, frameId, selectors,
callback)
On 2017/05/24 08:31:41, Sebastian Noack wrote:
> Perhaps we should make this function return a promise rather than taking a
> callback, in particular considering that the calling code wraps it into a
> proomise anyway?

Done.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:55: callback(error);
On 2017/05/24 08:31:41, Sebastian Noack wrote:
> On 2017/05/18 02:25:23, Manish Jethani wrote:
> > Doing this for any errors instead of just the one for unsupported cssOrigin.
> We
> > could also throw here if it's not an error we're interested in.
> 
> Yeah, we definitely should make sure that any unexpected error will be noticed
> during development and testing.

Done.

[Sebastian Noack, 2017-05-24 18:14:06]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:600: +  addSelectors(selectors, filters, {inject, traceOnly}
= {})
On 2017/05/24 17:13:34, Manish Jethani wrote:
> On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > Any reason you wrapped these two new options into an object?
> 
> 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.

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.

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/24 17:13:34, Manish Jethani wrote:
> On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > Couldn't we just do..
> > 
> > if (trace || inject)
> >   this.tracer.addSelectors(selectors, filters);
> > 
> > below, rather than duplicating this logic?
> 
> 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.

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.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (left):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:15: * along with Adblock Plus.  If not, see
<http://www.gnu.org/licenses/>.
On 2017/05/24 17:13:34, Manish Jethani wrote:
> On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > This file is new. So how does it come that there are lines in this patch
that
> > are not added, but already seem to exist? It looks like this patch has been
> > created on top of a previous iteration of this review. Please always upload
> > combined patches with all changes squashed. Otherwise, it's quite difficult
to
> > review all changes together.
> 
> This is probably upload.py messing up. I'm trying hg review now.

Well, that is what usually happens if you commit your changes for a new patch on
top, but don't specify a revision range to upload.py that includes all changes
related to this review. Are you sure, that's not what happened?

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
Asynchronous errors are still silenced, as they only get logged if
chrome.runtime.lastError is not accessed by any callback. Do we even need this
callback here? Otherwise the whole logic wouldn't need to be asynchronous in the
first place.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:87: if (selectors.length == 0)
Why do we have to treat no selectors as a special case?

[Manish Jethani, 2017-05-25 02:52:27]

Patch Set 9: Change inject to insertMode

Comments inline.

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:600: +  addSelectors(selectors, filters, {inject, traceOnly}
= {})
On 2017/05/24 18:14:05, Sebastian Noack wrote:
> On 2017/05/24 17:13:34, Manish Jethani wrote:
> > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > Any reason you wrapped these two new options into an object?
> > 
> > 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.
> 
> 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.

I've changed it now.

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/24 18:14:05, Sebastian Noack wrote:
> On 2017/05/24 17:13:34, Manish Jethani wrote:
> > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > Couldn't we just do..
> > > 
> > > if (trace || inject)
> > >   this.tracer.addSelectors(selectors, filters);
> > > 
> > > below, rather than duplicating this logic?
> > 
> > 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.
> 
> 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.

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.

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper.js
File lib/elemHideHelper.js (left):

https://codereview.adblockplus.org/29410607/diff/29440618/lib/elemHideHelper....
lib/elemHideHelper.js:15: * along with Adblock Plus.  If not, see
<http://www.gnu.org/licenses/>.
On 2017/05/24 18:14:05, Sebastian Noack wrote:
> On 2017/05/24 17:13:34, Manish Jethani wrote:
> > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > This file is new. So how does it come that there are lines in this patch
> that
> > > are not added, but already seem to exist? It looks like this patch has
been
> > > created on top of a previous iteration of this review. Please always
upload
> > > combined patches with all changes squashed. Otherwise, it's quite
difficult
> to
> > > review all changes together.
> > 
> > This is probably upload.py messing up. I'm trying hg review now.
> 
> Well, that is what usually happens if you commit your changes for a new patch
on
> top, but don't specify a revision range to upload.py that includes all changes
> related to this review. Are you sure, that's not what happened?

That may have been the case, I get your point. hg review has been working well
for me so far.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
On 2017/05/24 18:14:06, Sebastian Noack wrote:
> Asynchronous errors are still silenced, as they only get logged if
> chrome.runtime.lastError is not accessed by any callback. Do we even need this
> callback here? Otherwise the whole logic wouldn't need to be asynchronous in
the
> first place.

We need to resolve the promise, and if it failed then we need to report that to
the caller. The other option is to ignore the error and let the caller believe
that everything went OK, but then they won't try to inject the styles inline,
ads won't be blocked.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:87: if (selectors.length == 0)
On 2017/05/24 18:14:05, Sebastian Noack wrote:
> Why do we have to treat no selectors as a special case?

I thought you said a little bit earlier in the review that if the selectors list
was empty then we could skip calling insertCSS entirely. I do agree, hence the
special case.

https://codereview.adblockplus.org/29410607/diff/29447589/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29447589/include.preload.js#...
include.preload.js:600: +  addSelectorsInline(selectors, filters)
I've separated this code out into its own function because it's just neater this
way.

[Sebastian Noack, 2017-05-30 15:50:33]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/25 02:52:25, Manish Jethani wrote:
> On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > On 2017/05/24 17:13:34, Manish Jethani wrote:
> > > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > > Couldn't we just do..
> > > > 
> > > > if (trace || inject)
> > > >   this.tracer.addSelectors(selectors, filters);
> > > > 
> > > > below, rather than duplicating this logic?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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?

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
On 2017/05/25 02:52:26, Manish Jethani wrote:
> On 2017/05/24 18:14:06, Sebastian Noack wrote:
> > Asynchronous errors are still silenced, as they only get logged if
> > chrome.runtime.lastError is not accessed by any callback. Do we even need
this
> > callback here? Otherwise the whole logic wouldn't need to be asynchronous in
> the
> > first place.
> 
> We need to resolve the promise, and if it failed then we need to report that
to
> the caller. The other option is to ignore the error and let the caller believe
> that everything went OK, but then they won't try to inject the styles inline,
> ads won't be blocked.

What is the scenario in which we'd get an asynchronous error here? Is it worth
to consider to handle it? Or can we perhaps do something better than relying on
content scripts and author stylesheets? Note that if it wouldn't be for Chrome
and older Firefox versions, we wouldn't even care to keep that fallback around.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:87: if (selectors.length == 0)
On 2017/05/25 02:52:26, Manish Jethani wrote:
> On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > Why do we have to treat no selectors as a special case?
> 
> I thought you said a little bit earlier in the review that if the selectors
list
> was empty then we could skip calling insertCSS entirely. I do agree, hence the
> special case.

Yes, but the content script is already checking for this condition.

[Manish Jethani, 2017-05-31 06:21:39]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/30 15:50:30, Sebastian Noack wrote:
> On 2017/05/25 02:52:25, Manish Jethani wrote:
> > On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > > On 2017/05/24 17:13:34, Manish Jethani wrote:
> > > > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > > > Couldn't we just do..
> > > > > 
> > > > > if (trace || inject)
> > > > >   this.tracer.addSelectors(selectors, filters);
> > > > > 
> > > > > below, rather than duplicating this logic?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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?

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)

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
On 2017/05/30 15:50:31, Sebastian Noack wrote:
> On 2017/05/25 02:52:26, Manish Jethani wrote:
> > On 2017/05/24 18:14:06, Sebastian Noack wrote:
> > > Asynchronous errors are still silenced, as they only get logged if
> > > chrome.runtime.lastError is not accessed by any callback. Do we even need
> this
> > > callback here? Otherwise the whole logic wouldn't need to be asynchronous
in
> > the
> > > first place.
> > 
> > We need to resolve the promise, and if it failed then we need to report that
> to
> > the caller. The other option is to ignore the error and let the caller
believe
> > that everything went OK, but then they won't try to inject the styles
inline,
> > ads won't be blocked.
> 
> What is the scenario in which we'd get an asynchronous error here? Is it worth
> to consider to handle it? Or can we perhaps do something better than relying
on
> content scripts and author stylesheets? Note that if it wouldn't be for Chrome
> and older Firefox versions, we wouldn't even care to keep that fallback
around.

I couldn't get Firefox to give an error in the response, but I think it's
supposed to happen when the CSS is invalid. Firefox reports the error in the
console, but it doesn't report it in chrome.runtime.lastError, nor in the catch
handler when you use browser.tabs.insertCSS. Maybe you're right, we could just
ignore any error.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:87: if (selectors.length == 0)
On 2017/05/30 15:50:31, Sebastian Noack wrote:
> On 2017/05/25 02:52:26, Manish Jethani wrote:
> > On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > > Why do we have to treat no selectors as a special case?
> > 
> > I thought you said a little bit earlier in the review that if the selectors
> list
> > was empty then we could skip calling insertCSS entirely. I do agree, hence
the
> > special case.
> 
> Yes, but the content script is already checking for this condition.

First we try to insert the selectors in the background page, then we send the
selectors to the content script. The content script is not in the picture until
later.

[Sebastian Noack, 2017-05-31 11:13:30]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/31 06:21:38, Manish Jethani wrote:
> On 2017/05/30 15:50:30, Sebastian Noack wrote:
> > On 2017/05/25 02:52:25, Manish Jethani wrote:
> > > On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > > > On 2017/05/24 17:13:34, Manish Jethani wrote:
> > > > > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > > > > Couldn't we just do..
> > > > > > 
> > > > > > if (trace || inject)
> > > > > >   this.tracer.addSelectors(selectors, filters);
> > > > > > 
> > > > > > below, rather than duplicating this logic?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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?
> 
> 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)

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

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
On 2017/05/31 06:21:39, Manish Jethani wrote:
> On 2017/05/30 15:50:31, Sebastian Noack wrote:
> > On 2017/05/25 02:52:26, Manish Jethani wrote:
> > > On 2017/05/24 18:14:06, Sebastian Noack wrote:
> > > > Asynchronous errors are still silenced, as they only get logged if
> > > > chrome.runtime.lastError is not accessed by any callback. Do we even
need
> > this
> > > > callback here? Otherwise the whole logic wouldn't need to be
asynchronous
> in
> > > the
> > > > first place.
> > > 
> > > We need to resolve the promise, and if it failed then we need to report
that
> > to
> > > the caller. The other option is to ignore the error and let the caller
> believe
> > > that everything went OK, but then they won't try to inject the styles
> inline,
> > > ads won't be blocked.
> > 
> > What is the scenario in which we'd get an asynchronous error here? Is it
worth
> > to consider to handle it? Or can we perhaps do something better than relying
> on
> > content scripts and author stylesheets? Note that if it wouldn't be for
Chrome
> > and older Firefox versions, we wouldn't even care to keep that fallback
> around.
> 
> I couldn't get Firefox to give an error in the response, but I think it's
> supposed to happen when the CSS is invalid. Firefox reports the error in the
> console, but it doesn't report it in chrome.runtime.lastError, nor in the
catch
> handler when you use browser.tabs.insertCSS. Maybe you're right, we could just
> ignore any error.

In the case of invalid CSS, falling back to the content script wouldn't be much
better, it will merely move the error from one place to another. What we should
ideally do in case of an invalid selector, regardless whether we use insertCSS()
or content scripts to inject it, is bisecting the list of selectors until we
have all selectors injected except the broken one(s). This, however, is
unrelated of the change here.

[Manish Jethani, 2017-05-31 14:41:15]

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29440618/include.preload.js#...
include.preload.js:607: +      if (this.tracer)
On 2017/05/31 11:13:29, Sebastian Noack wrote:
> On 2017/05/31 06:21:38, Manish Jethani wrote:
> > On 2017/05/30 15:50:30, Sebastian Noack wrote:
> > > On 2017/05/25 02:52:25, Manish Jethani wrote:
> > > > On 2017/05/24 18:14:05, Sebastian Noack wrote:
> > > > > On 2017/05/24 17:13:34, Manish Jethani wrote:
> > > > > > On 2017/05/24 08:31:41, Sebastian Noack wrote:
> > > > > > > Couldn't we just do..
> > > > > > > 
> > > > > > > if (trace || inject)
> > > > > > >   this.tracer.addSelectors(selectors, filters);
> > > > > > > 
> > > > > > > below, rather than duplicating this logic?
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > 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)
> 
> Couldn't we detect #3 by selectors being an empty array?

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.

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29447574/lib/elemHideHelper....
lib/elemHideHelper.js:48: resolve(!chrome.runtime.lastError);
On 2017/05/31 11:13:30, Sebastian Noack wrote:
> On 2017/05/31 06:21:39, Manish Jethani wrote:
> > On 2017/05/30 15:50:31, Sebastian Noack wrote:
> > > On 2017/05/25 02:52:26, Manish Jethani wrote:
> > > > On 2017/05/24 18:14:06, Sebastian Noack wrote:
> > > > > Asynchronous errors are still silenced, as they only get logged if
> > > > > chrome.runtime.lastError is not accessed by any callback. Do we even
> need
> > > this
> > > > > callback here? Otherwise the whole logic wouldn't need to be
> asynchronous
> > in
> > > > the
> > > > > first place.
> > > > 
> > > > We need to resolve the promise, and if it failed then we need to report
> that
> > > to
> > > > the caller. The other option is to ignore the error and let the caller
> > believe
> > > > that everything went OK, but then they won't try to inject the styles
> > inline,
> > > > ads won't be blocked.
> > > 
> > > What is the scenario in which we'd get an asynchronous error here? Is it
> worth
> > > to consider to handle it? Or can we perhaps do something better than
relying
> > on
> > > content scripts and author stylesheets? Note that if it wouldn't be for
> Chrome
> > > and older Firefox versions, we wouldn't even care to keep that fallback
> > around.
> > 
> > I couldn't get Firefox to give an error in the response, but I think it's
> > supposed to happen when the CSS is invalid. Firefox reports the error in the
> > console, but it doesn't report it in chrome.runtime.lastError, nor in the
> catch
> > handler when you use browser.tabs.insertCSS. Maybe you're right, we could
just
> > ignore any error.
> 
> In the case of invalid CSS, falling back to the content script wouldn't be
much
> better, it will merely move the error from one place to another. What we
should
> ideally do in case of an invalid selector, regardless whether we use
insertCSS()
> or content scripts to inject it, is bisecting the list of selectors until we
> have all selectors injected except the broken one(s). This, however, is
> unrelated of the change here.

OK, that makes sense. Let's ignore any possible error in the callback then, at
least for now. I'll make the change.

[Manish Jethani, 2017-06-01 21:52:48]

Patch Set 10: Make hideElements synchronous

[Manish Jethani, 2017-06-02 03:42:19]

Patch Set 11: Simplify addSelectors

I got rid of the extra option to addSelectors here.

Patch Set 12: Further simplify addSelectors

Since we no longer care about asynchronous errors from chrome.tabs.insertCSS, we
can simply ignore whether "hide-elements" succeeded the second time when called
via ElementHideEmulation. This greatly simplifies the logic.

Thanks for nudging me in this direction.

[Manish Jethani, 2017-06-02 03:44:38]

On 2017/06/02 03:42:19, Manish Jethani wrote:
> Patch Set 11: Simplify addSelectors
> 
> I got rid of the extra option to addSelectors here.
> 
> Patch Set 12: Further simplify addSelectors
> 
> Since we no longer care about asynchronous errors from chrome.tabs.insertCSS,
we
> can simply ignore whether "hide-elements" succeeded the second time when
called
> via ElementHideEmulation. This greatly simplifies the logic.
      ^^^^

Correction: ElemHideEmulation

Also you can view the unified diff for include.preload.js, it's very
straightforward now.

[kzar, 2017-07-07 14:07:45]

LGTM, do you want to take another look at this Sebastian?

https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js#...
include.preload.js:666: +      // See issue #5090.
Nit: Maybe just put this sentence on the above line?

[Sebastian Noack, 2017-07-07 16:16:11]

https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper....
lib/elemHideHelper.js:57: port.on("get-selectors", (msg, sender) =>
In new code we use component.method (camel-cased) for message names. So this
should probably be "elemhide.getSelectors", and instead of "hide-elements", you
might want to go for "elemhide.injectSelectors".

https://codereview.adblockplus.org/29410607/diff/29453760/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29410607/diff/29453760/metadata.chrome#new...
metadata.chrome:78: lib/elemHideHelper.js
This name is a bit misleading: https://adblockplus.org/elemhidehelper

Just an idea, how about "cssInjection.js"?

[Manish Jethani, 2017-07-09 12:09:39]

Patch Set 13: Update message and file names

https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29410607/diff/29453760/include.preload.js#...
include.preload.js:666: +      // See issue #5090.
On 2017/07/07 14:07:45, kzar wrote:
> Nit: Maybe just put this sentence on the above line?

Done.

https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper.js
File lib/elemHideHelper.js (right):

https://codereview.adblockplus.org/29410607/diff/29453760/lib/elemHideHelper....
lib/elemHideHelper.js:57: port.on("get-selectors", (msg, sender) =>
On 2017/07/07 16:16:09, Sebastian Noack wrote:
> In new code we use component.method (camel-cased) for message names. So this
> should probably be "elemhide.getSelectors", and instead of "hide-elements",
you
> might want to go for "elemhide.injectSelectors".

Done.

https://codereview.adblockplus.org/29410607/diff/29453760/metadata.chrome
File metadata.chrome (right):

https://codereview.adblockplus.org/29410607/diff/29453760/metadata.chrome#new...
metadata.chrome:78: lib/elemHideHelper.js
On 2017/07/07 16:16:09, Sebastian Noack wrote:
> This name is a bit misleading: https://adblockplus.org/elemhidehelper
> 
> Just an idea, how about "cssInjection.js"?

I'll take that suggestion :)

Done.

[kzar, 2017-07-10 12:36:23]

LGTM, but please wait for Sebastian to approve this one.

[Sebastian Noack, 2017-07-17 09:03:06]

LGTM