519468 - matchOS is read from default prefs at startup

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dirty workaround

yeah, that a dupe of 429033, but I'm not sure we want to correct the original bug instead of using a workaround for B4, so I'm duplicating the bug.

Furthermore, my workaround is dirty! :)

Comment 1

[Axel Hecht [:Pike]]

This bug probably belongs wherever bug 429033 belongs.

From the chat in #developers, it's not really clear what this means in cooperation with fastload.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-2

this one looks more like what it's actually done for the command line overriding of the locale.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 403633 [details] [diff] [review]
WIP-2

>+      if (matchOS) {
>+        // compute lang and region code only when needed!
>+        nsCAutoString uiLocale;
>+        rv = getUILangCountry(uiLocale);
>+        if (NS_SUCCEEDED(rv))
>+          mSelectedLocale = uiLocale;

We should also remove any observer for SELECTED_LOCALE_PREF like we do above. But only if matchOS is true

>+      }
>+      else {
>+        nsXPIDLCString provider;
>+        rv = prefs->GetCharPref(SELECTED_LOCALE_PREF, getter_Copies(provider));
>+        if (NS_SUCCEEDED(rv))
>+          mSelectedLocale = provider;

And we probably need to add an observer for SELECTED_LOCALE_PREF here, just to mimic the behavior that happens in ::Init()

I think this patch has potential. Hopefully we won't need to flush any fastload caches.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-3

* Address comments
* Add a test to see if matchOS is different from the default one once the user prefs has been loaded

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 403762 [details] [diff] [review]
WIP-3

This patch is modeled on the way the UILocale commandline flag works.

This patch listens for "profile-after-change" to adjust the selected locale from the user prefs. It is actually fired sooner than the UILocale code. The UILocale code does not call FlushAllCaches. Since this patch fires before UILocale, it doesn't call FlushAllCaches either.

That might be an oversight for UILocale and therefore something we would need to do.

Comment 6

[Benjamin Smedberg]

Comment on attachment 403762 [details] [diff] [review]
WIP-3

That's a big hunk of code you've just duplicated. Please make a helper function to avoid the duplication.

Comment 7

[Benjamin Smedberg]

But I don't actually understand why you need to do it this way... can't you just install a pref observer for matchos, the same way there's already a pref observer for the locale/skin prefs?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-4

* Use an helper function
* Remove the "profile-after-change" observer to an observer on the matchOS pref

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: WIP-5

Comment 10

[Benjamin Smedberg]

Comment on attachment 407627 [details] [diff] [review]
WIP-5

I think you should register the SELECTED_LOCALE_PREF observer in nsChromeRegistry::Init in the same place you register the MATCH_OS and SELECTED_SKIN observers. Then in ::Observe, just call SelectLocalePref whenever matchOS or the selected locale changes.

And this needs an xpcshell test.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

all the selected locale change and matchOS are handle through SelectLocalePref now.
There is no more any removeObserver/addObserver calls into it, which means we are going to enter and refresh mSelectedLocale every time the selected locale pref change, even if we are into a 'matchOS mode'.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

canceling review because I suspect the patch to be wrong

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2 + xpcshell test

Add the xpcshell test

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

+ don' call FlushAllCache at startup

Comment 15

[Benjamin Smedberg]

Comment on attachment 408869 [details] [diff] [review]
Patch v0.2 + xpcshell test

>diff --git a/chrome/src/nsChromeRegistry.cpp b/chrome/src/nsChromeRegistry.cpp

>+nsresult
>+nsChromeRegistry::SelectLocalePref(nsIPrefBranch* prefs)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIPrefBranch2> prefs2 (do_QueryInterface(prefs));

Remove prefs2, you're not using it any more.

>+  PRBool matchOSLocale = PR_FALSE;
>+  rv = prefs->GetBoolPref(MATCH_OS_LOCALE_PREF, &matchOSLocale);
>+
>+  if (NS_SUCCEEDED(rv) && matchOSLocale) {
>+    // compute lang and region code only when needed!
>+    nsCAutoString uiLocale;
>+    rv = getUILangCountry(uiLocale);
>+    if (NS_SUCCEEDED(rv))
>+      mSelectedLocale = uiLocale;

You never call FlushAllCaches in this block. mSelectedLocale will be correct but the chrome caches won't reflect it.
  
>+  else {
>+    nsXPIDLCString provider;
>+    rv = prefs->GetCharPref(SELECTED_LOCALE_PREF, getter_Copies(provider));
>+    if (NS_SUCCEEDED(rv)) {
>+      mSelectedLocale = provider;
>+      
>+      if (!mInitialized)
>+        FlushAllCaches();

If *not* mInitialized? Shouldn't it be the other way, we flush caches if we *are* initialized? Isn't what you really mean "if the user changes the pref after the profile has been initialized". If that's what you mean, you should use a profile-initial-state observer and only call FlushAllCaches once that notification has been received.

Although I see now that we don't actually send any profile-initial-state notifications. We should do that here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#816

>diff --git a/chrome/test/unit/data/test_bug519468.manifest b/chrome/test/unit/data/test_bug519468.manifest

>+locale global en-US jar:en-US.jar!/locale/en-US/global/
>+locale global fr-FR jar:en-US.jar!/locale/en-US/global/

I think you'd be better off using a dummy packagename rather than "global", since there are various services in mozilla which use global locale stuff to initialize error messages and might get confused here.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

(In reply to comment #15)
> (From update of attachment 408869 [details] [diff] [review])
> >diff --git a/chrome/src/nsChromeRegistry.cpp b/chrome/src/nsChromeRegistry.cpp
> 
> >+nsresult
> >+nsChromeRegistry::SelectLocalePref(nsIPrefBranch* prefs)
> >+{
> >+  nsresult rv;
> >+  nsCOMPtr<nsIPrefBranch2> prefs2 (do_QueryInterface(prefs));
> 
> Remove prefs2, you're not using it any more.

Done.


> 
> >+  PRBool matchOSLocale = PR_FALSE;
> >+  rv = prefs->GetBoolPref(MATCH_OS_LOCALE_PREF, &matchOSLocale);
> >+
> >+  if (NS_SUCCEEDED(rv) && matchOSLocale) {
> >+    // compute lang and region code only when needed!
> >+    nsCAutoString uiLocale;
> >+    rv = getUILangCountry(uiLocale);
> >+    if (NS_SUCCEEDED(rv))
> >+      mSelectedLocale = uiLocale;
> 
> You never call FlushAllCaches in this block. mSelectedLocale will be correct
> but the chrome caches won't reflect it.

I've move the FlushAllCaches call in the observer method.

> >+  else {
> >+    nsXPIDLCString provider;
> >+    rv = prefs->GetCharPref(SELECTED_LOCALE_PREF, getter_Copies(provider));
> >+    if (NS_SUCCEEDED(rv)) {
> >+      mSelectedLocale = provider;
> >+      
> >+      if (!mInitialized)
> >+        FlushAllCaches();
> 
> If *not* mInitialized? Shouldn't it be the other way, we flush caches if we
> *are* initialized? Isn't what you really mean "if the user changes the pref
> after the profile has been initialized". If that's what you mean, you should
> use a profile-initial-state observer and only call FlushAllCaches once that
> notification has been received.

my bad, you're perfectly right.

> Although I see now that we don't actually send any profile-initial-state
> notifications. We should do that here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#816


Done.

> >diff --git a/chrome/test/unit/data/test_bug519468.manifest b/chrome/test/unit/data/test_bug519468.manifest
> 
> >+locale global en-US jar:en-US.jar!/locale/en-US/global/
> >+locale global fr-FR jar:en-US.jar!/locale/en-US/global/
> 
> I think you'd be better off using a dummy packagename rather than "global",
> since there are various services in mozilla which use global locale stuff to
> initialize error messages and might get confused here.

I have replace 'global' by 'testmatchos'

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

passed tests on try server

pushed:
http://hg.mozilla.org/mozilla-central/rev/4e4a74c88e7d

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 409643 [details] [diff] [review]
Patch v0.3

Needed for Fennec beta 5

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

m-192 pushed:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/78a164803038

Comment 20

[Axel Hecht [:Pike]]

I might be seeing a regression of this in my n810 fennec nightly. It insists on coming up in German, even though my OS locale is set to en-US. That's a 

Moailla/5.0 (X11; U; Linux armv6l; de; rv:1.9.2b2pre) Gecko/20091103

about:buildconfig references http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bae033362017.

Comment 21

[Aakash Desai [:aakashd]]

We definitely need this in our BFT's and Smoketests on litmus.

Comment 22

[Aakash Desai [:aakashd]]

Axel, works for me on francais and English (en-US), so verified FIXED with en-US:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091104 Firefox/3.6b2pre Fennec/1.0b5

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091104 Firefox/3.7a1pre Fennec/1.0b5


If you see any new issues, go ahead an file a new bug.

Comment 23

[Ben Bucksch (:BenB)]

This fails for me on a German system:
TEST-UNEXPECTED-FAIL | .../mozilla/_tests/xpcshell/test_chrome/unit/test_bug519468.js | en-US == de-DE

Comment 25

[Ben Bucksch (:BenB)]

Filed bug 557024 about the test failure.