1349106 - LocaleService::GetAppLocalesAsBCP47() always returns ["en-US"] if called from content side

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Jessica Jong [:jessica] (inactive)]

I have local-built Firefox with zh-TW lang pack, the browser UI is in traditional chinese, but calling LocaleService::GetAppLocalesAsBCP47() from XBL binding in content process always returns ["en-US"]. While calling from chrome process, it returns ["zh-TW", "en-US"].

I remember this used to work, so I revert my code base back to this rev:

changeset:   343912:711456564e1e
user:        Jessica Jong <jjong>
date:        Sun Feb 19 22:11:00 2017 -0500
summary:     Bug 1340396 - Add window.getAppLocales() for ChromeOrXBL. r=smaug

and it worked fine, so I guess this is a regression?

Comment 1

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

I'm surprised.

Are you saying that with rev 711456564e1e you were able to install langpack and get getAppLocales to return it?

The root cause of the bug is 1344355 comment 5, and I didn't fix it yet.

There are two ways to fix it - we can either:

a) Stop caching mAppLocale in LocaleService (as described in bug 1344355 comment 0)
b) Fix the underlying order of operations. (bug 1344355 comment 10 is a lead)

Unfortunately, the status is that we don't support langpacks very well now.

If you need it locally, I'd suggest either building Firefox with zh-TW locale[0] or removing the mAppLocale cache from LocaleService.

I'm working on the fix for the ordering and hope to have something soon.

I'll keep this bug open because if it did work, then maybe there indeed is a regression, but I have no idea how it happened.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Creating_a_language_pack#L10n_binary_repack

Comment 2

[Jessica Jong [:jessica] (inactive)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> I'm surprised.
> 
> Are you saying that with rev 711456564e1e you were able to install langpack
> and get getAppLocales to return it?

To be precise, I locally built Firefox with the following build options:
  ac_add_options --enable-ui-locale=zh-TW
  ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)

then window.getAppLocales() returns ["zh-TW", "en-US"].
I'm not sure if this is the same as "install langpack".

> 
> The root cause of the bug is 1344355 comment 5, and I didn't fix it yet.
> 
> There are two ways to fix it - we can either:
> 
> a) Stop caching mAppLocale in LocaleService (as described in bug 1344355
> comment 0)
> b) Fix the underlying order of operations. (bug 1344355 comment 10 is a lead)
> 
> Unfortunately, the status is that we don't support langpacks very well now.
> 
> If you need it locally, I'd suggest either building Firefox with zh-TW
> locale[0] or removing the mAppLocale cache from LocaleService.
> 
> I'm working on the fix for the ordering and hope to have something soon.
> 
> I'll keep this bug open because if it did work, then maybe there indeed is a
> regression, but I have no idea how it happened.
> 
> [0]
> https://developer.mozilla.org/en-US/docs/Mozilla/
> Creating_a_language_pack#L10n_binary_repack

I see that LocaleService hasn't changed much (besides from adding the BCP47, which I don't think it's related) , and neither did ChromeRegistry code, so I don't how it happened either. :(

I can workaround on local testing, but was just trying to find out why input type=time l10n is still not working on nightly builds after bug 1301312 was landed.

Comment 3

[Jessica Jong [:jessica] (inactive)]

I did a small experiment by calling `LocaleService::GetInstance()->Refresh();` before `LocaleService::GetInstance()->GetAppLocalesAsBCP47()` and it's returning the right results: ["zh-TW","en-US"]. So I guess it's the same as bug 1344355.

Comment 4

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

Yeah, so building zh-TW Firefox is different from installing a langpack.

The former will work fine, the latter is quirky. I'm fixing it now :)

Comment 5

[Jessica Jong [:jessica] (inactive)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> Yeah, so building zh-TW Firefox is different from installing a langpack.
> 
> The former will work fine, the latter is quirky. I'm fixing it now :)

Thanks for fixing it! I supposed fixing the case from installing a langpac will also fix the case of building Firefox with a specific locale.

Comment 6

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

Building specific locale should work now. If you build "zh-TW" Firefox, you should get it in GetAppLocales. It's just if you install zh-TW as langpack where it may not get registered in time :( The patch for this is in bug 1347306.

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> Building specific locale should work now. If you build "zh-TW" Firefox, you
> should get it in GetAppLocales. It's just if you install zh-TW as langpack
> where it may not get registered in time :( The patch for this is in bug
> 1347306.

Sorry, I am little confused, since this bug is about building with specific locale, see comment 2. Or has it been fixed recently in another bug?

Comment 8

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

In comment 2, you said:

"
To be precise, I locally built Firefox with the following build options:
  ac_add_options --enable-ui-locale=zh-TW
  ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)

then window.getAppLocales() returns ["zh-TW", "en-US"].
"

This should work at the moment.

But if you build "en-US" Firefox and then install "zh-TW" langpack, it may not.

Does it match your experience?

Comment 9

[Jessica Jong [:jessica] (inactive)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> In comment 2, you said:
> 
> "
> To be precise, I locally built Firefox with the following build options:
>   ac_add_options --enable-ui-locale=zh-TW
>   ac_add_options --with-l10n-base=(path to the lang pack downloaded from
> https://hg.mozilla.org/l10n-central/)
> 
> then window.getAppLocales() returns ["zh-TW", "en-US"].
> "
> 
> This should work at the moment.
> 
> But if you build "en-US" Firefox and then install "zh-TW" langpack, it may
> not.
> 
> Does it match your experience?

No :(, let me clarify a bit:

With the current code base, building with:
   ac_add_options --enable-ui-locale=zh-TW
   ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)
window.getAppLocalesAsBCP47() returns ["en-US"].

But after rolling back to rev 711456564e1e and building with:
   ac_add_options --enable-ui-locale=zh-TW
   ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)
window.getAppLocales() returns ["zh-TW", "en-US"].

With the current code base, building with:
   ac_add_options --enable-ui-locale=zh-TW
   ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)
and by changing the implementation of window.getAppLocalesAsBCP47() by calling LocaleService::GetInstance()->Refresh() before LocaleService::GetInstance()->GetAppLocalesAsBCP47(), then window.getAppLocalesAsBCP47() returns ["zh-TW", "en-US"].

Comment 10

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

Thanks, 

I'll investigate tomorrow!

Comment 11

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

Ok, :jessica, I need your help here.

I followed your steps on today nightly:

1) Cloned https://hg.mozilla.org/l10n-central/pl/
2) Built Firefox with:
   ac_add_options --enable-ui-locale=pl
   ac_add_options --with-l10n-base=(path to the lang pack downloaded from https://hg.mozilla.org/l10n-central/)

The first thing that happened was that the browser didn't build because of missing files in "zh-TW" localization.

3) So I copied a bunch of en-US files to zh-TW:
 - cp ./toolkit/locales/en-US/chrome/global/datetimebox.dtd ../l10n-central/zh-TW/toolkit/chrome/global/
 - cp ./browser/locales/en-US/chrome/browser/safebrowsing/safebrowsing.properties ../l10n-central/zh-TW/browser/chrome/browser/safebrowsing

