[adblockplusui] Issue 4915 - Define ext for background pages in antiadblockInit.js

Issue 3672 - Revert antiadblockInit.js from ext.i18n.getMessage to Utils.getString

[wspee, 2017-01-02 09:34:32]

During the review of https://codereview.adblockplus.org/29366966/ it was also
suggested to replace the usage of Utils.getString with ext.i18n.getMessage.

Unfortunately I screwed that one up by not testing the changes in Firefox, I
assumed ext.i18n was universally available.

Firefox doesn't have ext.i18n and there is no way (afaict) to provide ext.i18n
for antiadblockInit in a sane fashion.

Therefore I propose to revert the switch to ext.i18n.getMessage in
antiadblockInit.

[Sebastian Noack, 2017-01-04 00:04:40]

I have no idea how hard it would be to make ext.i18n for this file on Firefox.
So I'm adding Wladimir. Also note that if you use Utils.getString(), the prefix
"global_" is added again to the message ID.

[Wladimir Palant, 2017-02-16 10:18:46]

On 2017/01/04 00:04:40, Sebastian Noack wrote:
> I have no idea how hard it would be to make ext.i18n for this file on Firefox.

Currently, we only expose ext.i18n for content, so we would need to expose it to
background as well. From the look of it, this would merely require moving
global.ext.i18n from ext/content.js to ext/common.js and changing the require
statement in ext/background.js:

  let {_EventTarget: EventTarget, i18n: exports.i18n} = require("ext_common");

This change is implemented in adblockpluscore apparently, so only the Firefox
repository would need to be adapted. Then we could keep this using ext.i18n.
However, it should no longer assume that ext is defined globally.
messageResponder currently does this:

  var ext = ext || require("ext_background");

I'm not really happy with this approach (suggested by Thomas in
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js...
apparently), this implicitly relies on the fact that redeclaring an already
existing variable has no effect and also violates our ESLint rules unnecessarily
by using var in the first place. This approach won't work for antiadblockInit.js
either because it loads in its own function scope in Chrome (modules are wrapped
in anonymous functions). I would rather make the expectations here explicit:

  let ext = (
    typeof window != "undefined" && window.ext ?
    window.ext :
    require("ext_background")
  );

This is somewhat longer but more obvious and should do both here and in
messageResponder.js.

[Thomas Greiner, 2017-02-16 11:27:51]

On 2017/02/16 10:18:46, Wladimir Palant wrote:
> On 2017/01/04 00:04:40, Sebastian Noack wrote:
> > I have no idea how hard it would be to make ext.i18n for this file on
Firefox.
> 
> Currently, we only expose ext.i18n for content, so we would need to expose it
to
> background as well. From the look of it, this would merely require moving
> global.ext.i18n from ext/content.js to ext/common.js and changing the require
> statement in ext/background.js:
> 
>   let {_EventTarget: EventTarget, i18n: exports.i18n} = require("ext_common");
> 
> This change is implemented in adblockpluscore apparently, so only the Firefox
> repository would need to be adapted. Then we could keep this using ext.i18n.
> However, it should no longer assume that ext is defined globally.
> messageResponder currently does this:
> 
>   var ext = ext || require("ext_background");
> 
> I'm not really happy with this approach (suggested by Thomas in
>
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js...
> apparently), this implicitly relies on the fact that redeclaring an already
> existing variable has no effect and also violates our ESLint rules
unnecessarily
> by using var in the first place. This approach won't work for
antiadblockInit.js
> either because it loads in its own function scope in Chrome (modules are
wrapped
> in anonymous functions). I would rather make the expectations here explicit:
> 
>   let ext = (
>     typeof window != "undefined" && window.ext ?
>     window.ext :
>     require("ext_background")
>   );
> 
> This is somewhat longer but more obvious and should do both here and in
> messageResponder.js.

No objections from my end with this approach.

[wspee, 2017-02-20 13:52:43]

