642203 - Must treat locale pref as localized in multi-locale builds

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: patch

There is code in the platform that assumes "general.useragent.locale" is always a simple char preference. This is mostly the case, except for Fennec multi-locale builds where it is a localized pref, read from the intl.properties file.

Here is my first cut patch. I am sending to try server too.

Comment 1

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

Green on "Rev3 Fedora 12 tryserver opt test xpcshell"

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 519718 [details] [diff] [review]
patch

>   /**
>+   * Gets a complex preference.
>+   *
>+   * @param  aName
>+   *         The name of the preference
>+   * @param  aDefaultValue
>+   *         A value to return if the preference does not exist
>+   * @param  aType
>+   *         The interface type of the preference

Swap these so they match the actual order.

>+   * @return the value of the preference or fallback to simple char preference
>+   *         if there is none
>+   */
>+  getComplexPref: function(aName, aType, aDefaultValue) {

>diff --git a/toolkit/mozapps/extensions/nsBlocklistService.js b/toolkit/mozapps/extensions/nsBlocklistService.js
>--- a/toolkit/mozapps/extensions/nsBlocklistService.js
>+++ b/toolkit/mozapps/extensions/nsBlocklistService.js
>@@ -241,17 +241,17 @@ function matchesOSABI(blocklistElement) 
>  * Gets the current value of the locale.  It's possible for this preference to
>  * be localized, so we have to do a little extra work here.  Similar code
>  * exists in nsHttpHandler.cpp when building the UA string.
>  */
> function getLocale() {
>   try {
>       // Get the default branch
>       var defaultPrefs = gPref.getDefaultBranch(null);
>-      return defaultPrefs.getCharPref(PREF_GENERAL_USERAGENT_LOCALE);
>+      return defaultPrefs.getComplexPref(PREF_GENERAL_USERAGENT_LOCALE, Ci.nsIPrefLocalizedString).data;

Wrap this line so it isn't so long.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 519718 [details] [diff] [review]
patch

in AndroidBridge::SetSelectedLocale(), we convert the locale from UTF8 to UTF16. Since you're getting your complex pref as a UTF16, I'd rather you change SetSelectedLocale() to take a UTF16 and only do a UTF8 to UTF16 conversion for the simple char pref case in nsAppShell.cpp

Comment 4

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

Attachment: patch 2

Addressed Dave's comments and made the UTF8 / UTF16 changes suggested by Brad.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 519771 [details] [diff] [review]
patch 2

>+    branch->GetComplexValue("general.useragent.locale",
>+                            NS_GET_IID(nsIPrefLocalizedString),
>+                            getter_AddRefs(pls));
>+    if (pls) {
>+        nsXPIDLString uval;
>+        pls->ToString(getter_Copies(uval));
>+        if (uval)
>+            locale.Assign(uval);
>+    }

test for success here, so:

    nsresult rv = branch->GetComplexValue("general.useragent.locale",
                            NS_GET_IID(nsIPrefLocalizedString),
                            getter_AddRefs(pls));
    if (NS_SUCCEEDED(rv) && pls) {
        nsXPIDLString uval;

>+    else {
>+        nsXPIDLCString cval;
>+        branch->GetCharPref("general.useragent.locale", getter_Copies(cval));
>+        if (cval)
same here
>+            CopyUTF8toUTF16(cval, locale);
nit, use locale.AssignWithConversion


>     PRBool match = PR_FALSE;
>-    nsCString locale;
>     branch->GetBoolPref("intl.locale.matchOS", &match);
get match before you get the locale and skip getting the locale if match is true

>-    if (!match ||
>-        NS_FAILED(branch->GetCharPref("general.useragent.locale", getter_Copies(locale))))
>-        bridge->SetSelectedLocale(EmptyCString());
>+    if (!match || locale.IsEmpty())
>+        bridge->SetSelectedLocale(EmptyString());
>     else
>         bridge->SetSelectedLocale(locale);
>     return rv;
this can be simplified to bridge(match ? EmptyString() : locale); 
or you can return early above when match is true and just assume match is false if you've gotten this far.


same comments apply to the observe method. This is close, but I'd like to see another patch

Comment 6

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

Attachment: patch 3

With changes

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 519777 [details] [diff] [review]
patch 3

two small nits, please check the return value of the GetBoolPref (I know that's not your code) and you don't need to re-declare rv in every scope.

Also, let's test this before pushing (I'll post a multilocale build with your changes)

Comment 8

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

Testing hows it breaks the Android behavior. Debugging...

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

to help with the review, here's an inter-diff with mfinkle's patch (ignoring ws change since there's a whole block moved up an indentation level)
http://pastebin.mozilla.org/1171010

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

small ws cleanup

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 519795 [details] [diff] [review]
patch


>@@ -134,14 +135,36 @@ nsAppShell::Init()
>     NS_ENSURE_SUCCESS(rv, rv);
>     branch->AddObserver("intl.locale.matchOS", this, PR_FALSE);
>     branch->AddObserver("general.useragent.locale", this, PR_FALSE);
>+
>+    nsString locale;
>     PRBool match = PR_FALSE;
>-    nsCString locale;
>     branch->GetBoolPref("intl.locale.matchOS", &match);
>-    if (!match ||
>-        NS_FAILED(branch->GetCharPref("general.useragent.locale", getter_Copies(locale))))
>-        bridge->SetSelectedLocale(EmptyCString());
>-    else
>-        bridge->SetSelectedLocale(locale);
>+
>+    NS_ENSURE_SUCCESS(rv, rv);


What is this ensure for?


>+    
>+    if (match) {
>+        bridge->SetSelectedLocale(EmptyString());
>+        return NS_OK;
>+    }
>+    nsCOMPtr<nsIPrefLocalizedString> pls;
>+    rv = branch->GetComplexValue("general.useragent.locale",
>+                                 NS_GET_IID(nsIPrefLocalizedString),
>+                                 getter_AddRefs(pls));
>+    if (NS_SUCCEEDED(rv) && pls) {
>+        nsXPIDLString uval;
>+        pls->ToString(getter_Copies(uval));
>+        if (uval)
>+            locale.Assign(uval);
>+        }
>+    else {

brace style is wrong.  Please line up the }.


>         return nsBaseAppShell::Observe(aSubject, aTopic, aData);
>-    } else if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) && (
>-                   !wcscmp((const wchar_t*)aData, L"intl.locale.matchOS") ||
>-                   !wcscmp((const wchar_t*)aData, L"general.useragent.locale"))) {
>+    } else if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) && aData && (
>+                   nsDependentString(aData).Equals(
>+                       NS_LITERAL_STRING("general.useragent.locale")) ||
>+                   nsDependentString(aData).Equals(
>+                       NS_LITERAL_STRING("intl.locale.matchOS"))))
>+    {

I do not understand this change.  why doesn't wcscmp work here?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #11)
> Comment on attachment 519795 [details] [diff] [review]
> What is this ensure for?
the GetBoolPref was missing a rv = in front of it, fixed.

> brace style is wrong.  Please line up the }.
fixed

> I do not understand this change.  why doesn't wcscmp work here?

this is what was failing from mfinkle's change. I have theories as to why, but the point here is that it fails and nsDepenedentString.Equals() works

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 519798 [details] [diff] [review]
patch

please file a follow up on why wcscmp can not be used here.  i think we want to know the reason.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/e273946b74c8

Comment 15

[Tony Chung [:tchung]]

Mark, how can i verify this patch is fixed on the nightly?

Comment 16

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

The original reason we caught this bug was the metrics team finding bogus blocklist pings. Instaed of having a locale code, like "en-US" or "fr", the URL had "chrome://global/locale/intl.properties" in it.

You could turn on extension.logging.enabled = true and watch the error console for a blocklist ping.

Comment 17

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:8.0a1)Gecko/20110905
Firefox/8.0a1 Fennec/8.0a1
Device: Samsung Galaxy S
OS: Android 2.2