Closed Bug 1379905 Opened 7 years ago Closed 7 years ago

Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales

Categories

(Core :: Internationalization, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Once bug 1379420 lands, we should switch mozIntl and intl/locale/DateTimeFormat to use it.
Blocks: 1366134
Depends on: 1379420
Priority: -- → P3
Depends on: 1380916
Summary: Switch mozIntl and DateTimeFormat to use GetRegionalPrefsLocales → Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8886724 [details]
Bug 1379905 - Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/157522/#review165662

LGTM, modulo a few comment fixups etc as noted below. However, as this touches a .webidl file, I think you need to get a DOM peer to rubber-stamp it as well before it lands.

::: dom/base/nsGlobalWindow.cpp:15498
(Diff revision 2)
>  }
>  
>  void
> -nsGlobalWindow::GetAppLocalesAsBCP47(nsTArray<nsString>& aLocales)
> +nsGlobalWindow::GetRegionalPrefsLocales(nsTArray<nsString>& aLocales)
>  {
> -  nsTArray<nsCString> appLocales;
> +  nsTArray<nsCString> rpLocales;

While you're touching this, it would make sense for `rpLocales` to be an `AutoTArray<nsCString,10>` to avoid a heap allocation (in the presumably normal case when the list isn't very long).

::: dom/base/nsGlobalWindow.cpp:15501
(Diff revision 2)
> -  for (uint32_t i = 0; i < appLocales.Length(); i++) {
> -    aLocales.AppendElement(NS_ConvertUTF8toUTF16(appLocales[i]));
> +  for (uint32_t i = 0; i < rpLocales.Length(); i++) {
> +    aLocales.AppendElement(NS_ConvertUTF8toUTF16(rpLocales[i]));
>    }

And for added modern-C++ goodness, this could be rewritten as:

    for (const auto& loc : rpLocales) {
      aLocales.AppendElement(NS_ConvertUTF8toUTF16(loc));
    }

::: dom/tests/mochitest/chrome/test_intlUtils_getLocaleInfo.html:47
(Diff revision 2)
>      expected: {
>        locale: "ar",
>        direction: "rtl",
>      }
>    },
>    // IntlUtils uses current app locales if locales is not provided.

s/app/regional pref/

::: dom/tests/mochitest/chrome/test_window_getRegionalPrefsLocales.html:19
(Diff revision 2)
>  <div id="content" style="display: none">
>  <script>
>  
> -let appLocales = window.getAppLocalesAsBCP47();
> -ok(appLocales.length > 0, "getAppLocales returns at least one locale.");
> -is(appLocales[0], "en-US", "The first app locale should be en-US.");
> +let rpLocales = window.getRegionalPrefsLocales();
> +ok(rpLocales.length > 0, "getRegionalPrefsLocales returns at least one locale.");
> +is(rpLocales[0], "en-US", "The first app locale should be en-US.");

s/app/regional pref/

::: toolkit/components/mozintl/mozIntl.js:18
(Diff revision 2)
>   * This helper function retrives currently used app locales, allowing
>   * all mozIntl APIs to use the current app locales unless called with
>   * explicitly listed locales.

The comment here should be updated to match the new behavior implemented by getLocales() -- it now returns regional prefs locales, not app locales.
Attachment #8886724 - Flags: review?(jfkthame) → review+
Comment on attachment 8886724 [details]
Bug 1379905 - Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales.

Thanks Jonathan!

Olli, you reviewed bug 1340396 and here I'm renaming it to follow the new distinction in LocaleService between languages use to localize Firefox from languages used to localize date/time etc.

Can you rubber stamp it please?
Attachment #8886724 - Flags: review?(bugs)
Do I need to review .webidl change only?
As per:
> However, as this touches a .webidl file, I think you need to get a DOM peer to rubber-stamp it as well before it lands.

from my reviewer.

yes pls!
Comment on attachment 8886724 [details]
Bug 1379905 - Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/157522/#review165786

