Introduce LocaleService::GetRegionalPrefsLocales

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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)

Updated

a year ago
Blocks: 1366134
(Assignee)

Updated

a year ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Priority: -- → P3

Comment 3

a year ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Blocks: 1379905
(Assignee)

Updated

a year ago
Blocks: 1379910

Comment 6

a year ago
mozreview-review
Comment on attachment 8884579 [details]
Bug 1379420 - Introduce LocaleService::GetRegionalPrefsLocales.

https://reviewboard.mozilla.org/r/155464/#review161180
Attachment #8884579 - Flags: review?(jfkthame) → review+

Comment 7

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe0297d33e87
Introduce LocaleService::GetRegionalPrefsLocales. r=jfkthame

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe0297d33e87
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.