Issue 524 - Stop using @-moz-document

Here we split up the monolithic stylesheet we used to inject into a stylesheet common for all domains (unconditional selectors) and a domain-specific one. All kinds of exceptions are already dealt with when generating the domain-specific stylesheet so the hit counter no longer needs to allow some elements - all are hidden, the hit counter is merely counting. The counting is dealt with within the element hiding code - no longer going into content policy checks, so all the special cases there can be removed.

[kzar, 2016-10-06 12:10:47]

I'm probably not the best person to review this, but I've had a go. (CCing Tom
since he is more familiar with Firefox APIs.)

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide....
lib/child/elemHide.js:213: * Channel returning CSS data for the global
stylesheet.
Nit: I guess this comment should be updated now it's also for domain specific
stylesheets.

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide....
lib/child/elemHide.js:236: port.emitSync("getSelectorsForDomain", this._domain)
:
Dumb question but how does getSelectorsForDomain and getUncondtionalSelectors
resolve to ElemHide.getSelectorsForDomain and ElemHide.getUncondtionalSelectors?
I can't see where you've required them.

[Wladimir Palant, 2016-10-06 12:24:06]

Improved comment

[Wladimir Palant, 2016-10-06 12:24:44]

Improved comment

[Wladimir Palant, 2016-10-06 12:27:27]

On 2016/10/06 12:10:47, kzar wrote:
> I'm probably not the best person to review this, but I've had a go.

Well, this is your chance to get accustomed with this code - feel free to ask
questions :). Given that you've implemented the corresponding core changes, you
actually are the person with the best understanding of what I am doing here.

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide....
lib/child/elemHide.js:213: * Channel returning CSS data for the global
stylesheet.
On 2016/10/06 12:10:46, kzar wrote:
> Nit: I guess this comment should be updated now it's also for domain specific
> stylesheets.

Done.

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide....
lib/child/elemHide.js:236: port.emitSync("getSelectorsForDomain", this._domain)
:
On 2016/10/06 12:10:46, kzar wrote:
> Dumb question but how does getSelectorsForDomain and getUncondtionalSelectors
> resolve to ElemHide.getSelectorsForDomain and
ElemHide.getUncondtionalSelectors?
> I can't see where you've required them.

This is the content process - we cannot require ElemHide here (at least not
without duplicating all the data which we don't want). This is a message sent to
the parent process, elemHideFF.js will produce the proper response then.

[kzar, 2016-10-06 13:02:32]

Fair enough, well I don't have many questions now, except I wonder if counting
element hiding hits is worth the price since we no longer need to match
conditionally. (See my inline comment.)

LGTM

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356078/diff/29356079/lib/child/elemHide....
lib/child/elemHide.js:236: port.emitSync("getSelectorsForDomain", this._domain)
:
On 2016/10/06 12:27:28, Wladimir Palant wrote:
> On 2016/10/06 12:10:46, kzar wrote:
> > Dumb question but how does getSelectorsForDomain and
getUncondtionalSelectors
> > resolve to ElemHide.getSelectorsForDomain and
> ElemHide.getUncondtionalSelectors?
> > I can't see where you've required them.
> 
> This is the content process - we cannot require ElemHide here (at least not
> without duplicating all the data which we don't want). This is a message sent
to
> the parent process, elemHideFF.js will produce the proper response then.

Ah I see.

https://codereview.adblockplus.org/29356078/diff/29356098/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356078/diff/29356098/lib/child/elemHide....
lib/child/elemHide.js:279: RequestNotifier.addNodeData(window.document,
window.top, hit);
(Doesn't storing all the details returned by registerElemHideHit for each hidden
element add quite a bit of overhead? On the other hand I guess it's kind of
useful to know when element hiding filters have matched.)

https://codereview.adblockplus.org/29356078/diff/29356098/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29356078/diff/29356098/lib/contentPolicy.j...
lib/contentPolicy.js:115: * @param {String} data.location  location of the
request
FWIW I'm happy to see this change, I remember this code being really confusing
the first time I needed to change it.

[Wladimir Palant, 2016-10-06 13:08:49]

https://codereview.adblockplus.org/29356078/diff/29356098/lib/child/elemHide.js
File lib/child/elemHide.js (right):

https://codereview.adblockplus.org/29356078/diff/29356098/lib/child/elemHide....
lib/child/elemHide.js:279: RequestNotifier.addNodeData(window.document,
window.top, hit);
On 2016/10/06 13:02:32, kzar wrote:
> (Doesn't storing all the details returned by registerElemHideHit for each
hidden
> element add quite a bit of overhead? On the other hand I guess it's kind of
> useful to know when element hiding filters have matched.)

That's how the list of blockable items works - it needs to show everything that
affected the display of the current page. Not having element hiding in there
would leave out an important piece of the data.

The other feature using this data is the issue reporter - it also needs to know
which filters matched on the current page.