542999 - Setting general.useragent.locale in user profile should override any intl.locale.matchOS setting

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch

This is a followup for bug 429033.

On most Linux distros, I believe, intl.locale.matchOS is set to true at GRE level.

Extensions such as Quick Locale Switcher modify the general.useragent.locale preference to allow forcing a given locale.

After bug 519468, it is now possible for users to change the value of intl.locale.matchOS, which is good.

But it also means that Quick Locale Switcher can't work until it also sets intl.locale.matchOS.

I however think that it makes sense for general.useragent.locale, if set from the user preferences, to change the locale, whatever the value of intl.locale.matchOS is.

The attached patch does this, and adapts the test case from bug 519468 to fit this new paradigm. It also changes the manifest depending on the system locale, because the current test fails if the system locale is not en_US.

The new test should work with any system locale. It alternatively tries setting the locale to en-US, the systemLocale, and another different one, that isn't in the manifest (which tests if the fallback to en-US works), while the manifest contains locales for en-US and the systemLocale (if en-US and fr-FR if the systemLocale is en_US). There is an additional test to verify that if the system locale is not represented in the manifest, then we get a locale from the manifest.

Comment 1

[Benjamin Smedberg]

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

Excellent.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/mozilla-central/rev/78d7e307a632

Comment 3

[Axel Hecht [:Pike]]

This doesn't seem to work when you're trying to enable the default language. As I debugged this, that's a feature of the prefs system, you can't set a user pref to the same value as the default pref.

In my local case:
Build is en-US, systemlocale is 'de-DE'. The tests trying to select en-US as locale fail, and I think they're right :-/

Comment 4

[Mike Hommey [:glandium]]

Are you saying that test_bug519468.js fails for you ? At the time I wrote the patch, I tested various values of system locales and it worked in all cases.

Comment 5

[Axel Hecht [:Pike]]

Yes, I do.

The line with
{matchOS: true, selected: locales[0], locale: locales[0]},
fwiw.

(PS, I found it hard to tell which test actually failed, as the actual test data doesn't show up in the log or the stack somehow)

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> Yes, I do.
> 
> The line with
> {matchOS: true, selected: locales[0], locale: locales[0]},
> fwiw.

Confirmed. Now that I think of it, I may have been testing with a complex value as general.useragent.locale, which means setting to en_US would actually not reset the value even if it ended up being the same.

Could you file a new bug about your issue?

> (PS, I found it hard to tell which test actually failed, as the actual test
> data doesn't show up in the log or the stack somehow)

I didn't want to modify how the test works overall, so I wouldn't be responsible for that ;)

Comment 7

[Mike Hommey [:glandium]]

FTR, I see two possible fixes for that shortcoming:
- fix bug 58326
- remove general.useragent.locale from firefox-l10n.js. It's set to chrome://global/locale/intl.properties in greprefs.

Comment 8

[Axel Hecht [:Pike]]

(In reply to comment #7)
> FTR, I see two possible fixes for that shortcoming:
> - fix bug 58326

I'm not optimistic, tbh.

> - remove general.useragent.locale from firefox-l10n.js. It's set to
> chrome://global/locale/intl.properties in greprefs.

This is not. We need the pref to select the locale, which then opens the jar in which the intl.properties is. Chicken and egg.

The other way to resolve this would be to actually just require that locale switchers set both prefs. In other words, back this out.

Comment 9

[Mike Hommey [:glandium]]

(In reply to comment #8)
> This is not. We need the pref to select the locale, which then opens the jar in
> which the intl.properties is. Chicken and egg.

When you have only one locale, this is a non issue. When you have more, you already need to set which one you want explicitely.

Comment 10

[Mike Hommey [:glandium]]

(In reply to comment #9)
> explicitely.

Or implicitely, with matchos.

Comment 11

[Axel Hecht [:Pike]]

That's not really all there is, counter example is a single-locale build with multi-locale extensions.

Comment 12

[Mike Hommey [:glandium]]

(In reply to comment #11)
> That's not really all there is, counter example is a single-locale build with
> multi-locale extensions.

That's comment 9

Comment 13

[Axel Hecht [:Pike]]

no, it isn't.

Comment 14

[Axel Hecht [:Pike]]

I'd like to get this backed out.

There's obviously no easy fix in the chrome registry for the non-working parts, and all other other proposals here don't have owners, and vague impact on the mozilla ecosystem.

And it's keeping me from actually hacking on the bcp47 bug 525494, thus fixing "some time soon" isn't good enough for me.

If we have a fully working patch, I'd be fine with getting this back in the tree, even though I think the easiest, safest, and most straight-forward fix is to just make the locale switcher play with both prefs. Which is, btw, what the fennec locale switcher already does.

Comment 15

[Daniel Veditz [:dveditz]]

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

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 16

[Axel Hecht [:Pike]]

As I mentioned previosly, the patch doesn't even work. I still need to figure out if the tests passed before this landing, or if I screwed up my backout.

Comment 17

[(no longer active)]

I'm going to back this patch out, based on the objections that Pike has raised, which have not been addressed at all here.

Comment 18

[(no longer active)]

And FWIW, we shouldn't take a patch which doesn't address bug 557024, *specifically* one that special cases the test to mask its brokenness for German and French developers, but not the rest of the world.

Comment 19

[(no longer active)]

Backed out:

http://hg.mozilla.org/mozilla-central/rev/d919a5aabf9e (backout)
http://hg.mozilla.org/mozilla-central/rev/0bb90b42a363 (merge)

Comment 20

[(no longer active)]

Followup to the backout to fix a merge conflict resolution error:

http://hg.mozilla.org/mozilla-central/rev/00fb5565fc30

Comment 21

[Mike Hommey [:glandium]]

(In reply to comment #18)
> And FWIW, we shouldn't take a patch which doesn't address bug 557024,
> *specifically* one that special cases the test to mask its brokenness for
> German and French developers, but not the rest of the world.

FWIW, the test was doing the same for all locales: test with en_US, the system locale and one other locale. Except for en_US system locale, where that was en_US and two other locales.

Also, the only reason I remember that makes it not work properly is comment 7.

Comment 22

[Axel Hecht [:Pike]]

To reiterate, the test works perfectly fine, and shows problems with the code on non-en-US systems.

Benjamin and I talked about this at the platform workweek, but I didn't file a bug on it yet. Maybe we can just morph this one.

The idea is to remove intl.locale.matchOS, and to make that a non-locale special value for general.useragent.locale. That requires that all consumers of that pref handle that right, or rather, not consume it directly, though.

Not sure if this would still be in scope for Fx4, Benjamin?

Comment 23

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

I think this is now fixed? We have one pref `intl.locale.requested` that, if set to a value, cancels the old `matchOS` behavior. Reopen if needed.