Issue 3208 - Separate contentPolicy module into a parent and child part

This is hopefully fairly straightforward now.

[tschuster, 2015-11-07 12:29:04]

I have to say, this turned out to be really clean.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:83: let handler = (message =>
JSON.stringify(this.shouldAllow(message.data)));
Why is that necessary? Message listeners should be to return everything that can
be JSON-stringified.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:111: * @param fremes {Object[]}
typo: frames

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:148: if (!match && Prefs.enabled)
nit: match can't be true here.

[Wladimir Palant, 2015-11-07 18:57:33]

Yes, this change being clean is the reason for the other twenty patches
preceding it.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:83: let handler = (message =>
JSON.stringify(this.shouldAllow(message.data)));
On 2015/11/07 12:29:03, tschuster wrote:
> Why is that necessary? Message listeners should be to return everything that
can
> be JSON-stringified.

I don't know why but without stringification it doesn't work - it looks like
return values don't go through structured cloning but will be turned into CPOW
instead.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:111: * @param fremes {Object[]}
On 2015/11/07 12:29:03, tschuster wrote:
> typo: frames

Yes, I noticed and fixed this when I added description for location parameter
(when I documented elemhide keys). I will apply the fix here instead, on Monday.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:148: if (!match && Prefs.enabled)
On 2015/11/07 12:29:03, tschuster wrote:
> nit: match can't be true here.

Yes, and neither in the next if block. The original idea of the flow for this
function (around 8 years ago) isn't what it turned out to be. But I better
improve this part of the logic some other time, it has been too many changes
already as it stands now.

[Wladimir Palant, 2015-11-09 10:59:21]

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:83: let handler = (message =>
JSON.stringify(this.shouldAllow(message.data)));
On 2015/11/07 12:29:03, tschuster wrote:
> Why is that necessary? Message listeners should be to return everything that
can
> be JSON-stringified.

I tried this again and removing stringification doesn't seem to have any effect
- not sure what was the problem before. I removed it.

https://codereview.adblockplus.org/29329571/diff/29329705/lib/contentPolicy.j...
lib/contentPolicy.js:111: * @param fremes {Object[]}
On 2015/11/07 12:29:03, tschuster wrote:
> typo: frames

Done.

[Thomas Greiner, 2015-11-11 16:52:33]

Looks good overall. Just some minor stuff I noticed.

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
lib/child/contentPolicy.js:325: Utils.runAsync(postProcessNodes);
I'd suggest either stopping this loop at some point or introducing an
exponentially increasing timeout so that in case we're - for whatever reason -
not getting a collapsed class back that it wouldn't lead to an infinite loop but
instead only to elements not getting collapsed.

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.j...
lib/contentPolicy.js:109: * @param contentType {String}
The documentation of the parameters is no longer accurate since they are no
longer passed separately.

