Closed Bug 642203 Opened 9 years ago Closed 9 years ago

Must treat locale pref as localized in multi-locale builds

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Attachment #519718 - Flags: review?(dtownsend)
Green on "Rev3 Fedora 12 tryserver opt test xpcshell"
Attachment #519718 - Flags: review?(blassey.bugs)
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.
Attachment #519718 - Flags: review?(dtownsend) → review+
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
Assignee: nobody → mark.finkle
Attached patch patch 2 (obsolete) — Splinter Review
Addressed Dave's comments and made the UTF8 / UTF16 changes suggested by Brad.
Attachment #519718 - Attachment is obsolete: true
Attachment #519718 - Flags: review?(blassey.bugs)
Attachment #519771 - Flags: review?(blassey.bugs)
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
Attachment #519771 - Flags: review?(blassey.bugs) → review-
Attached patch patch 3 (obsolete) — Splinter Review
With changes
Attachment #519771 - Attachment is obsolete: true
Attachment #519777 - Flags: review?(blassey.bugs)
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)
Attachment #519777 - Flags: review?(blassey.bugs) → review+
Testing hows it breaks the Android behavior. Debugging...
Attached patch patch (obsolete) — Splinter Review
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
Attachment #519777 - Attachment is obsolete: true
Attachment #519795 - Flags: review?(doug.turner)
Attached patch patchSplinter Review
small ws cleanup
Attachment #519795 - Attachment is obsolete: true
Attachment #519795 - Flags: review?(doug.turner)
Attachment #519798 - Flags: review?(doug.turner)
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?
(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 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.
Attachment #519798 - Flags: review?(doug.turner) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/e273946b74c8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Mark, how can i verify this patch is fixed on the nightly?
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.
Depends on: 657067
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.