Issue 3853 - Use new messaging API for the first-run page

Issue 3853 - Use new messaging API for the first-run page

Repository: hg.adblockplus.org/adblockplus

[Thomas Greiner, 2016-04-18 13:22:50]

Looks good, just some minor remarks.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:43: var EventTarget = global.ext._EventTarget = function(port,
windowID)
Detail: It looks a bit contradictory to export something with a "_" prefix
because that prefix is usually associated with internally used variables.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:51: var wrapper = (message, sender) =>
Detail: I noticed that throughout this review you use `var` in situations where
you'd usually use `let` and that you don't use destructuring. So I wonder what's
up with that?

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:61: const Page = require("ext_background").Page;
Detail: I'm wondering why `Page` is in "ext_background" in the first place and
not in here if it's shared functionality.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:70: listener._ext_wrapper = wrapper;
Detail: This property name is not in accordance to JavaScript naming
conventions.

Note that we could use a `Symbol` as a property name instead to avoid potential
conflicts.

[Wladimir Palant, 2016-04-18 15:28:56]

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:43: var EventTarget = global.ext._EventTarget = function(port,
windowID)
On 2016/04/18 13:22:50, Thomas Greiner wrote:
> Detail: It looks a bit contradictory to export something with a "_" prefix
> because that prefix is usually associated with internally used variables.

It's used internally by the ext layer, not meant for external consumption.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:51: var wrapper = (message, sender) =>
On 2016/04/18 13:22:50, Thomas Greiner wrote:
> Detail: I noticed that throughout this review you use `var` in situations
where
> you'd usually use `let` and that you don't use destructuring. So I wonder
what's
> up with that?

The first-run page loads the files via usual <script> tags, without version=1.7
(the latter wouldn't work in Chrome). So let statements are off limits.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:61: const Page = require("ext_background").Page;
On 2016/04/18 13:22:50, Thomas Greiner wrote:
> Detail: I'm wondering why `Page` is in "ext_background" in the first place and
> not in here if it's shared functionality.

It's not shared functionality - pages only exist for the background "page",
there is no such concept for content.

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:70: listener._ext_wrapper = wrapper;
On 2016/04/18 13:22:50, Thomas Greiner wrote:
> Detail: This property name is not in accordance to JavaScript naming
> conventions.
> 
> Note that we could use a `Symbol` as a property name instead to avoid
potential
> conflicts.

Done.

[Thomas Greiner, 2016-04-18 17:47:15]

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:51: var wrapper = (message, sender) =>
On 2016/04/18 15:28:56, Wladimir Palant wrote:
> On 2016/04/18 13:22:50, Thomas Greiner wrote:
> > Detail: I noticed that throughout this review you use `var` in situations
> where
> > you'd usually use `let` and that you don't use destructuring. So I wonder
> what's
> > up with that?
> 
> The first-run page loads the files via usual <script> tags, without
version=1.7
> (the latter wouldn't work in Chrome). So let statements are off limits.

Doesn't the same then also apply to `const`? (see
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7...)

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:61: const Page = require("ext_background").Page;
On 2016/04/18 15:28:56, Wladimir Palant wrote:
> On 2016/04/18 13:22:50, Thomas Greiner wrote:
> > Detail: I'm wondering why `Page` is in "ext_background" in the first place
and
> > not in here if it's shared functionality.
> 
> It's not shared functionality - pages only exist for the background "page",
> there is no such concept for content.

Ok, since this part of the code is only executed in the context of the
background page, mind adding a short comment to clarify that?

https://codereview.adblockplus.org/29338928/diff/29340500/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29340500/ext/common.js#newco...
ext/common.js:23: let wrapperSymbol = Symbol("ext-wrapper");
Detail: I guess the `let` here is unintentional because you said that "let
statements are off limits".

[Wladimir Palant, 2016-04-19 11:57:28]

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco...
ext/common.js:51: var wrapper = (message, sender) =>
On 2016/04/18 17:47:14, Thomas Greiner wrote:
> Doesn't the same then also apply to `const`? (see
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7...)

It doesn't, Firefox has been supporting const in "regular" JavaScript for a
while, let statements on the other hand only with Firefox 44.

https://codereview.adblockplus.org/29338928/diff/29340500/ext/common.js
File ext/common.js (right):

https://codereview.adblockplus.org/29338928/diff/29340500/ext/common.js#newco...
ext/common.js:23: let wrapperSymbol = Symbol("ext-wrapper");
On 2016/04/18 17:47:14, Thomas Greiner wrote:
> Detail: I guess the `let` here is unintentional because you said that "let
> statements are off limits".

Correct, that's the problem when testing in newer Firefox versions :-(

[Thomas Greiner, 2016-04-19 12:06:07]

LGTM