However, the online documentation (see
http://usejsdoc.org/tags-param.html#parameters-with-properties) doesn't consider
object destructuring yet so the suggestion is to work around it by making up a
name for that object (see https://github.com/jsdoc3/jsdoc/issues/987).

FYI: Google's Closure Compiler opted for a different approach:
https://github.com/google/closure-compiler/issues/1114

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.j...
lib/contentPolicy.js:197: addHit(null, contentType, docDomain, false, location,
exception);
Detail: Isn't it more common to use `-1` for a non-existent index (e.g.
Array.prototype.indexOf`)? That'd allow you to use the common `... > -1` check
instead of `typeof ... == "number"` in child/contentPolicy.

[Wladimir Palant, 2015-11-11 17:43:27]

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
lib/child/contentPolicy.js:325: Utils.runAsync(postProcessNodes);
On 2015/11/11 16:52:33, Thomas Greiner wrote:
> I'd suggest either stopping this loop at some point or introducing an
> exponentially increasing timeout so that in case we're - for whatever reason -
> not getting a collapsed class back that it wouldn't lead to an infinite loop
but
> instead only to elements not getting collapsed.

Actually, that's a left-over from a previous version of this code - the parent
will set collapsedClass synchronously, so there is no scenario where retrying
makes sense.

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.j...
lib/contentPolicy.js:109: * @param contentType {String}
On 2015/11/11 16:52:33, Thomas Greiner wrote:
> The documentation of the parameters is no longer accurate since they are no
> longer passed separately.
> 
> However, the online documentation (see
> http://usejsdoc.org/tags-param.html#parameters-with-properties) doesn't
consider
> object destructuring yet so the suggestion is to work around it by making up a
> name for that object (see https://github.com/jsdoc3/jsdoc/issues/987).
> 
> FYI: Google's Closure Compiler opted for a different approach:
> https://github.com/google/closure-compiler/issues/1114

Done.

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.j...
lib/contentPolicy.js:197: addHit(null, contentType, docDomain, false, location,
exception);
On 2015/11/11 16:52:33, Thomas Greiner wrote:
> Detail: Isn't it more common to use `-1` for a non-existent index (e.g.
> Array.prototype.indexOf`)? That'd allow you to use the common `... > -1` check
> instead of `typeof ... == "number"` in child/contentPolicy.

That's what I had initially but frameIndex: -1 doesn't correctly represent the
context here. In fact, the context isn't a frame, and also not some unknown
frame but the original DOM element - so not passing anything for this parameter
seems to be the best option for me.

[Thomas Greiner, 2015-11-11 18:07:32]

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/child/contentPo...
lib/child/contentPolicy.js:325: Utils.runAsync(postProcessNodes);
On 2015/11/11 17:43:25, Wladimir Palant wrote:
> On 2015/11/11 16:52:33, Thomas Greiner wrote:
> > I'd suggest either stopping this loop at some point or introducing an
> > exponentially increasing timeout so that in case we're - for whatever reason
-
> > not getting a collapsed class back that it wouldn't lead to an infinite loop
> but
> > instead only to elements not getting collapsed.
> 
> Actually, that's a left-over from a previous version of this code - the parent
> will set collapsedClass synchronously, so there is no scenario where retrying
> makes sense.

Ah, you're right.

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329863/lib/contentPolicy.j...
lib/contentPolicy.js:197: addHit(null, contentType, docDomain, false, location,
exception);
On 2015/11/11 17:43:25, Wladimir Palant wrote:
> On 2015/11/11 16:52:33, Thomas Greiner wrote:
> > Detail: Isn't it more common to use `-1` for a non-existent index (e.g.
> > Array.prototype.indexOf`)? That'd allow you to use the common `... > -1`
check
> > instead of `typeof ... == "number"` in child/contentPolicy.
> 
> That's what I had initially but frameIndex: -1 doesn't correctly represent the
> context here. In fact, the context isn't a frame, and also not some unknown
> frame but the original DOM element - so not passing anything for this
parameter
> seems to be the best option for me. 

Makes sense then, I agree.

https://codereview.adblockplus.org/29329571/diff/29329965/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329965/lib/contentPolicy.j...
lib/contentPolicy.js:38: let collapsedClass = "";
Detail: This variable is only used inside `Policy.init` so it'd be more
appropriate to move it into there. Previously, `postProcessNodes` also made use
of it but since that function had been moved out of here there's no need to keep
it global anymore.

[Wladimir Palant, 2015-11-12 12:28:52]

https://codereview.adblockplus.org/29329571/diff/29329965/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29329965/lib/contentPolicy.j...
lib/contentPolicy.js:38: let collapsedClass = "";
On 2015/11/11 18:07:32, Thomas Greiner wrote:
> Detail: This variable is only used inside `Policy.init` so it'd be more
> appropriate to move it into there. Previously, `postProcessNodes` also made
use
> of it but since that function had been moved out of here there's no need to
keep
> it global anymore.

Done.

[tschuster, 2015-11-12 14:04:18]

LGTM

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.j...
lib/contentPolicy.js:79:
messageManager.addMessageListener("AdblockPlus:ShouldAllow", handler);
nit: The surrounding brackets are unnecessary.

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.j...
lib/contentPolicy.js:110: * @return {Object} An object containing properties
block, collapse and hits
I think s/block/allow

[Thomas Greiner, 2015-11-12 14:18:55]

LGTM

[Wladimir Palant, 2015-11-12 15:16:32]

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.j...
lib/contentPolicy.js:79:
messageManager.addMessageListener("AdblockPlus:ShouldAllow", handler);
On 2015/11/12 14:04:17, tschuster wrote:
> nit: The surrounding brackets are unnecessary.

This code is going to be replaced in
https://codereview.adblockplus.org/29329742/ anyway, no unnecessary brackets
there. So I rather don't do the rebasing dance for this one again.

https://codereview.adblockplus.org/29329571/diff/29330071/lib/contentPolicy.j...
lib/contentPolicy.js:110: * @return {Object} An object containing properties
block, collapse and hits
On 2015/11/12 14:04:17, tschuster wrote:
> I think s/block/allow

Right, fixed.