397778 - Localizing extension descriptions - languages codes need to be too specific

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092605 Minefield/3.0a9pre

With the old method of localizing extension descriptions (using the preference value), if I put a value in for xx-YY and the browser was set to use xx then the description would display.  For example, fr-FR and fr.

With the new method for Firefox 3.0, the browser locale must be exactly as specified in the install.rdf file or the default description will be used.  So if I enter a fr-FR description and then set the locale to fr, the English description will be displayed.

All other extension GUI localization works if I create a fr-FR locale and specify fr as the locale so I would expect the description localization to work as well.

Reproducible: Always

Steps to Reproduce:
1. Put a default description in install.rdf
2. Add an entry into the install.rdf for fr-FR locale
2. Change the general.useragent.locale preference to "fr"

Actual Results:  
The default description is displayed.

Expected Results:  
The fr-FR description should display.

Comment 1

[Dave Townsend [:mossop]]

I did specifically check on this when I did the implementation and was advised to do exact matching only. You can specify multiple locales for an em:localized section to cover fr, fr-FR, so I suspect this is a wontfix.

Comment 2

[Michael Kraft [:morac]]

I had a feeling it might be.  The only thing is that now it works one way specifically for the install.rdf file and works a different way for everything else.  I would think it should be consistent. 

Comment 3

[Axel Hecht [:Pike]]

Dave, did you get a reason to do exact checks only, or is this just to make the code less cumbersome? As I'm curious, could you give an lxr link to the actual check?

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Dave, did you get a reason to do exact checks only, or is this just to make the
> code less cumbersome? As I'm curious, could you give an lxr link to the actual
> check?

Irritatingly I can't find the exact discussion about it but I'm pretty sure it was you I spoke to about it on IRC.

This is the code that chooses the appropriate localized set: http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#8113

Comment 5

[Axel Hecht [:Pike]]

I think we should do the following, for a locale code in in the form

ab[-CD[-misc]]

check ab-CD-misc (if there), if not found, check ab-CD (if there), if not found, check ab.

We should go through this for both gLocale and "en-US".

I'd probably copy over the descriptions into a hash to make that slightly easier.

Comment 6

[Dave Townsend [:mossop]]

This is a part of one of the EM P1's and alters the behaviour of extensions, we should fix this in time for beta.

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #5)
> I think we should do the following, for a locale code in in the form
> 
> ab[-CD[-misc]]
> 
> check ab-CD-misc (if there), if not found, check ab-CD (if there), if not
> found, check ab.

I've just looked back over this and the proposed solution doesn't fit with the original request. In the request we have an entry in install.rdf for fr-FR and the browser locale is fr. The proposed solution seems to work for the opposite case, where we have an entry for fr but the browser locale is set to fr-FR.

Comment 8

[Dave Townsend [:mossop]]

The chrome registry locale selector appears to be implemented here:

http://mxr.mozilla.org/seamonkey/source/chrome/src/nsChromeRegistry.cpp#197

Comment 9

[Dave Townsend [:mossop]]

So the chrome rule is:

If we have a locale entry exactly matching the UI locale then use that.
If we have any locale entry with the same major locale part as the UI major locale part then use that.
If we have a locale entry for en-US then use that.

For the second rule the following applies:

en == en-US
en-GB == en-US

Comment 10

[Axel Hecht [:Pike]]

Hrm. Let's try to rephrase it:

We should take the most specific match between gLocale and the language codes in install.rdf, and fall back to the most specific match between "en-US" and the language codes.

Unless you want to really shoot over the top and use intl.accept_languages ;-), which might be a localized pref, and then you could iterate through fy-NL, nl-NL, nl. But as the chrome reg doesn't give you that, either, I wouldn't bother.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This reimplements the locale matching according to the spec. Essentially:

If we have an exact match then use that.
If we have inexact matches then use the one with the most matching parts.
If there is more than one locale with the same number of matching parts then use the most general one (i.e. en in preference to en-GB)

We check the globally set locale first and if there are absolutely no matches then check en-US.

I've broken most of the locale matching algorithm out of the datasource since it is also called from the EM. This also makes it possible to implement a lookup cache for the datasource should we find a perf problem with displaying the add-ons window, but I think that can come later if it is necessary.

Comment 12

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Takes a couple of points from Rob over email, moves the locale loop to inside the function and no need to pass the locale in since we always use the current locale.

Also a couple of additional improvements. Now uses case insensitive matching and we only do the second pass searching for en-US if the current locale is not English. If the current locale is English it would find a valid match if there is a valid match for en-US so if it doesn't then there isn't an English locale present and so no point searching against en-US.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 284445 [details] [diff] [review]
patch rev 2

Looks good!

Comment 14

[Dave Townsend [:mossop]]

Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.256; previous revision: 1.255
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug257155.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug257155.js,v  <--  test_bug257155.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug397778.js,v
done
Checking in toolkit/mozapps/extensions/test/unit/test_bug397778.js;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/test_bug397778.js,v  <--  test_bug397778.js
initial revision: 1.1
done
Checking in toolkit/mozapps/extensions/test/unit/addons/test_bug257155/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug257155/install.rdf,v  <--  install.rdf
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug397778/install.rdf,v
done
Checking in toolkit/mozapps/extensions/test/unit/addons/test_bug397778/install.rdf;
/cvsroot/mozilla/toolkit/mozapps/extensions/test/unit/addons/test_bug397778/install.rdf,v  <--  install.rdf
initial revision: 1.1
done