1409185 - Generalize language-matching for date/time patterns in OSPreferences

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

Currently, Windows code has a special code that allows us to look into OS settings for a given locale if language part matches between OS regional prefs locale and appLocale.

We can generalize this code to make it work on all platforms making Firefox in en-US use en-GB dates on Mac and Linux.

Comment 1

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

Attachment: Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

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

Comment 2

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

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

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

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

https://reviewboard.mozilla.org/r/189966/#review195884

::: intl/locale/LocaleService.cpp:251
(Diff revision 2)
> +  // This facilitates scenarios such as Firefox in "en-US" and User sets
> +  // regional prefs to "en-GB".
> +  nsAutoCString appLocale;
> +  AutoTArray<nsCString, 10> regionalPrefsLocales;
> +  LocaleService::GetInstance()->GetAppLocaleAsBCP47(appLocale);
> +  OSPreferences::GetInstance()->GetRegionalPrefsLocales(regionalPrefsLocales);

OSPreferences::GetRegionalPrefsLocales() can fail and return an empty list, AFAICS; therefore, we need to check its return value and not just assume we can safely look at regionalPrefsLocales[0] below.

::: intl/locale/windows/OSPreferences_win.cpp:99
(Diff revision 2)
>  {
> -  nsAutoCString reqLocale;
> +  UTF8ToUnicodeBuffer(aLocale, (char16_t*)aBuffer);

The GetWindowsLocaleFor() function is no longer doing enough to justify its existence. :) Just do the UTF8ToUnicodeBuffer operation directly at the call site.

Comment 4

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

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

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

Comment 5

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

Thanks Jonathan! :)

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

https://reviewboard.mozilla.org/r/189966/#review196044

::: intl/locale/windows/OSPreferences_win.cpp:119
(Diff revision 3)
>  bool
>  OSPreferences::ReadDateTimePattern(DateTimeFormatStyle aDateStyle,
>                                     DateTimeFormatStyle aTimeStyle,
>                                     const nsACString& aLocale, nsAString& aRetVal)
>  {
> -  WCHAR buffer[LOCALE_NAME_MAX_LENGTH];
> +  LPWSTR localeName;

No, the buffer still needs to be declared here, otherwise `localeName` is just an uninitialized pointer to....somewhere off in the weeds. Not a good place to write the Unicode data into!

So you want something more like

    WCHAR localeName[LOCALE_NAME_MAX_LENGTH];
    UTF8ToUnicodeBuffer(aLocale, (char16_t*)localeName);

so that there's actually some storage backing `localeName`.

(I'm not entirely sure if the cast is needed there; the compiler can tell you.)

Comment 7

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

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189966/diff/3-4/

Comment 8

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

> (I'm not entirely sure if the cast is needed there; the compiler can tell you.)

the compiler has spoken - it fancies the cast very much.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

https://reviewboard.mozilla.org/r/189966/#review197200

::: intl/locale/LocaleService.cpp:256
(Diff revision 4)
> +
> +

Two blank lines are excessive, please drop one of them.

Comment 10

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

Comment on attachment 8919052 [details]
Bug 1409185 - Generalize language-matching for date/time patterns in OSPreferences.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189966/diff/4-5/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/016ca1f0bbbc
Generalize language-matching for date/time patterns in OSPreferences. r=jfkthame

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/016ca1f0bbbc