adblockplusopera: Port UI code from Chrome

I have ported most of our UI code over from Chrome.

Please only review the following, the rest is just copied as-is:

- background.js
- config.xml
- i18n.js (just the i18n object at the top)
- options.js (only a few changes, best look at the diff to Chrome)

I have deliberately omitted the popup, because both click hiding and enabling/disabling ABP on specific domains is not trivial.

The code is in a branch called port-chrome-ui.

[Wladimir Palant, 2012-10-12 15:04:02]

icons/abp-18.png looks bad. The way to create that icon is to take
icons/abp-16.png and to add once transparent pixel on each side (that's how
icons/abp-19.png was created for Chrome).

I like it how few changes were required in options.js. Maybe add an i18n object
to i18n.js in Chrome as well? Then we can use that file (and i18n.js itself)
pretty much without changes.

http://codereview.adblockplus.org/8560083/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/1/background.js#newcode10
background.js:10: importAll("utils", this);
Please don't use importAll, it hides what is actually being imported. Chrome
uses it because the module system used to work like that and introducing
importAll() was easier in the short term than making it work properly.

http://codereview.adblockplus.org/8560083/diff/1/background.js#newcode183
background.js:183: });
a) Is it necessary to assign explicitly here? Can't this be done by
loadMessages(), without any callback? The file name to be loaded is hardcoded, I
don't see any obvious reason not to hardcode the variable name as well. I would
declare that variable just before loadMessages() for clarity (and also place
loadMessages() call right behind it to group related code).

b) This is asynchronous, are we safe from race conditions here? Please add a
comment noting that this will cause issues with any code trying to access
messages during startup. I *assume* that there will be no issues with the
first-run page when we add it, there might be issues with filter loading code -
it uses strings occasionally (adblockplus_compat doesn't implement
Utils.getString() properly so right now it doesn't matter).

I think that the only synchronous alternative is using a <script> tag but that
would require a change to the messages.json files.

c) Are we using the right language IDs? This is particularly something to be
checked for "pt_BR" and similar, shouldn't it be "pt-BR" in Opera? Also, what if
Opera uses "es-ES" and we provide only "es", will it correctly fall back to our
locale?

http://codereview.adblockplus.org/8560083/diff/1/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode11
i18n.js:11: var text = message.message;
"Uncaught exception: message is undefined". You might get an undefined value as
parameter if the key isn't defined for the locale in question. Personally, I
would prefer to fix the caller - it shouldn't try to get keys that don't exist
in the first place.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode13
i18n.js:13: if (!args)
if (!args || !message.placeholders) I think, otherwise you might fail trying to
iterate over an undefined value.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode23
i18n.js:23: var index = parseInt(content.slice(1));
parseInt(..., 10) please.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode42
i18n.js:42: return "Missing translation: " + key;
Actually, missing translations are currently quite common in our codebase - it
should try to fall back to the "en" locale rather than failing immediately
(that's what Chrome does).

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode66
i18n.js:66: var locale = i18n.getMessage("@@ui_locale");
@@ui_locale is a special string that isn't part of the locale files - it's the
application locale. But that function is an old hack from Adthward, I already
removed all code using it. Let's remove it here and in ABP/Chrome.

[Wladimir Palant, 2012-10-14 06:50:38]

http://codereview.adblockplus.org/8560083/diff/1/options.html
File options.html (right):

http://codereview.adblockplus.org/8560083/diff/1/options.html#newcode265
options.html:265: </div>
We probably want to hide the three options above via JavaScript, currently they
won't do anything.

Also: note that shouldShowIcon option actually needs to be implemented.

[Felix Dahlke, 2012-10-16 14:20:01]

I've addressed all issues. There's now a new file "update_locales.py" that I
would like you to review as well.

http://codereview.adblockplus.org/8560083/diff/1/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/1/background.js#newcode10
background.js:10: importAll("utils", this);
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> Please don't use importAll, it hides what is actually being imported. Chrome
> uses it because the module system used to work like that and introducing
> importAll() was easier in the short term than making it work properly.

Okay, done. I tried to keep it similar to Chrome so I copied even the unused
imports. But background.js differs a lot so that probably doesn't make sense.

http://codereview.adblockplus.org/8560083/diff/1/background.js#newcode10
background.js:10: importAll("utils", this);
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> Please don't use importAll, it hides what is actually being imported. Chrome
> uses it because the module system used to work like that and introducing
> importAll() was easier in the short term than making it work properly.

Done. I actually did that to keep the files similar, but they're so different
that that's probably not worth it.

http://codereview.adblockplus.org/8560083/diff/1/background.js#newcode183
background.js:183: });
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> a) Is it necessary to assign explicitly here? Can't this be done by
> loadMessages(), without any callback? The file name to be loaded is hardcoded,
I
> don't see any obvious reason not to hardcode the variable name as well. I
would
> declare that variable just before loadMessages() for clarity (and also place
> loadMessages() call right behind it to group related code).

