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

RESOLVED FIXED in Firefox 18

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
6 years ago
7 months ago

People

(Reporter: mcsmurf, Assigned: mcsmurf)

Tracking

Trunk
mozilla19

Firefox Tracking Flags

(firefox18 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: AppsUtils.jsm used getSelectedLocale("browser"), should use getSelectedLocale("global") instead? → AppsUtils.jsm uses getSelectedLocale("browser"), should use getSelectedLocale("global") instead?
Frank - Would you be interested in contributing a patch to fix this?
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 681490 [details] [diff] [review]
Patch
For getting confirmation, I'd suggest asking fabrice for feedback?
(Assignee)

Updated

6 years ago
Attachment #681490 - Flags: review?(fabrice)
(Assignee)

Comment 5

6 years ago
Confirming for now, I think it's a bug.
Assignee: nobody → bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

6 years ago
All tests have passed on try (except for two random orange tests in layout/).
Attachment #681490 - Flags: review?(fabrice) → review+
(Assignee)

Comment 7

6 years ago
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
(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.

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

6 years ago
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: -
Attachment #681490 - Flags: approval-mozilla-aurora?
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.
Attachment #681490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
status-firefox18: --- → fixed
Whiteboard: [leave open]
Target Milestone: --- → mozilla19

Updated

7 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.