On 2017/02/16 11:27:51, Thomas Greiner wrote:
> On 2017/02/16 10:18:46, Wladimir Palant wrote:
> > On 2017/01/04 00:04:40, Sebastian Noack wrote:
> > > I have no idea how hard it would be to make ext.i18n for this file on
> Firefox.
> > 
> > Currently, we only expose ext.i18n for content, so we would need to expose
it
> to
> > background as well. From the look of it, this would merely require moving
> > global.ext.i18n from ext/content.js to ext/common.js and changing the
require
> > statement in ext/background.js:
> > 
> >   let {_EventTarget: EventTarget, i18n: exports.i18n} =
require("ext_common");
> > 
> > This change is implemented in adblockpluscore apparently, so only the
Firefox
> > repository would need to be adapted. Then we could keep this using ext.i18n.
> > However, it should no longer assume that ext is defined globally.
> > messageResponder currently does this:
> > 
> >   var ext = ext || require("ext_background");
> > 
> > I'm not really happy with this approach (suggested by Thomas in
> >
>
https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js...
> > apparently), this implicitly relies on the fact that redeclaring an already
> > existing variable has no effect and also violates our ESLint rules
> unnecessarily
> > by using var in the first place. This approach won't work for
> antiadblockInit.js
> > either because it loads in its own function scope in Chrome (modules are
> wrapped
> > in anonymous functions). I would rather make the expectations here explicit:
> > 
> >   let ext = (
> >     typeof window != "undefined" && window.ext ?
> >     window.ext :
> >     require("ext_background")
> >   );
> > 
> > This is somewhat longer but more obvious and should do both here and in
> > messageResponder.js.
> 
> No objections from my end with this approach.

See also https://codereview.adblockplus.org/29376555/

[Sebastian Noack, 2017-02-20 16:27:25]

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit...
lib/antiadblockInit.js:32: );
For reference, if it wouldn't be for ESLint's non-undef rule, this could be as
simple as:

  let ext = typeof ext != "undefined" ? ext : require("ext_background");

But even without directly accessing global variables from other scripts, it can
still be a little simpler:

  let ext = typeof window != "undefined" &&
            window.ext || require("ext_background");

[wspee, 2017-03-01 14:53:38]

(Moved Sebastian to cc since he's not available)

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit...
lib/antiadblockInit.js:32: );
On 2017/02/20 16:27:25, Sebastian Noack wrote:
> For reference, if it wouldn't be for ESLint's non-undef rule, this could be as
> simple as:
> 
>   let ext = typeof ext != "undefined" ? ext : require("ext_background");
> 
> But even without directly accessing global variables from other scripts, it
can
> still be a little simpler:
> 
>   let ext = typeof window != "undefined" &&
>             window.ext || require("ext_background");

Done.

[kzar, 2017-03-02 09:36:30]

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit...
lib/antiadblockInit.js:28: let ext = typeof window != "undefined" &&
I'm not sure this is right, since if ext is already declared using let  then
redeclaring it here will throw an exception. Also are we expecting this code to
run somewhere where window doesn't exist?

I also wasn't sure about the best way to do this, see my attempt here
https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js

Thomas what do you think?

[Wladimir Palant, 2017-03-02 11:14:40]

Patch Set 3 works and is in line with Sebastian's comments but no "looks good"
from me because I'm strongly opposed to the logic obfuscation that happened
here. I can approve this if reverted to Patch Set 2 where the logic is obvious.
Otherwise - Thomas is module owner here, his call.

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29376560/lib/antiadblockInit...
lib/antiadblockInit.js:32: );
On 2017/02/20 16:27:25, Sebastian Noack wrote:
> For reference, if it wouldn't be for ESLint's non-undef rule, this could be as
> simple as:
> 
>   let ext = typeof ext != "undefined" ? ext : require("ext_background");

That wouldn't work, ESLint isn't spoiling the fun here but rather preventing a
bug. ext refers to the local let variable here, not to the global one you would
be interested in.

