1409158 - Retrieve the correct OSPreferences::GetRegonalPrefsLocales on Unix using LC_TIME

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

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

As per analysis in bug 1337069, the low hanging fruit here is to retrieve the LC_TIME as the operating system regionalprefslocale.

Comment 1

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

Attachment: Bug 1409158 - Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix.

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

Comment 2

[Code Review Bot [:reviewbot]]

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 3

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

Comment on attachment 8919051 [details]
Bug 1409158 - Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix.

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

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

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

Comment on attachment 8919051 [details]
Bug 1409158 - Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix.

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

Comment 6

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

> 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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4577afc2a13d
Use LC_TIME to retrieve OSPreferences::GetRegionalPrefsLocales on Unix. r=jfkthame

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4577afc2a13d