Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29555932: Issue 5781 - Merge messages for regular and emulated element hiding filters (Closed)

Created:
Sept. 25, 2017, 11:07 p.m. by Sebastian Noack
Modified:
Sept. 26, 2017, 11:09 p.m.
Reviewers:
Manish Jethani
CC:
hub, kzar
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 13

Patch Set 2 : Addressed comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -9 lines) Patch
M include.preload.js View 1 2 chunks +4 lines, -7 lines 1 comment Download
M lib/cssInjection.js View 1 3 chunks +9 lines, -2 lines 0 comments Download

Messages

Total messages: 4
Sebastian Noack
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#newcode352 include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns); }, In the next step ...
Sept. 25, 2017, 11:13 p.m. (2017-09-25 23:13:19 UTC) #1
Manish Jethani
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#newcode352 include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns); }, This might read better ...
Sept. 26, 2017, 11:50 a.m. (2017-09-26 11:50:19 UTC) #2
Sebastian Noack
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#newcode352 include.preload.js:352: callback => { callback(this.elemHideEmulationPatterns); }, On 2017/09/26 11:50:18, Manish ...
Sept. 26, 2017, 9:50 p.m. (2017-09-26 21:50:41 UTC) #3
Manish Jethani
Sept. 26, 2017, 10:45 p.m. (2017-09-26 22:45:19 UTC) #4
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.

Powered by Google App Engine
This is Rietveld