> But even without directly accessing global variables from other scripts, it
can
> still be a little simpler:
> 
>   let ext = typeof window != "undefined" &&
>             window.ext || require("ext_background");

No, not simpler but clearly more obfuscated.

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit...
lib/antiadblockInit.js:28: let ext = typeof window != "undefined" &&
On 2017/03/02 09:36:30, kzar wrote:
> I'm not sure this is right, since if ext is already declared using let  then
> redeclaring it here will throw an exception.

This part is unproblematic, it's a module which means that it always has its own
scope - this code isn't running in global scope and not conflicting with the
globally defined ext variable.

> Also are we expecting this code to
> run somewhere where window doesn't exist?

Yes, we do. Currently Firefox, in future maybe libadblockplus and more.

> I also wasn't sure about the best way to do this, see my attempt here
> https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js

See my comment in this review as to why this approach is a bad one. Too bad that
Sebastian felt that he had to obfuscate my suggestion by misusing logical
operators.

[Thomas Greiner, 2017-03-02 11:41:16]

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit...
lib/antiadblockInit.js:28: let ext = typeof window != "undefined" &&
On 2017/03/02 11:14:39, Wladimir Palant wrote:
> On 2017/03/02 09:36:30, kzar wrote:
> > I'm not sure this is right, since if ext is already declared using let  then
> > redeclaring it here will throw an exception.
> 
> This part is unproblematic, it's a module which means that it always has its
own
> scope - this code isn't running in global scope and not conflicting with the
> globally defined ext variable.

I assume the confusion here comes from the fact that only some files in the
adblockplusui repository are actual modules.

It'd probably help if we turn the other files in this repo into modules as well.

> > I also wasn't sure about the best way to do this, see my attempt here
> >
https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js
> 
> See my comment in this review as to why this approach is a bad one. Too bad
that
> Sebastian felt that he had to obfuscate my suggestion by misusing logical
> operators.

While I find it personally quite convenient to use `let foo = foo || "foo";` in
certain situations, I do agree with Wladimir that we're overdoing it here in
this case.

Is there any reason not to go with the below variant?

let ext;
if (typeof window != "undefined" && window.ext)
  ext = window.ext; // although ESLint probably forces us to use destructuring
here
else
  ext = require("ext_background");

[wspee, 2017-03-03 09:25:21]

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29370594/diff/29377745/lib/antiadblockInit...
lib/antiadblockInit.js:28: let ext = typeof window != "undefined" &&
On 2017/03/02 11:41:16, Thomas Greiner wrote:
> On 2017/03/02 11:14:39, Wladimir Palant wrote:
> > On 2017/03/02 09:36:30, kzar wrote:
> > > I'm not sure this is right, since if ext is already declared using let 
then
> > > redeclaring it here will throw an exception.
> > 
> > This part is unproblematic, it's a module which means that it always has its
> own
> > scope - this code isn't running in global scope and not conflicting with the
> > globally defined ext variable.
> 
> I assume the confusion here comes from the fact that only some files in the
> adblockplusui repository are actual modules.
> 
> It'd probably help if we turn the other files in this repo into modules as
well.
> 
> > > I also wasn't sure about the best way to do this, see my attempt here
> > >
> https://codereview.adblockplus.org/29375899/diff/29377713/messageResponder.js
> > 
> > See my comment in this review as to why this approach is a bad one. Too bad
> that
> > Sebastian felt that he had to obfuscate my suggestion by misusing logical
> > operators.
> 
> While I find it personally quite convenient to use `let foo = foo || "foo";`
in
> certain situations, I do agree with Wladimir that we're overdoing it here in
> this case.
> 
> Is there any reason not to go with the below variant?
> 
> let ext;
> if (typeof window != "undefined" && window.ext)
>   ext = window.ext; // although ESLint probably forces us to use destructuring
> here
> else
>   ext = require("ext_background");

Done. Any objections?

[Wladimir Palant, 2017-03-06 08:33:35]

LGTM

[Thomas Greiner, 2017-03-06 19:24:26]

LGTM