Issue 5781 - Merge messages for regular and emulated element hiding filters

Issue 5781 - Merge messages for regular and emulated element hiding filters

[Sebastian Noack, 2017-09-25 23:13:19]

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js#...
include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns);
},
In the next step we should pass patterns just as argument to apply(). Passing in
the window here isn't necessary either, anymore. Hubert, FYI.

[Manish Jethani, 2017-09-26 11:50:19]

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js#...
include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns);
},
This might read better as another method of ElemHide, similar to addSelectors
and hideElements, then we can simply pass that method:

  getEmulationPatterns(callback)
  { 
    callback(this.elemHideEmulationPatterns);
  },

But no strong opinion.

If we leave it as it is, we can lose the braces as the caller doesn't care about
the return value and it would make it semantically identical to the
addSelectorsFunc and hideElemsFunc arguments.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:47: let emulated = [];
How about we call this emulationPatterns, similar to how it's referred to in the
content script? "emulated" being an adjective, it feels more like a boolean
value to me than an array of objects.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:55: let hostname = extractHostFromFrame(sender.frame);
I notice that this is a change in behavior, the old one just uses
sender.frame.url.hostname, maybe we should add this information to the Trac
issue.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:65: emulated.push({selector: filter.selector, text:
filter.text});
Just an idea, this could be written idiomatically as:

  emulationPatterns = ElemHideEmulation.getRulesForDomain(hostname)
                      .map(({selector, text}) => ({selector, text}));

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:71: let response = {trace, inject, emulated};
Maybe not as part of this very change, but we should consider not passing the
emulation patterns to the content script at all unless necessary. If user style
sheets are supported (which will soon be true for Chrome as well!), we should
just inject the selectors here and then pass the emulation patterns to the
content script only if trace is true.

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

  ...
  if (trace || inject)
  {
    response.selectors = selectors;
    response.emulationPatterns = emulationPatterns;
  }

This would also simplify the code in the content script, plus we could get rid
of "elemhide.injectSelectors"

[Sebastian Noack, 2017-09-26 21:50:41]

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/include.preload.js#...
include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns);
},
On 2017/09/26 11:50:18, Manish Jethani wrote:
> This might read better as another method of ElemHide, similar to addSelectors
> and hideElements, then we can simply pass that method:
> 
>   getEmulationPatterns(callback)
>   { 
>     callback(this.elemHideEmulationPatterns);
>   },
> 
> But no strong opinion.
> 
> If we leave it as it is, we can lose the braces as the caller doesn't care
about
> the return value and it would make it semantically identical to the
> addSelectorsFunc and hideElemsFunc arguments.

Since you don't have a strong opinion, I think I prefer to keep the arrow
function here. After all this logic is trivial, even more than before where we
used an arrow function as well. And in the next step we should get rid of this
callback completely, anyway. But I guess I can remove the braces, did that.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:47: let emulated = [];
On 2017/09/26 11:50:18, Manish Jethani wrote:
> How about we call this emulationPatterns, similar to how it's referred to in
the
> content script? "emulated" being an adjective, it feels more like a boolean
> value to me than an array of objects.

Done.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:55: let hostname = extractHostFromFrame(sender.frame);
On 2017/09/26 11:50:18, Manish Jethani wrote:
> I notice that this is a change in behavior, the old one just uses
> sender.frame.url.hostname, maybe we should add this information to the Trac
> issue.

You are right, and extractHostFromFrame() should be used, the old behavior was
wrong. The difference is that if the message got sent from within a frame with
dynamically constructed content, the URL will be "about:blank" and the hostname
will be "", essentially only matching generic filters (emulation filters cannot
be generic). Instead we must fall back to the parent frame's hostname, this is
what extractHostFromFrame() is doing. I updated the issue.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:65: emulated.push({selector: filter.selector, text:
filter.text});
On 2017/09/26 11:50:18, Manish Jethani wrote:
> Just an idea, this could be written idiomatically as:
> 
>   emulationPatterns = ElemHideEmulation.getRulesForDomain(hostname)
>                       .map(({selector, text}) => ({selector, text}));

This will result in less efficient code for two reasons:

1. We create yet another object (array), even if there are no emulation filters.
That is also true for regular element hiding, but avoiding it there isn't
possible without additional boilerplate, which probably isn't worth it.
2. map() has to call from native code back into JS for each item, and the JIT
compiler cannot inline these calls.