::: dom/webidl/Window.webidl:533
(Diff revision 3)
>     * This API always returns at least one locale.
>     *
>     * Example: ["en-US", "de", "pl", "sr-Cyrl", "zh-Hans-HK"]
>     */
>    [Func="IsChromeOrXBL"]
> -  sequence<DOMString> getAppLocalesAsBCP47();
> +  sequence<DOMString> getRegionalPrefsLocales();

Not sure this method name is too clear, especially the "prefs" part. Perhaps the comment could somehow clarify it.
Attachment #8886724 - Flags: review?(bugs) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bfdd14469c99
Switch mozIntl, datetimebox and DateTimeFormat to use GetRegionalPrefsLocales. r=jfkthame,smaug
Zibi, can you explain what this patch does? mozIntl will now completely follow the chosen regional settings so an en-US version will again display German dates/times as some people wanted? But Intl.DateTimeFormat and toLocale[Date|Time]String(), so ECMA-402, will still follow the app locale? Or is everything going to follow the OS settings again? In which case mozIntl wouldn't be necessary any more? I'm slightly confused.

And we need a preference for that in bug 1379910, no?
(In reply to Jorg K (GMT+2) from comment #11)
> Zibi, can you explain what this patch does? mozIntl will now completely
> follow the chosen regional settings so an en-US version will again display
> German dates/times as some people wanted?

It's a simplified version of that.
Depending on the pref, we will now either use App locales or OS locales for regional preferences.

In both cases we'll still also look into regional prefs from the OS and if the locale matches, use them. If the pref is set to get the OS locale, then, naturally, the locale will always match.

So yes, with this pref on, the "de" Windows and "en-US" Firefox should display "de" date formats.

> But Intl.DateTimeFormat and
> toLocale[Date|Time]String(), so ECMA-402, will still follow the app locale?

By default (without locale parameter), yes. Currently, SpiderMonkey has only one "defualtLocale" and it follows appLocale. If we'll want ECMA402 to follow regionalPrefsLocale, I'd suggest adding another locale variable to SpiderMonkey and using it for regional settings related APIs.

So, RegionalPrefsLocales is going to be used by Chrome APIs (mozIntl, DateTimeFormat.cpp) and affect Gecko App UI.

> And we need a preference for that in bug 1379910, no?

Yes, although I'm not sure if that's a high priority for Firefox. You may want to add it for Thunderbird, or even consider changing the defaults (I'd not do that, but it's not my call) for Thunderbird.
Oh, the pref was already implemented here:
https://hg.mozilla.org/mozilla-central/rev/fe0297d33e87#l4.15
It's just not in the UI, that's bug 1379910. Now I understand.

I'll play around with it when this bug here lands. Filed bug 1384007 for TB work. Thanks Zibi for all your work and patience, much appreciated!
https://hg.mozilla.org/mozilla-central/rev/bfdd14469c99
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
For the record, it looks like this was auto-blamed for a ~6% PerfHerder regression in "Strings PerfStripCharsCRLF windows10-64 opt":
 https://treeherder.mozilla.org/perf.html#/alerts?id=8190

But the graph returned to the baseline value a few commits later, with a ~6% "improvement":
 https://treeherder.mozilla.org/perf.html#/alerts?id=8196

As I noted in bug 1349683 comment 14 (the bug credited with the supposed perf win), I suspect this was from tripping some arbitrary boundary in PGO or something semi-arbitrary like that.  It's not random noise -- the builds in the "bad range" seem reliably worse (on retriggers) than the surrounding "good range" builds, in the PerfHerder graph -- but thankfully we seem to have returned to the baseline "good" value, so we don't have to worry about it too much.
Thanks Daniel!

I'd also attribute it to something arbitrary. All this patch does, in all test runs, is add a single C++ function that calls the one used previously.
The only thing that could cost is the pref read, which I expect to be cached.
You need to log in before you can comment on or make changes to this bug.