1360596 - Gecko-side localisation (e.g. context menu) broken in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Locale switching and selection, defect, P1)

Description

[Jan Henning [:JanH]]

+++ This bug was initially created as a clone of Bug #1351873 +++

[Tracking Requested - why for this release]:
Gecko localisation in Fennec is broken again, with everything not part of the Java UI being displayed in English.

Regression range is between 2017-04-25 and 2017-04-26 (https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a30dc237c3a600a5231f2974fc2b85dfb5513414&tochange=0f5ba06c4c5959030a05cb852656d854065e2226)

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is likely from bug 1348042. Wasn't sure if this is covered by bug 1357240.

Comment 2

[Marcia Knous [:marcia]]

Tracking 55+ for this breakage.

Comment 3

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

Reproduced, taking.

Comment 4

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

So, the reason we see this regression is that we use "global" package for retrieving available locales in Gecko, but in Fennec, we only have en-US for it.

The only two packages that we have for Fennec that retrieve locales are "browser" and "branding".

When I change "global" to "browser" in:
1) http://searchfox.org/mozilla-central/source/intl/locale/LocaleService.cpp#120
2) http://searchfox.org/mozilla-central/source/chrome/nsChromeRegistryChrome.cpp#740

the regression disappears.

:pike, :rnewman, should I special-case Fennec?

Comment 5

[Axel Hecht [:Pike]]

We have http://searchfox.org/mozilla-central/search?q=symbol:_ZN22nsChromeRegistryChrome21OverrideLocalePackageERK10nsACStringRS0_&redirect=false for this specifically, so you should make sure that it's still called in your code-path.

Comment 6

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

Attachment: Bug 1360596 - Test against overriden locale packages for available locales changed.

In Fennec scenario, the package registration we should be monitoring for is different than "global".
OverrideLocalePackage handles the package overrides and gives us the right package name.

Review commit: https://reviewboard.mozilla.org/r/136270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136270/

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8864588 [details]
Bug 1360596 - Test against overriden locale packages for available locales changed.

https://reviewboard.mozilla.org/r/136270/#review139392

::: chrome/nsChromeRegistryChrome.cpp:741
(Diff revision 1)
>                                    mSelectedSkin);
>      SendManifestEntry(chromePackage);
>    }
>  
> -  if (strcmp(package, "global") == 0) {
> +  nsAutoCString realpkg;
> +  nsAutoCString pkg(package);

Should use nsDependentCString for `pkg` here (avoids actually copying the string to a new buffer).

Oh, but we already have such a wrapper above: `packageName`. So just use that instead.

::: chrome/nsChromeRegistryChrome.cpp:744
(Diff revision 1)
>  
> -  if (strcmp(package, "global") == 0) {
> +  nsAutoCString realpkg;
> +  nsAutoCString pkg(package);
> +  nsresult rv = OverrideLocalePackage(pkg, realpkg);
> +  if (NS_FAILED(rv))
> +    return;

style nit: braces even for a single-line `if` body.

::: chrome/nsChromeRegistryChrome.cpp:746
(Diff revision 1)
> +  nsAutoCString pkg(package);
> +  nsresult rv = OverrideLocalePackage(pkg, realpkg);
> +  if (NS_FAILED(rv))
> +    return;
> +
> +  if (realpkg.Equals("global")) {

If I understand the comments above, this probably won't ever match on fennec because OverrideLocalePackage will give us a different name. So should we have some kind of equivalent test for what fennec actually uses?

Comment 8

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

> If I understand the comments above, this probably won't ever match on fennec because OverrideLocalePackage will give us a different name

omg, yeah, I mixed up this in the patch. It should actually just take OverrideLocalePackage for "global" and check that against package name.

I'll update the patch.

Comment 9

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

Comment on attachment 8864588 [details]
Bug 1360596 - Test against overriden locale packages for available locales changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/136270/diff/1-2/

Comment 10

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

Hah, the reason it worked in my tests is that if you don't report any updates to locales, Gecko on Fennec is loaded late enough that all locales are reported properly.

But that won't work for Gecko of course, so now I just take the "mainPackage" which will be "global" in most cases, and "browser" in Fennec.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8864588 [details]
Bug 1360596 - Test against overriden locale packages for available locales changed.

https://reviewboard.mozilla.org/r/136270/#review139578

Looks fine, AFAICS.

However, for bonus points: notice that OverrideLocalePackage is a private method in nsChromeRegistryChrome, and always returns NS_OK; there are no error cases. So the return value (and subsequently checking it) is completely redundant.

I guess it's possible the optimizer figures this out -- especially if it decides to inline OverrideLocalePackage, it could detect that `rv` is a known constant and testing it is redundant -- but we should just simplify the source here.

So I'd happily accept a followup patch to change the return type of OverrideLocalePackage to void, and remove the `if (NS_FAILED(...))` checks at all the call sites, so the existing cases that read:

    nsCString realpackage;
    nsresult rv = OverrideLocalePackage(aPackage, realpackage);
    if (NS_FAILED(rv))
      return rv;

will be simplified to:

    nsAutoCString realpackage;
    OverrideLocalePackage(aPackage, realpackage);

(also switching them to nsAutoCString while we're in the area, as I assume package names aren't likely to be hugely long).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aab0dfdae32f
Test against overriden locale packages for available locales changed. r=jfkthame

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aab0dfdae32f