Retrieve the correct OSPreferences::GetRegonalPrefsLocales on Unix using LC_TIME

RESOLVED FIXED in Firefox 58

Status

()

Core
Internationalization
P3
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Assignee)

Description

a month ago
As per analysis in bug 1337069, the low hanging fruit here is to retrieve the LC_TIME as the operating system regionalprefslocale.
(Assignee)

Updated

a month ago
Assignee: nobody → gandalf
Blocks: 1337069
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 2

a month ago
mozreview-review
Comment on attachment 8919051 [details]
Bug 1409158 - Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix.

https://reviewboard.mozilla.org/r/189942/#review195108


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/locale/gtk/OSPreferences_gtk.cpp:35
(Diff revision 1)
>  {
> -  // For now we're just taking System Locales since we don't know of any better
> -  // API for regional prefs.
> -  return ReadSystemLocales(aLocaleList);
> +  MOZ_ASSERT(aLocaleList.IsEmpty());
> +
> +  // For now we're just taking the LC_TIME from POSIX environment for all
> +  // regional preferences.
> +  const char* posixID = setlocale(LC_TIME, NULL);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  const char* posixID = setlocale(LC_TIME, NULL);
                                           ^
                                           nullptr
Comment hidden (mozreview-request)

Updated

a month ago
Priority: -- → P3

Comment 4

a month ago
mozreview-review
Comment on attachment 8919051 [details]
Bug 1409158 - Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix.

https://reviewboard.mozilla.org/r/189942/#review195852

Seems reasonable enough. I'm not sure what we should do if LC_NUMERIC, for example, were set to something different - but let's not worry about that for now.

::: intl/locale/gtk/OSPreferences_gtk.cpp:35
(Diff revision 2)
> +  const char* posixID = setlocale(LC_TIME, nullptr);
> +  nsAutoCString localeStr;
> +  localeStr.Assign(posixID);

It'd be more concise to just write this as

    nsAutoCString localeStr(setlocale(LC_TIME, nullptr));

and should be directly equivalent.
Attachment #8919051 - Flags: review?(jfkthame) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
> I'm not sure what we should do if LC_NUMERIC, for example, were set to something different - but let's not worry about that for now.

I'd like to get to the point where we'll allow you to do `GetRegionalPreferences(LocaleService::Type::Numeric)` or sth like that, but that's a bit longer term. :)

Comment 7

a month ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4577afc2a13d
Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/4577afc2a13d
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

a month ago
Depends on: 1410266
Duplicate of this bug: 1391411
Blocks: 1404166
You need to log in before you can comment on or make changes to this bug.