Issue 5395 - Make sure element hiding emulation doesn't degrade website performance too much

Note that this is based on revision d30ba414cc3d - branching off to avoid adding more changes to Chrome before release. This will have to be merged with master later.

Two changes here: stylesheet-only document modifications will no longer unnecessarily trigger rules that don't depend on stylesheets (such as :-abp-has()). Also, new stylesheets will be processed at most every 3 seconds.

[Felix Dahlke, 2017-07-10 14:37:19]

LGTM with a nit

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:438: this.addSelectors(stylesheets);
Nit: I'm probably rusty, but couldn't the above three lines be rewritten as:

  this.addSelectors(this._stylesheetQueue);
  this._stylesheetQueue = null;

[Wladimir Palant, 2017-07-10 14:38:52]

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:438: this.addSelectors(stylesheets);
On 2017/07/10 14:37:19, Felix Dahlke wrote:
> Nit: I'm probably rusty, but couldn't the above three lines be rewritten as:
> 
>   this.addSelectors(this._stylesheetQueue);
>   this._stylesheetQueue = null;

The trouble here is reentrance - if addSelector() call somehow ends up calling
onLoad() we'll have an issue.

[kzar, 2017-07-10 14:50:18]

While I'm not the best person to review this change I've had a good go and it
LGTM.

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:354: addSelectors(stylesheets)
Nit: IMO this function could do with a comment to explain the behaviour when the
stylesheets parameter is passed / omitted. It's true I'm not familiar with this
code but that aspect took me a while to grok when it's actually kinda simple.

[Felix Dahlke, 2017-07-10 14:52:14]

Makes sense, even more LGTM then.

I forgot to say earlier that it'd be good to have some test coverage for the
`selectorOnlyChange` logic, but I'm quite fine with that being done later.

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:438: this.addSelectors(stylesheets);
On 2017/07/10 14:38:52, Wladimir Palant wrote:
> On 2017/07/10 14:37:19, Felix Dahlke wrote:
> > Nit: I'm probably rusty, but couldn't the above three lines be rewritten as:
> > 
> >   this.addSelectors(this._stylesheetQueue);
> >   this._stylesheetQueue = null;
> 
> The trouble here is reentrance - if addSelector() call somehow ends up calling
> onLoad() we'll have an issue.

Makes sense.

[Wladimir Palant, 2017-07-11 08:18:11]

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:354: addSelectors(stylesheets)
On 2017/07/10 14:50:17, kzar wrote:
> Nit: IMO this function could do with a comment to explain the behaviour when
the
> stylesheets parameter is passed / omitted. It's true I'm not familiar with
this
> code but that aspect took me a while to grok when it's actually kinda simple.

Done.

[kzar, 2017-07-11 11:05:14]

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29485567/diff/29485568/chrome/content/elem...
chrome/content/elemHideEmulation.js:354: addSelectors(stylesheets)
On 2017/07/11 08:18:11, Wladimir Palant wrote:
> On 2017/07/10 14:50:17, kzar wrote:
> > Nit: IMO this function could do with a comment to explain the behaviour when
> the
> > stylesheets parameter is passed / omitted. It's true I'm not familiar with
> this
> > code but that aspect took me a while to grok when it's actually kinda
simple.
> 
> Done.

Nice, thanks.