1379420 - Introduce LocaleService::GetRegionalPrefsLocales

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

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

In bug 1366134 we're trying to figure out a UX for dealing with scenarios when the OS locales doesn't match App locales and the user wants to use OS locales for regional preferences rather than App locales.

In this bug I'd like to add a new method on LocaleService that returns locales for regional preferences sensitive operations.
This method will use a preference to let the user chose between the two strategies.

Comment 1

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

Attachment: Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

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

Comment 2

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

Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

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

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/155464/#review160680

::: intl/locale/LocaleService.cpp:222
(Diff revision 2)
>      SanitizeForBCP47(locale);
>      aRetVal.AppendElement(locale);
>    }
>  }
>  
> +bool

This method never fails, so it doesn't need a `bool` return value, it can just be `void`.

Huh, wait a sec - it's declared as `void` in the header. Does this even compile?

::: intl/locale/LocaleService.cpp:225
(Diff revision 2)
>  }
>  
> +bool
> +LocaleService::GetRegionalPrefsLocales(nsTArray<nsCString>& aRetVal)
> +{
> +  bool useOSLocales = Preferences::GetBool("intl.regional_prefs.use_os_locales", false);

I think you should add this pref with its default value to all.js to make it easier for people to find/tweak.

What about exposing it in the browser UI -- is there any plan/decision regarding that?

::: intl/locale/LocaleService.cpp:231
(Diff revision 2)
> +
> +  if (useOSLocales && OSPreferences::GetInstance()->GetSystemLocales(aRetVal)) {
> +    return true;
> +  }
> +
> +  LocaleService::GetInstance()->GetAppLocalesAsBCP47(aRetVal);

Drop the `LocaleService::GetInstance()->` prefix, as this is just calling another method on the current LocaleService object.

::: intl/locale/LocaleService.cpp:635
(Diff revision 2)
> +{
> +  AutoTArray<nsCString,10> tempLocales;
> +  nsTArray<nsCString>* rgLocalesPtr;
> +
> +  GetRegionalPrefsLocales(tempLocales);
> +  rgLocalesPtr = &tempLocales;

This pointer doesn't seem necessary, AFAICS. (See below.)

(After ditching this, I'd also suggest renaming tempLocales as rgLocales.)

::: intl/locale/LocaleService.cpp:637
(Diff revision 2)
> +  nsTArray<nsCString>* rgLocalesPtr;
> +
> +  GetRegionalPrefsLocales(tempLocales);
> +  rgLocalesPtr = &tempLocales;
> +
> +  *aCount = rgLocalesPtr->Length();

`*aCount = tempLocales.Length();`

::: intl/locale/LocaleService.cpp:641
(Diff revision 2)
> +
> +  *aCount = rgLocalesPtr->Length();
> +  *aOutArray = static_cast<char**>(moz_xmalloc(*aCount * sizeof(char*)));
> +
> +  for (uint32_t i = 0; i < *aCount; i++) {
> +    (*aOutArray)[i] = moz_xstrdup((*rgLocalesPtr)[i].get());

`(*aOutArray)[i] = moz_xstrdup(tempLocales[i].get());`

Comment 4

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

Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

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

Comment 5

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

Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/155464/#review160680

> This method never fails, so it doesn't need a `bool` return value, it can just be `void`.
> 
> Huh, wait a sec - it's declared as `void` in the header. Does this even compile?

I switched it right before submitting the patch and didn't rebuild after that. My bad :) Switched to void.

> I think you should add this pref with its default value to all.js to make it easier for people to find/tweak.
> 
> What about exposing it in the browser UI -- is there any plan/decision regarding that?

I'd like to expose it in the Preferences UI in Firefox. I'd like to file a bug for it. But even without the UI it still would be useful to let advanced users seeking the behavior to fine-tune it for their needs.

Also, Thunderbird/SM would be able to provide the UI for it independently and their users seems to be more affected by this.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/155464/#review161180

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe0297d33e87
Introduce LocaleService::GetRegionalPrefsLocales. r=jfkthame

Comment 8

[Carsten Book [:Tomcat]]

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