Hm, don't you think it would be less obvious that this happens when the script
is loaded if we don't make the call at the bottom? That's where I expect all
immediately executed code.

Anyway, there's no reason not to work with i18nMessages directly, I've changed
that.

> b) This is asynchronous, are we safe from race conditions here? Please add a
> comment noting that this will cause issues with any code trying to access
> messages during startup. I *assume* that there will be no issues with the
> first-run page when we add it, there might be issues with filter loading code
-
> it uses strings occasionally (adblockplus_compat doesn't implement
> Utils.getString() properly so right now it doesn't matter).
> 
> I think that the only synchronous alternative is using a <script> tag but that
> would require a change to the messages.json files.

As discussed on IRC, we'll use a synchronous XHR.
 
> c) Are we using the right language IDs? This is particularly something to be
> checked for "pt_BR" and similar, shouldn't it be "pt-BR" in Opera? Also, what
if
> Opera uses "es-ES" and we provide only "es", will it correctly fall back to
our
> locale?

It seems you're right, they're using a different format. The only documentation
I could find on this is:

http://dev.opera.com/articles/view/creating-multilingual-extensions/

The format is apparently en-gb - all lower case and separated by minus instead
of underscore.

I've written a small script that copies the locales from adblockpluschrome,
renames the folders accordingly and replaces all occurences of "Chrome" with
"Opera". I also had to implement fallback on missing keys, so loadMessages() has
become more complex.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js
File i18n.js (right):

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode11
i18n.js:11: var text = message.message;
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> "Uncaught exception: message is undefined". You might get an undefined value
as
> parameter if the key isn't defined for the locale in question. Personally, I
> would prefer to fix the caller - it shouldn't try to get keys that don't exist
> in the first place.

Done.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode13
i18n.js:13: if (!args)
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> if (!args || !message.placeholders) I think, otherwise you might fail trying
to
> iterate over an undefined value.

You're right, done.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode23
i18n.js:23: var index = parseInt(content.slice(1));
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> parseInt(..., 10) please.

