Issue 1663 - Various Firefox-related changes of the first-run page

A bunch of issues only came up when testing in Firefox.

1) Platform-specific utils.js is pointless.

2) Social links should also be updated after subscription contents were downloaded.

3) i18n.js includes lots of Firefox-specific functionality, this one should move into the abstraction layer now that we have one for Firefox.

4) messageResponder.js shouldn't assume that all classes are already defined in the global context - it should require() them explicitly instead.

5) |global| parameter not used consistently in a few places.

6) Synchronous approach to getting application locale doesn't work in Firefox, messaging has to be used there.

[Wladimir Palant, 2015-01-06 22:35:20]

Realized that I forgot to update the fake background page, done that as well
(lots of whitespace-only changes here, sorry about that).

[Sebastian Noack, 2015-01-07 09:57:33]

LGTM

[Thomas Greiner, 2015-01-07 12:19:54]

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n...
i18n.js:22: if (ext.i18n.noSpecialMessages)
Nit: Shouldn't we avoid negatives like that? Instead I'd suggest renaming it to
`ext.i18n.hasSpecialMessages`

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:20: if (typeof ext == "undefined")
Nit: To make it consistent with all other references to `ext` in this file
please change it to `global.ext` here as well

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:97: if (message.what == "issues")
There's quite a bunch of ifs now so please turn this into a switch-statement.

[Sebastian Noack, 2015-01-07 12:22:17]

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n...
i18n.js:22: if (ext.i18n.noSpecialMessages)
On 2015/01/07 12:19:55, Thomas Greiner wrote:
> Nit: Shouldn't we avoid negatives like that? Instead I'd suggest renaming it
to
> `ext.i18n.hasSpecialMessages`

Maybe we don't even need that property. You could just check whether
ext.i18n.getMessage("@@ui_locale") and/or ext.i18n.getMessage("@@bid_dir")
return an non-empty string. Would be even better IMO.

[Sebastian Noack, 2015-01-07 12:23:21]

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:20: if (typeof ext == "undefined")
On 2015/01/07 12:19:55, Thomas Greiner wrote:
> Nit: To make it consistent with all other references to `ext` in this file
> please change it to `global.ext` here as well

Why not just |"ext" in global| then?

[Wladimir Palant, 2015-01-07 16:33:10]

I addressed the comments with the exception of adding a nested switch statement.

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n...
i18n.js:22: if (ext.i18n.noSpecialMessages)
On 2015/01/07 12:22:17, Sebastian Noack wrote:
> On 2015/01/07 12:19:55, Thomas Greiner wrote:
> > Nit: Shouldn't we avoid negatives like that? Instead I'd suggest renaming it
> to
> > `ext.i18n.hasSpecialMessages`
> 
> Maybe we don't even need that property. You could just check whether
> ext.i18n.getMessage("@@ui_locale") and/or ext.i18n.getMessage("@@bid_dir")
> return an non-empty string. Would be even better IMO.

Done.

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:20: if (typeof ext == "undefined")
On 2015/01/07 12:19:55, Thomas Greiner wrote:
> Nit: To make it consistent with all other references to `ext` in this file
> please change it to `global.ext` here as well

Done.

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:97: if (message.what == "issues")
On 2015/01/07 12:19:55, Thomas Greiner wrote:
> There's quite a bunch of ifs now so please turn this into a switch-statement.

Quite frankly, I consider a nested switch statement absolutely unreadable.

[Sebastian Noack, 2015-01-07 16:40:55]

LGTM

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n...
i18n.js:22: if (ext.i18n.noSpecialMessages)
On 2015/01/07 16:33:10, Wladimir Palant wrote:
> On 2015/01/07 12:22:17, Sebastian Noack wrote:
> > On 2015/01/07 12:19:55, Thomas Greiner wrote:
> > > Nit: Shouldn't we avoid negatives like that? Instead I'd suggest renaming
it
> > to
> > > `ext.i18n.hasSpecialMessages`
> > 
> > Maybe we don't even need that property. You could just check whether
> > ext.i18n.getMessage("@@ui_locale") and/or ext.i18n.getMessage("@@bid_dir")
> > return an non-empty string. Would be even better IMO.
> 
> Done.

Usually I'd prefer to store the return value in a variable, instead calling
|ext.i18n.getMessage("@@ui_locale")| twice. But given that we are in the global
scope here, I think it's reasonable to avoid additional variables. Of course you
could just use an IIFE. But I guess it's not important.

Just for understanding, you didn't mind implementing
ext.i18n.getMessage("@@ui_locale"|"@@_bidi_dir") because those can only be
retrieved in the core, while translations can be used also in the content, on
Firefox?

[Wladimir Palant, 2015-01-07 16:52:40]

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/i18n...
i18n.js:22: if (ext.i18n.noSpecialMessages)
On 2015/01/07 16:40:56, Sebastian Noack wrote:
> Just for understanding, you didn't mind implementing
> ext.i18n.getMessage("@@ui_locale"|"@@_bidi_dir") because those can only be
> retrieved in the core, while translations can be used also in the content, on
> Firefox?

Yes, translations in the content process cannot check the locale of the
adblockplus package - that can only be done in the chrome process. While
synchronous messaging would be an option, this doesn't really seem appropriate
here.

[Thomas Greiner, 2015-01-07 17:31:38]

LGTM

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:97: if (message.what == "issues")
On 2015/01/07 16:33:10, Wladimir Palant wrote:
> On 2015/01/07 12:19:55, Thomas Greiner wrote:
> > There's quite a bunch of ifs now so please turn this into a
switch-statement.
> 
> Quite frankly, I consider a nested switch statement absolutely unreadable.

Since for this particular case we're debating personal preferences, let's leave
it as you wrote it for now.

Generally, how about introducing brackets for switch-statements to our coding
style to make them more legible. It would also eliminate the need to consider
the exception that's specified in Mozilla's coding style: "When you need to
declare a variable in a switch, put the block in braces" (see
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...)

[Wladimir Palant, 2015-01-07 19:03:39]

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
File messageResponder.js (right):

http://codereview.adblockplus.org/6432313504169984/diff/5741031244955648/mess...
messageResponder.js:97: if (message.what == "issues")
On 2015/01/07 17:31:38, Thomas Greiner wrote:
> Generally, how about introducing brackets for switch-statements to our coding
> style to make them more legible. It would also eliminate the need to consider
> the exception that's specified in Mozilla's coding style: "When you need to
> declare a variable in a switch, put the block in braces" (see
>
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...)

While I am inclined to agree, this will unfortunately make switch statements
even more verbose than they already are. And it's still going to require two
indentation levels.