811693 - AppsUtils.jsm uses getSelectedLocale("browser"), should use getSelectedLocale("global") instead?

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Frank Wein [:mcsmurf]]

I've noticed that this code http://hg.mozilla.org/mozilla-central/annotate/dd68409d7810/dom/apps/src/AppsUtils.jsm#l248 is using getSelectedLocale("browser") to get the current locale.
let chrome = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).QueryInterface(Ci.nsIToolkitChromeRegistry);
let locale = chrome.getSelectedLocale("browser").toLowerCase();

I think this is wrong. The code should use getSelectedLocale("global") instead as this is core code that's not specific to the "browser" package.
Also see Bug 811383 on this, some AITC xpcshell tests fail in SeaMonkey due to that. When I replace "browser" with "global" the tests start working. So I think this is the correct fix.

Comment 1

[Jason Smith [:jsmith]]

Frank - Would you be interested in contributing a patch to fix this?

Comment 2

[Frank Wein [:mcsmurf]]

I already submitted my suggested patch from Comment 0 to the try server :) https://tbpl.mozilla.org/?tree=Try&rev=d0c257404deb I just need to know if it's the correct fix as I'm not really familiar with nsIXULChromeRegistry and getSelectedLocale. To me it looks like it's the correct fix, but I would like someone else to confirm this.

Comment 3

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 4

[Jason Smith [:jsmith]]

For getting confirmation, I'd suggest asking fabrice for feedback?

Comment 5

[Frank Wein [:mcsmurf]]

Confirming for now, I think it's a bug.

Comment 6

[Frank Wein [:mcsmurf]]

All tests have passed on try (except for two random orange tests in layout/).

Comment 7

[Frank Wein [:mcsmurf]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe5fc3e1ce5

Leaving open to see/check if this can land on mozilla-aurora, too.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cbe5fc3e1ce5

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Frank Wein [:mcsmurf] from comment #7)
> Pushed to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cbe5fc3e1ce5
> 
> Leaving open to see/check if this can land on mozilla-aurora, too.

You need to ask aurora-approval on the patch instead of leaving open.

Comment 10

[Frank Wein [:mcsmurf]]

Comment on attachment 681490 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: This code won't work in SeaMonkey, xpcshell test times out
Testing completed (on m-c, etc.): All tests that use this piece of code pass (there are a few tests that use ManifestHelper indirectly)
Risk to taking this patch (and alternatives if risky): No risk, Pike assured me that getSelectedLocale("global") works in all Mozilla apps (so also on Android). This will probably change in the future, see Bug 812784. But for now this works fine.
String or UUID changes made by this patch: -

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 681490 [details] [diff] [review]
Patch

[Triage Comment]
Approving for FF18 in support of SeaMonkey, since this is near-zero risk to Firefox and we're weeks away from release.

If this is landed on mozilla-aurora before ~11AM PT tomorrow, this will make the merge from Aurora 18 to Beta 18. If landed 11AM-5PM PT tomorrow on mozilla-beta, it will make the first FF18 Beta. If landed after that, it will end up in the second FF18 beta.

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-aurora/rev/329792016afe