There is no need to go crazy about micro optimizations here, but the more
efficient approach is IMO also more readable, but defiantly not any less.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:71: let response = {trace, inject, emulated};
On 2017/09/26 11:50:18, Manish Jethani wrote:
> Maybe not as part of this very change, but we should consider not passing the
> emulation patterns to the content script at all unless necessary. If user
style
> sheets are supported (which will soon be true for Chrome as well!), we should
> just inject the selectors here and then pass the emulation patterns to the
> content script only if trace is true.
> 
>   if (!inject && (selectors.length > 0 || emulationPatterns.length > 0)
>     hideElements(sender.page.id, sender.frame.id, selectors,
emulationPatterns);
> 
>   ...
>   if (trace || inject)
>   {
>     response.selectors = selectors;
>     response.emulationPatterns = emulationPatterns;
>   }
> 
> This would also simplify the code in the content script, plus we could get rid
> of "elemhide.injectSelectors"

It seems you misunderstand the concept of emulation filters. These filters
provide non-standard CSS features implemented by our content scripts, therefore
the content script must process them regardless whether user stylesheets are
supported or not. In some cases the content script will transfrom them (based on
what is in the DOM) to compatible selectors which are then sent back to the
background page through elemhide.injectSelectors message if user stylesheets are
supported, while more advanced emulation filters will cause the elements to be
hidden directly through the DOM.

https://codereview.adblockplus.org/29555932/diff/29556751/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29555932/diff/29556751/include.preload.js#...
include.preload.js:506: this.emulatedPatterns = null;
Don't leak memory.

[Manish Jethani, 2017-09-26 22:45:19]

LGTM

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:65: emulated.push({selector: filter.selector, text:
filter.text});
On 2017/09/26 21:50:40, Sebastian Noack wrote:
> On 2017/09/26 11:50:18, Manish Jethani wrote:
> > Just an idea, this could be written idiomatically as:
> > 
> >   emulationPatterns = ElemHideEmulation.getRulesForDomain(hostname)
> >                       .map(({selector, text}) => ({selector, text}));
> 
> This will result in less efficient code for two reasons:
> 
> 1. We create yet another object (array), even if there are no emulation
filters.
> That is also true for regular element hiding, but avoiding it there isn't
> possible without additional boilerplate, which probably isn't worth it.
> 2. map() has to call from native code back into JS for each item, and the JIT
> compiler cannot inline these calls.

Fair enough.

I just checked on both Node.js and Chrome and map does in fact take about twice
as long to execute, while on Firefox it performs just as well as let...of.

https://codereview.adblockplus.org/29555932/diff/29555933/lib/cssInjection.js...
lib/cssInjection.js:71: let response = {trace, inject, emulated};
On 2017/09/26 21:50:40, Sebastian Noack wrote:
> On 2017/09/26 11:50:18, Manish Jethani wrote:
> > Maybe not as part of this very change, but we should consider not passing
the
> > emulation patterns to the content script at all unless necessary. If user
> style
> > sheets are supported (which will soon be true for Chrome as well!), we
should
> > just inject the selectors here and then pass the emulation patterns to the
> > content script only if trace is true.
> > 
> >   if (!inject && (selectors.length > 0 || emulationPatterns.length > 0)
> >     hideElements(sender.page.id, sender.frame.id, selectors,
> emulationPatterns);
> > 
> >   ...
> >   if (trace || inject)
> >   {
> >     response.selectors = selectors;
> >     response.emulationPatterns = emulationPatterns;
> >   }
> > 
> > This would also simplify the code in the content script, plus we could get
rid
> > of "elemhide.injectSelectors"
> 
> It seems you misunderstand the concept of emulation filters. These filters
> provide non-standard CSS features implemented by our content scripts,
therefore
> the content script must process them regardless whether user stylesheets are
> supported or not. In some cases the content script will transfrom them (based
on
> what is in the DOM) to compatible selectors which are then sent back to the
> background page through elemhide.injectSelectors message if user stylesheets
are
> supported, while more advanced emulation filters will cause the elements to be
> hidden directly through the DOM.

Yes, it seems I misunderstood. I was thinking we could just do "new
ElemHideEmulation(...).apply(...)" in the background page, but apparently that
won't work.