Not again! :) Done.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode42
i18n.js:42: return "Missing translation: " + key;
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> Actually, missing translations are currently quite common in our codebase - it
> should try to fall back to the "en" locale rather than failing immediately
> (that's what Chrome does).

Done, see my changes and comments to background.js.

http://codereview.adblockplus.org/8560083/diff/1/i18n.js#newcode66
i18n.js:66: var locale = i18n.getMessage("@@ui_locale");
On 2012/10/12 15:04:02, Wladimir Palant wrote:
> @@ui_locale is a special string that isn't part of the locale files - it's the
> application locale. But that function is an old hack from Adthward, I already
> removed all code using it. Let's remove it here and in ABP/Chrome.

Done.

http://codereview.adblockplus.org/8560083/diff/1/options.html
File options.html (right):

http://codereview.adblockplus.org/8560083/diff/1/options.html#newcode265
options.html:265: </div>
On 2012/10/14 06:50:38, Wladimir Palant wrote:
> We probably want to hide the three options above via JavaScript, currently
they
> won't do anything.
> 
> Also: note that shouldShowIcon option actually needs to be implemented.

Done.

http://codereview.adblockplus.org/8560083/diff/1/options.html#newcode268
options.html:268: <p
class="i18n_found_a_bug">"https://adblockplus.org/forum/viewforum.php?f=10"</p>
I've also changed this link to the Opera forum, as discussed.

[Wladimir Palant, 2012-10-17 10:17:14]

Publishing incomplete review to get you going.

http://codereview.adblockplus.org/8560083/diff/12001/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode191
background.js:191: request.send();
What happens if the file doesn't exist? Won't this throw an exception?

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode209
background.js:209: function loadI18nMessages()
From the look of it, this function will never load en-gb strings.

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode213
background.js:213: var baseLocale = "en";
Is there an easy way to get the default locale from config.xml? Or would we need
to load/parse it?

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode214
background.js:214: var language = locale.split("-")[0];
locale is undefined?

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode219
background.js:219: if (locale !== language)
What's the point of this check? Shouldn't we always load the strings for the
language?

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode222
background.js:222: loadMessagesForLocale();
Locale-independent messages.json? Sounds pointless.

[Felix Dahlke, 2012-10-18 08:08:45]

I've addressed the issues you've found so far. It seems some parts of my code
are not easy to understand right now, I'd appreciate suggestions.

http://codereview.adblockplus.org/8560083/diff/12001/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode191
background.js:191: request.send();
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> What happens if the file doesn't exist? Won't this throw an exception?

Yes, you're right. Done.

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode209
background.js:209: function loadI18nMessages()
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> From the look of it, this function will never load en-gb strings.

It does, that's the locale I use.

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode213
background.js:213: var baseLocale = "en";
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> Is there an easy way to get the default locale from config.xml? Or would we
need
> to load/parse it?

Oh, didn't notice it was specified there. I have just dug around a little, it
doesn't seem like there is an easy way to get it though, we'd have to parse
config.xml. Seems worth doing though, what do you think?

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode214
background.js:214: var language = locale.split("-")[0];
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> locale is undefined?

No, it's a global. It's defined in a locale-specific file "locale.js", I've
described this in update_locales.py. I've added a comment here as well to make
it clear.

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode219
background.js:219: if (locale !== language)
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> What's the point of this check? Shouldn't we always load the strings for the
> language?

"language" is just the language part, without the country. e.g. just "en".

Locale loading works like this:

1. We first load messages for the base locale, "en"
2. We then load messages for the country part of the user's locale, which is
"en" in the case of "en-gb". So if baseLocale equals languageLocale, we skip it.
3. Finally, we load messages for the user's actual locale. If there is no
country part, we don't bother, because we already have the messages for the
language part.

http://codereview.adblockplus.org/8560083/diff/12001/background.js#newcode222
background.js:222: loadMessagesForLocale();
On 2012/10/17 10:17:14, Wladimir Palant wrote:
> Locale-independent messages.json? Sounds pointless.

messages.json exists once for each locale and is loaded using Opera's mechanism.
See update_locales.py for more details.

[Wladimir Palant, 2012-10-18 12:30:39]

Another incomplete review - as discussed on IRC, I consider the locale fallback
mechanism broken as it is.

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py
File update_locales.py (right):

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode20
update_locales.py:20: return locale.lower().replace("_", "-")
Actually, the correct mapping is a bit more complicated. If I see it correctly,
the differences are:

en-US => en
es => es-ES
es-419 => es-LA
pt-PT => pt

Note: Apparently, we should be using en-US rather than en in Chrome. Other than
that our locale names are correct.

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode30
update_locales.py:30: messages = messages.replace("Chrome", "Opera")
LOL
Let's hope this doesn't have any unintended side-effects...

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode39
update_locales.py:39: # show the country part in navigator.language (only "de",
not "de-de").
That isn't really the case. navigator.lenguage returns Opera's locale name which
is "de", "de-DE" isn't a separate language. It will also return "en-GB" and
"pt-BR". This file shouldn't be required if we use the correct locale mapping.

[Wladimir Palant, 2012-10-18 12:30:42]

Another incomplete review - as discussed on IRC, I consider the locale fallback
mechanism broken as it is.

[Felix Dahlke, 2012-10-18 13:09:10]

