Closed Bug 397778 Opened 12 years ago Closed 12 years ago

Localizing extension descriptions - languages codes need to be too specific

Categories

(Toolkit :: Add-ons Manager, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: morac, Assigned: mossop)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.
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.
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. 
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?
(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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M9
Flags: blocking-firefox3? → blocking-firefox3+
Assignee: nobody → dtownsend
(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.
The chrome registry locale selector appears to be implemented here:

http://mxr.mozilla.org/seamonkey/source/chrome/src/nsChromeRegistry.cpp#197
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
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.
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Attachment #283541 - Flags: review?(robert.bugzilla)
Attached patch patch rev 2Splinter Review
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.
Attachment #283541 - Attachment is obsolete: true
Attachment #283541 - Flags: review?(robert.bugzilla)
Attachment #284445 - Flags: review?(benjamin)
Whiteboard: [has patch][need review rob/bsmedberg]
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
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][need review rob/bsmedberg]
Keywords: dev-doc-needed
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.