Introduce LocaleService::GetRegionalPrefsLocales

RESOLVED FIXED in Firefox 56

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

7 months 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

7 months ago
Blocks: 1366134
(Assignee)

Updated

7 months ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

6 months ago
Priority: -- → P3

Comment 3

6 months 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

6 months 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

6 months ago
Blocks: 1379905
(Assignee)

Updated

6 months ago
Blocks: 1379910

Comment 6

6 months 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

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

Comment 8

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe0297d33e87
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months 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.