Addressed all update_locales.py issues and uploaded a new patch set.

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py
File update_locales.py (right):

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode20
update_locales.py:20: return locale.lower().replace("_", "-")
On 2012/10/18 12:30:40, Wladimir Palant wrote:
> Actually, the correct mapping is a bit more complicated. If I see it
correctly,
> the differences are:
> 
> en-US => en
> es => es-ES
> es-419 => es-LA
> pt-PT => pt
> 
> Note: Apparently, we should be using en-US rather than en in Chrome. Other
than
> that our locale names are correct.

Done.

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode30
update_locales.py:30: messages = messages.replace("Chrome", "Opera")
On 2012/10/18 12:30:40, Wladimir Palant wrote:
> LOL
> Let's hope this doesn't have any unintended side-effects...

Right now it doesn't :) Maybe r"\bChrome\b" to be a bit safer?

http://codereview.adblockplus.org/8560083/diff/12001/update_locales.py#newcode39
update_locales.py:39: # show the country part in navigator.language (only "de",
not "de-de").
On 2012/10/18 12:30:40, Wladimir Palant wrote:
> That isn't really the case. navigator.lenguage returns Opera's locale name
which
> is "de", "de-DE" isn't a separate language. It will also return "en-GB" and
> "pt-BR". This file shouldn't be required if we use the correct locale mapping.

I'm definitely using en-GB and navigator.language returns just "en", "de-de" was
a bad example. I've read about others having this problem, so I went with the
locale.js hack. Can't find the posts anymore though.

[Wladimir Palant, 2012-10-19 15:47:41]

http://codereview.adblockplus.org/8560083/diff/42001/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/42001/background.js#newcode191
background.js:191: request.send();
There is still a potential unhandled exception here :-(
I just tested and a synchronous XMLHttpRequest will throw an exception on
request.send() if a non-existent file is requested. It will *not* complete but
leave request.response uninitialized.

http://codereview.adblockplus.org/8560083/diff/42001/background.js#newcode223
background.js:223: if (locale !== defaultLocale)
I'm not really happy with the hack you use to define the locale variable. How
about just loading the fallback each time? It won't do any harm if the locale is
already "en" but it will make the code simpler and remove reliance on
assumptions about how Opera's locale selection mechanism works.

[Felix Dahlke, 2012-10-19 16:07:27]

Addressed the latest review comments.

http://codereview.adblockplus.org/8560083/diff/42001/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/42001/background.js#newcode191
background.js:191: request.send();
On 2012/10/19 15:47:41, Wladimir Palant wrote:
> There is still a potential unhandled exception here :-(
> I just tested and a synchronous XMLHttpRequest will throw an exception on
> request.send() if a non-existent file is requested. It will *not* complete but
> leave request.response uninitialized.

Yes, it will return an undefined value. My first approach was to check for
exceptions here but I couldn't differentiate between "could not load" and "could
not parse" when the file is loaded locally, so I decided to just return
undefined in case of error, and handle the general case in loadI18nMessages(). I
did test this, just tested it again. Works fine. Would you prefer if I threw an
Error here instead of returning undefined?

http://codereview.adblockplus.org/8560083/diff/42001/background.js#newcode223
background.js:223: if (locale !== defaultLocale)
On 2012/10/19 15:47:41, Wladimir Palant wrote:
> I'm not really happy with the hack you use to define the locale variable. How
> about just loading the fallback each time? It won't do any harm if the locale
is
> already "en" but it will make the code simpler and remove reliance on
> assumptions about how Opera's locale selection mechanism works.

Done. It's not necessary anymore and I'm frankly happy to get rid of that hack,
which was apparently only necessary for OS X anyway.

[Wladimir Palant, 2012-10-22 08:57:51]

LGTM but I guess that we want to incorporate my recent Chrome changes to make
sure the differences are minimal.

http://codereview.adblockplus.org/8560083/diff/42001/background.js
File background.js (right):

http://codereview.adblockplus.org/8560083/diff/42001/background.js#newcode191
background.js:191: request.send();
Ok, it seems that the widget:// protocol behaves differently from file:// here -
my bad. I verified that the fallback works correctly if the locale doesn't
exist.