Introduce LocaleService::GetRegionalPrefsLocales

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

(Blocks 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3
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());`
Attachment #8884579 - Flags: review?(jfkthame)
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 on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/155464/#review161180
Attachment #8884579 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe0297d33e87
Introduce LocaleService::GetRegionalPrefsLocales. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/fe0297d33e87
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.