1334772 - Migrate all callsites that should retrieve current app locale to use the new LocaleService

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

As stated in bug 1325870 comment 0 we currently have at least three ways in the platform that the code is using to retrieve the current locale used by the app:

1) http://searchfox.org/mozilla-central/search?q=general.useragent.locale&case=false&regexp=false&path=
2) http://searchfox.org/mozilla-central/search?q=getApplicationLocale&path=
3) http://searchfox.org/mozilla-central/search?q=getSelectedLocale&path=


I'd like to move all of those callsites to use LocaleService::GetAppLocales (or LocaleService::GetAppLocale).

Looking at the callsites, it seems to me that:

1) should all be using mozILocaleService. All those callsites are really trying to get the current locale and they just assume that the pref will return it.
We should migrate them all to use it.

2) Those are also all wrong, maybe even more. Looking into nsLocaleService it basically assigns system locale to mApplicationLocale on all platforms except of windows where it retrieves GetUserDefaultLCID which is a deprecated function from windows times that doesn't return very useful information.

Which means that all of this code uses potentially different locale than Firefox.

3) This is a bit more complex, and I'd like to get help to understand it fully.

There is a notion of "packages" that this API uses. Majority of calls use package "global", but I see package "browser", and in tests there are some other names like "overpack" and "test-langpack".

Can I safely ignore that notion of packages in the new locale service and just assume that all of those will want to call to a single locale list (that initially will get the locale from nsChromeRegistry for package "global")?

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

NI jfkthame and pike.

Comment 2

[Jonathan Kew (:jfkthame)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
> 3) This is a bit more complex, and I'd like to get help to understand it
> fully.
> 
> There is a notion of "packages" that this API uses. Majority of calls use
> package "global", but I see package "browser", and in tests there are some
> other names like "overpack" and "test-langpack".
> 
> Can I safely ignore that notion of packages in the new locale service and
> just assume that all of those will want to call to a single locale list
> (that initially will get the locale from nsChromeRegistry for package
> "global")?

This looks connected to bug 848297 and its (currently unresolved) followups. It's not clear to me that we can do that until we've moved to the l20n architecture with support for runtime fallbacks, but :Pike will doubtless understand all this better.

Comment 3

[Axel Hecht [:Pike]]

There's some correlation between getApplicationLocale and http://searchfox.org/mozilla-central/search?q=symbol:_ZN19nsICollationFactory15CreateCollationEP9nsILocalePP12nsICollation&redirect=false.

I wonder if we should factor this into nsICollationFactory for those call sites.

....

Comment 4

[Axel Hecht [:Pike]]

http://searchfox.org/mozilla-central/source/browser/components/distribution.js#65 probably needs a real investigation. I suggest mkaply to talk to about that.

This and comment 3 might be good things to get into their own bugs?

....

Comment 5

[Axel Hecht [:Pike]]

Much of the general.useragent.locale stuff is pretty straightforward.

No idea about http://searchfox.org/mozilla-central/source/toolkit/components/downloads/ApplicationReputation.cpp#1226.

http://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#141 is probably another mkaply foo.

http://searchfox.org/mozilla-central/source/toolkit/modules/Locale.jsm#13 might be something that asks to be replaced wholesome?

Crashreporter annotation, http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4300, maybe :bs or ted?

... getSelectedLocale's left at this point.

Comment 6

[Axel Hecht [:Pike]]

getSelectedLocale:

No idea about http://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.js#240. Jonathan?

http://searchfox.org/mozilla-central/source/dom/xul/nsXULPrototypeCache.cpp#490 is special. No idea whom to ask.

(I wonder if http://searchfox.org/mozilla-central/source/editor/composer/nsEditorSpellCheck.cpp#924 should've passed true?)

I didn't dive too deep into the tests, but many of them look like they're actually testing chrome registry, so they want to stay untouched.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

> http://searchfox.org/mozilla-central/source/toolkit/modules/Locale.jsm#13 might be something that asks to be replaced wholesome?

Yep.

And http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/l10n/locale.js as well.

I'll file a bug for that once I get cpp callsites in order.

Comment 8

[Jonathan Kew (:jfkthame)]

(In reply to Axel Hecht [:Pike] from comment #6)
> getSelectedLocale:
> 
> No idea about
> http://searchfox.org/mozilla-central/source/browser/components/
> customizableui/content/panelUI.js#240. Jonathan?

I guess the main reason for setting a lang attribute there is so that hyphenation will work for long icon labels in the panel. So it wants to be the application locale, to get the right hyphenation rules for the UI language.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

One more bit - http://searchfox.org/mozilla-central/search?q=getLocaleComponentForUserAgent&path= - all those calls should be evaluated and either use OSPreferences or be removed all together.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Update on the status of this:

1) GetApplicationLocale has been fixed by bug 1346674 - it was basically taking OS locale, and we switched it all to take App locale.

The prominent change is that JS environment now uses app locale as a default so all `Intl.DateTimeFormat(undefined)` will localize into the current app's locale.

2) GetSelectedLocale is in bug 1347314. It's a sizable patch but a pretty trivial fix thanks to the above - lot's of JS calls were just to get the locale for Intl.DateTimeFormat.

This will enable us to plug L10nRegistry and get LocaleService to negotiate app's locales from both registries at once.

3) uses of `general.useragent.locale` is tackled in bug 1346616 - this is of lower priority, since at least for now we're not going to change the storage of the pref.
I'll still want to get the patch in so that we can unify and cache the callsites, but there should be no visible consequences to this change and hopefully no regressions.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think we can mark it as fixed. There are still some leftovers around but we'll file specific bugs for them as part of the deprecation process.