4) With those changes I was able to ./mach build
5) Next I tried to `./mach run` which failed with yellow screen of death
6) So I copied full browser.dtd from en-US to zh-TW:
 - cp browser/locales/en-US/chrome/browser/browser.dtd ../l10n-central/zh-TW/browser/chrome/browser/browser.dtd
7) Again, ./mach build
8) `./mach run`
9) ctrl+shift+j to open browser console (browser UI is in zh-TW)
10) Services.locale.getAppLocalesAsBCP47(); // Array [ "zh-TW", "en-US" ]

So, here's my question. In your description you didn't mention all the troubles that come with building a non en-US locale on mozilla-central coming from missing translations. Which in turn makes me a bit suspicious that maybe you did something differently and in result got different outcome.

Can you verify please?

Comment 12

[Jessica Jong [:jessica] (inactive)]

Basically, I did similar steps to get my build succesful. Whenever I find something missing, I just copied from trunk.

From step "10) Services.locale.getAppLocalesAsBCP47(); // Array [ "zh-TW", "en-US" ]", it seems that you are calling from chrome side, since you need to `Cu.import("resource://gre/modules/Services.jsm");` for Services. Have you tried calling from content side? Maybe you can just verify what date/time XBL binding gets.

Comment 13

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

Ahh, ok. Good. The mystery is gone!

Yes, unfortunately we currently don't have a good way to get app locale from content process :( I have a potential fix in bug 1347306.

And yes, I can now see why it worked for you before a certain point. Basically we used to take ChromeRegistry locale (which we have in the content process) as the browser locale. Which is not good.
Now we negotiate locale, but we can't in the content process get: a) all available locales, and b) requested locales.

So basically, I'd say that if you want to use the app locale in the content process, you should pass it from chrome process for now :(

I'll keep this one open and assigned to myself.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Thanks for the clarification.

Some more questions I have are:
Why does calling LocaleService->Refresh() before calling LocaleService->GetAppLocalesAsBCP47() works? do you think we can workaround this way first?
And do you know when will this be fixed for content process?

Thanks again!

Comment 15

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

> Why does calling LocaleService->Refresh() before calling LocaleService->GetAppLocalesAsBCP47() works?

I have no idea! Let me investigate :)

> And do you know when will this be fixed for content process?

I'd expect to land that within the next 2 weeks.

Comment 16

[Jessica Jong [:jessica] (inactive)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> > Why does calling LocaleService->Refresh() before calling LocaleService->GetAppLocalesAsBCP47() works?
> 
> I have no idea! Let me investigate :)
> 
> > And do you know when will this be fixed for content process?
> 
> I'd expect to land that within the next 2 weeks.

Noted. Thanks for fixing this!

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Looks like this is QA testing blocker for it impacts the l10n/RTL testing on time/date input.
Is it ok that I set it to P1?

Comment 18

[Makoto Kato [:m_kato]]

Wesley, although it is OK to P1, when do you ship input=date to release build?

Comment 19

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Makoto Kato [:m_kato] from comment #18)
> Wesley, although it is OK to P1, when do you ship input=date to release
> build?

The goal is to release in 57, but we will need a full QA cycle starting from May (during the nightly 55 and 56).

Comment 20

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

I just posted the first patch for review in bug 1348042. This one will get us a cross-process communication for locales.

Now, unfortunately, the JS contexts are created before we can do that so I need another patch to make JS contexts react to intl:app-locales-changed. I'll file a bug for that.

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Zibi,
Do you have an ETA for this one? 
I'm asking so that QA can better allocate their testing time slots for "input type = time" l10n function tests.

Comment 22

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

I have a green try in bug 1348042 and am waiting for a re-review from :qdot which should happen today.

My hope is that this will bu fixed today or tomorrow.

Comment 23

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #22)
> I have a green try in bug 1348042 and am waiting for a re-review from :qdot
> which should happen today.
> 
> My hope is that this will bu fixed today or tomorrow.

nice. I just saw it's landing.

Comment 24

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

Jessica, can you verify that this is fixed now for your use case, please?

Comment 25

[Jessica Jong [:jessica] (inactive)]

It's working now for my use case (building with specific locale)! Thanks for fxiing it.
I'll try tomorrow's official nightly.

Comment 26

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

Great to hear! Marking this as fixed. Please, reopen if needed.