Issue 3499 - Use new messaging API in the content policy implementation

Issue 3499 - Use new messaging API in the content policy implementation

Repository: hg.adblockplus.org/adblockplus

[Erik, 2016-03-15 20:57:42]

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

https://codereview.adblockplus.org/29338279/diff/29338280/lib/child/contentPo...
lib/child/contentPolicy.js:475: scheduledNodes = null;
It appears that `postProcessNodes` is run twice now instead of once before
during the first execution, and won't this `scheduledNodes` value be incorrect
when `postProcessNodes` is run a second time?

Either way the code is a little confusing to me, I wonder if there is a less
confusing way to do this?

Perhaps we can generate the class when the add-on starts up and pass that to the
`lib/child/bootstrap.js` file when it is loaded? or leave this to another time
and just use a sync message here for the time being?

[Erik, 2016-03-15 22:57:00]

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.j...
lib/contentPolicy.js:91: port.on("shouldAllow", (message, sender) =>
this.shouldAllow(message));
`(message, sender) => this.shouldAllow(message)` => `(message) =>
this.shouldAllow(message)` ?

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.j...
lib/contentPolicy.js:99: port.on("getCollapsedClass", (message, sender) =>
collapsedClass);
`(message, sender) => collapsedClass` => `() => collapsedClass` ?

[Wladimir Palant, 2016-03-16 10:13:00]

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

https://codereview.adblockplus.org/29338279/diff/29338280/lib/child/contentPo...
lib/child/contentPolicy.js:475: scheduledNodes = null;
On 2016/03/15 20:57:41, Erik wrote:
> It appears that `postProcessNodes` is run twice now instead of once before
> during the first execution, and won't this `scheduledNodes` value be incorrect
> when `postProcessNodes` is run a second time?

No, we don't run this code multiple times (return statement above is important)
- we merely delay it until the class is available. While scheduledNodes list
might increase in the meantime, that's not an issue because all nodes will be
processed.

Passing parameters to child/bootstrap.js will break code locality - all the
sudden the involved parties are no longer contentPolicy.js and
child/contentPolicy.js but also main.js and child/bootstrap.js. Using
synchronous messaging is also not really an option, this was only synchronous
here originally because async messaging with responses was very complex.

Let's try promises, this will hopefully make this easier to follow...

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.j...
lib/contentPolicy.js:91: port.on("shouldAllow", (message, sender) =>
this.shouldAllow(message));
On 2016/03/15 22:57:00, Erik wrote:
> `(message, sender) => this.shouldAllow(message)` => `(message) =>
> this.shouldAllow(message)` ?

I'd rather name all parameters explicitly even if they aren't used. Thomas, what
do you think?

[Thomas Greiner, 2016-03-16 10:59:11]

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338279/diff/29338280/lib/contentPolicy.j...
lib/contentPolicy.js:91: port.on("shouldAllow", (message, sender) =>
this.shouldAllow(message));
On 2016/03/16 10:13:00, Wladimir Palant wrote:
> On 2016/03/15 22:57:00, Erik wrote:
> > `(message, sender) => this.shouldAllow(message)` => `(message) =>
> > this.shouldAllow(message)` ?
> 
> I'd rather name all parameters explicitly even if they aren't used. Thomas,
what
> do you think?

Personally, I also prefer naming parameters explicitly even if they're not used
because it might help if you need it later on.

I guess we've been quite inconsistent with that throughout our code base though
since we don't have a guideline for that. Usually, I'd not comment on such
personal preferences to avoid requiring unnecessary changes to be made.

[Erik, 2016-03-16 21:40:28]

On 2016/03/15 10:57:40, Wladimir Palant wrote:

LGTM

[Thomas Greiner, 2016-03-23 18:47:59]

LGTM, merely a small suggestion

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

https://codereview.adblockplus.org/29338279/diff/29338392/lib/child/contentPo...
lib/child/contentPolicy.js:459: collapsedClass.then(cls =>
Detail: Anything wrong with calling it "collapsedClass" instead of "cls"?
Because the promise is just a different representation of that same value.

[Wladimir Palant, 2016-03-24 07:27:27]

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

https://codereview.adblockplus.org/29338279/diff/29338392/lib/child/contentPo...
lib/child/contentPolicy.js:459: collapsedClass.then(cls =>
On 2016/03/23 18:47:59, Thomas Greiner wrote:
> Detail: Anything wrong with calling it "collapsedClass" instead of "cls"?
> Because the promise is just a different representation of that same value.

They are not the same thing however. I would consider this highly confusing if
collapsedClass could refer to both the promise (variable outside the closure)
and actual value (parameter).