711041 - Improve l10n language matching algorithm

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

The current algorithm tries to mimic Firefox one:
  http://mxr.mozilla.org/mozilla-central/source/chrome/src/nsChromeRegistryChrome.cpp#93

But this code is known to have a sub-optimal behavior for any non-naive locale. On top of that, my implementation isn't working exactly like Firefox one, as thanksfully reported Tomoya in bug 691782 comment 14.

I've explicitely decided to stick to a simple algorithm in order to land a first patch quickly. So that we can take more time to get feedback and review a proper new way to select a locale.

Regarding my discussion with Pike, we should:
 1) First, read `general.useragent.locale`,
 2) Then, iterate on each item of `intl.accept_languages`,
 3) Finally, if none match, we have multiple options:
  - fallback to the "default addon locale". We will need to specify it somewhere in package.json or in code,
  - fallback to the first locale file we have,
  - fallback to en-US, if we have it, or fallback to one of previous option.

How should we match locales for a given language?
  ja match ja, ja-JP, ja-JP-mac, i.e /ja(-.*)?/
  ja-JP match ja-JP, ja-JP-mac, i.e /ja-JP(-.*)?/
(See bug 691782 comment 14)
I don't know if the priority should be random or not?
Except in case, we have exactly same form, we will select it first.

In the current implementation, I'm having a "loosy" matching where we match `fr` when locale is `fr-FR`, only after previously described choices are applied. Should I keep doing such "loosy matches" ? May be only for 1) ?
It looks like the RFC doesn't suggest such behavior:
http://tools.ietf.org/html/rfc4647#page-14

Comment 1

[Alexandre Poirot [:ochameau]]

Tomoya: btw, as you have already provided a meaningfull patch, if you feel like working on this. I'd be happy to help you hacking on the SDK code!
Otherwise, no worries, I'll dedicate some time to this soon after bug 691782 landing.

Comment 2

[Dave Townsend [:mossop]]

Alex, you might find the algorithm here that Pike helped me develop for the add-ons manager useful: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#392

At some point there was talk of making an xpcom service that would implement something like this so all the different places we do locale selection could use it, not sure what happened with that.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 329

Thanks Dave, that's a really usefull piece of code!!
I have a question about `getLocale()` function.
I uses getComplexValue:
  http://hg.mozilla.org/mozilla-central/annotate/f76b576a9e28/toolkit/mozapps/extensions/XPIProvider.jsm#l383
It was introduce via this bug:
  https://bugzilla.mozilla.org/show_bug.cgi?id=642203#c16

Do you think that's something we should take care of?
If yes, Do you know where/how is set such preference value? I'd like to add a specific unit test and implement this if it is relevant.

And do you remember things about "intl.locale.matchOS"? 
I'd like to add some comment about this preference.

Comment 4

[Axel Hecht [:Pike]]

What happens when we change the meaning of that pref? Background, there are a few bugs in the chrome registry that don't blend well with how people are using the pref today. For in-tree users, we'll need to adapt that, but for jetpacks?

Comment 5

[Alexandre Poirot [:ochameau]]

You mean the matchOS pref? If something has to be changed, we will have to change AddonManager and Jetpack code at the same time. This code is just like a fork of Mossop's code.
Do you have some concrete example about this, because I don't really see what's wrong here?

Comment 6

[Axel Hecht [:Pike]]

I actually mean general.useragent.locale, the hidden agenda is to get rid of matchOS as a separate pref alltogether. If you want to see what's wrong in the chrome registry, just set your OS locale to 'de' and try to get the tests to pass.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Alexandre Poirot (:ochameau) from comment #3)
> Created attachment 589880 [details]
> Pull request 329
> 
> Thanks Dave, that's a really usefull piece of code!!
> I have a question about `getLocale()` function.
> I uses getComplexValue:
>  
> http://hg.mozilla.org/mozilla-central/annotate/f76b576a9e28/toolkit/mozapps/
> extensions/XPIProvider.jsm#l383
> It was introduce via this bug:
>   https://bugzilla.mozilla.org/show_bug.cgi?id=642203#c16
> 
> Do you think that's something we should take care of?
> If yes, Do you know where/how is set such preference value? I'd like to add
> a specific unit test and implement this if it is relevant.

Yes when reading that pref you should assume it itself might be localized. Fennec did this for a time, I'm not sure of their future plans but supporting it is straightforward so let's just do it.

> And do you remember things about "intl.locale.matchOS"? 
> I'd like to add some comment about this preference.

That pref overrides the locale set in prefs and instead attempts to get it from the OS IIRC.

Comment 8

[Axel Hecht [:Pike]]

PS: also many linux distros ship as a multi-locale build.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 589880 [details]
Pull request 329

I implemented the handling of "localized preference" for general.useragent.locale.

irakli: I'd have liked to avoid asking all my reviews to you, but you are the only one left available for JS stuff ...
Here is some additional comments for the review:
I removed all code relative to locale matching from addon-kit/l10n to a new api-utils/locale module, that handles reading all firefox preferences and apply a precise algorithm to build the order of preference in which locales should be used

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 589880 [details]
Pull request 329

Tomoya, feel free to give your feedback on this patch.
I'm using exactly same algorithm than AddonManager to order locales preferences.
I've done my best to handle locales gracefully, but as it is a complex subject, I may have miss something. The only different thing is that I use content locales too, specified in "intl.accept_languages" preference.
You may give a look at the unit test, that highlight main choices made by
https://github.com/mozilla/addon-sdk/pull/329/files#L2R104

The ordering algorithm live here:
https://github.com/mozilla/addon-sdk/pull/329/files#L1R85

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 589880 [details]
Pull request 329

I do like overall change, I just want you to address array.add before landing or talk me out why we should not do that. All the other comments are suggestions which you're welcome to address, but it's up to you. In any case I don't think this require another round.

Thanks!

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 589880 [details]
Pull request 329

I've split code in mutliple pieces and try to implement better error/edge cases/fallback handling. I ended up adding two methods in preferences-service, so that it will need another round!

Please see my comment about array.add.

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/16c7de0e53e33b294c8f59f3e0e52bc56d9f0d3b
Merge pull request #329 from ochameau/improve-locale-matching

Bug 711041: Improve locale matching in l10n module r=@gozala

Comment 14

[dynamis (Tomoya ASAI)]

Comment on attachment 589880 [details]
Pull request 329

The module works fine but your test includes wrong code.
I filed new pull request on github about this:
https://github.com/mozilla/addon-sdk/pull/349

But this bug is already RESOLVED FIXED, should I file another bug?