This is a spin-off from bug 1308329. That bug covers OSPreferences for Regional Settings (hour12/24, date time format etc.) but it seems to me that we'll also want to use it for retrieving the list of OS locales, and we should start with just that. My plan now is to introduce intl/locale/OSPreferences C++ API and mozIIntlOSPreferences JS API here, then work in parallel on mozLocaleService in bug 1332207 and on extending OSPreferences in bug 1308329.

Comment on attachment 8829692 [details] Bug 1333184 - Introduce OSPreferences API. just feedback request for now. :jfkthame, does it look reasonable?

Comment on attachment 8829692 [details] Bug 1333184 - Introduce OSPreferences API. Uhh, sorry for the mess, mozreview is insisting. I added some stub for Windows API - decided to go for GetUserPreferredUILanguages because the other you suggested (Fallback) already injects fallback locales into the chain. I'd like to keep separation of concerns - this API returns system locales, as they are defined by the user/OS. I didn't find winapi for reacting to events when locale chain changes. I also added stubs for macos (including events). I also believe that we should return the langauge id part of the locale id, and it's ok if macos returns zh-Hans-CN and windows only zh-CN - both just have to be canonicalized and should work after fed into language negotiation. I don't know how to convert the winapi into our nsTArray<nsCString>.

Comment on attachment 8829692 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/106698/#review107818 Looks promising; some questions/suggestions below. BTW, when it's closer to being ready to land, I think it'd be great if this were split into a bunch of commits for separate review -- first, the interface and generic stuff, and then a separate commit for each platform-specific implementation. Though I don't really know much about managing mozreview stuff; if it's hard to do the separation, I guess it's not crucial. ::: intl/locale/OSPreferences.h:22 (Diff revision 2) > + > + class OSPreferences > + { > + > + public: > + static void GetSystemLocales(nsTArray<nsCString>& aRetVal); I'm not sure we want this (and the other methods below, probably) to be `static`, as we may want to cache the list from the OS in our OSPreferences instance (so we only have to re-query the OS and re-canonicalize the result if there's been a change). ::: intl/locale/OSPreferences.h:34 (Diff revision 2) > + if (obsSvc) { > + obsSvc->NotifyObservers(nullptr, "intl:system-locales-changed", nullptr); > + } > + } > + > + static nsCString CanonicalizeLanguageTag(const char* loc) I think this would probably be better declared as something like bool CanonicalizeLanguageTag(nsCString& aLoc) which would modify the given string in-place (leaving it untouched and returning `false` on failure). This lets the caller use an nsAutoCString, so we don't need to create a new heap-allocated string. ::: intl/locale/android/OSPreferences.cpp:11 (Diff revision 2) > + > +#include "OSPreferences.h" > + > +using namespace mozilla::intl; > + > +void OSPreferences::GetSystemLocales(nsTArray<nsCString>& aLocaleList) { nit: the open-brace belongs on a new line - this looks like a JS habit :) ::: intl/locale/gtk/OSPreferences.cpp:28 (Diff revision 2) > + // extract one locale and trim any leading/trailing whitespace > + nsAutoCString loc; > + loc = Substring(start, cp); > + loc.CompressWhitespace(true, true); > + > + nsCString cloc = OSPreferences::CanonicalizeLanguageTag(loc.get()); See comment on CanonicalizeLanguageTag. Then, this would become if (OSPreferences::CanonicalizeLanguageTag(loc)) { aLocaleList.AppendElement(loc); } But what about dupes? If the list here is coming straight from something like an environment variable, it can probably contain duplicates, and I suspect it'll be better if we filter those out. So, if (OSPreferences::CanonicalizeLanguageTag(loc) && !aLocaleList.Contains(loc)) { aLocaleList.AppendElement(loc); } assuming that if a locale code is repeated, we only need to keep the first occurrence. ::: intl/locale/gtk/OSPreferences.cpp:42 (Diff revision 2) > +/** > + * For historical reasons, we'll initially use a locale from env LANG > + * but we will load LANGUAGES as well, and if the first locale in the chain > + * matches one from LANG, we'll add the remaining ones to the fallback > + */ > +void OSPreferences::GetSystemLocales(nsTArray<nsCString>& aLocaleList) { `{` on new line ::: intl/locale/gtk/OSPreferences.cpp:49 (Diff revision 2) > + > + // ICU uses LANG/LC_ALL > + nsCString defaultLang = CanonicalizeLanguageTag(uloc_getDefault()); > + > + // get the fallback chain > + const char* langs = getenv("LANGUAGE"); `getenv()` can return null, so we'd better check this. ::: intl/locale/gtk/OSPreferences.cpp:51 (Diff revision 2) > + nsTArray<nsCString> fallbackLangs; > + ParseLocaleList(nsCString(langs), fallbackLangs); As this is just for a local helper function, no need to construct a `nsCString` -- just declare ParseLocaleList as taking a `const char*` and pass `langs` directly. ::: intl/locale/gtk/OSPreferences.cpp:58 (Diff revision 2) > + > + aLocaleList.AppendElement(defaultLang); > + > + if (!fallbackLangs.IsEmpty() && fallbackLangs[0].Equals(defaultLang)) { > + for (size_t i = 1; i < fallbackLangs.Length(); i++) { > + aLocaleList.AppendElement(fallbackLangs[i]); Don't we want to call Canonicalize on these as well? Also, probably should check for duplicates and skip them. ::: intl/locale/gtk/moz.build:11 (Diff revision 2) > + > +SOURCES += [ > + 'OSPreferences.cpp', > +] > + > +CXXFLAGS += CONFIG['GLIB_CFLAGS'] Is this needed? I don't see any use of glib functions in there at the moment. ::: intl/locale/mac/OSPreferences.cpp:31 (Diff revision 2) > + aLocaleList.AppendElement(NS_ConvertUTF16toUTF8(str)); > + } > + CFRelease(languages); > +} > + > +static void _localeChanged(CFNotificationCenterRef, void*, CFStringRef, const void*, CFDictionaryRef) We don't generally do the underscore-prefix thing in C++ code -- your JS is showing. ;-) Just declaring the function as `static` here is enough to hide it from the outside world. ::: intl/locale/mac/OSPreferences.cpp:39 (Diff revision 2) > + CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), (void*)this, _localeChanged, > + kCFLocaleCurrentLocaleDidChangeNotification, > + NULL, CFNotificationSuspensionBehaviorDeliverImmediately); You can't do this (specifically, refer to `this`) in a static function -- there is no object involved. Fortunately, I don't think you actually need to pass an object here; null is fine. Though I wonder if all these functions should actually be instance methods of the (singleton) OSPreferences object, rather than class (`static`) methods. If we start caching state in the object, we'll need to be referring to `this` in order to access/update its cached instance data. ::: intl/locale/mozIIntlOSPreferences.idl:20 (Diff revision 2) > +%} > + > +[scriptable, uuid(D401FB5B-4542-4D98-B53C-9EA50D8F341D)] > +interface mozIIntlOSPreferences : nsISupports > +{ > + [implicit_jscontext] jsval GetSystemLocales(); One nit: I think in IDL, we normally declare methods without an initial capital, don't we? More important, I was wondering is whether we want this to be easily callable from C++ as well as from JS? If so, it might be preferable to declare it as something like void getSystemLocales([optional] out unsigned long localeCount, [retval, array, size_is(localeCount)] out string locales); which results in a method that I believe can be called from plain C++ code (without a JSContext, etc), but is still callable from JS just as conveniently. (We'd obviously need to update the code in GetSystemLocales that sets up the return value; I can provide that if we want to use this signature.) ::: intl/locale/windows/OSPreferences.cpp:12 (Diff revision 2) > +#include "OSPreferences.h" > +#include "nsWin32Locale.h" > + > +using namespace mozilla::intl; > + > +void OSPreferences::GetSystemLocales(nsTArray<nsCString>& aLocaleList) { new line for { ::: intl/locale/windows/OSPreferences.cpp:15 (Diff revision 2) > + StringBuilder languagesBuffer = new StringBuilder(); > + uint languagesCount, languagesBufferSize = 0; > + > + if (!GetUserPreferredUILanguages( > + MUI_LANGUAGE_NAME, > + out languagesCount, > + null, > + ref languagesBufferSize)) > + { > + return; > + } > + > + languagesBuffer.EnsureCapacity((int)languagesBufferSize); > + > + if (!GetUserPreferredUILanguages( > + MUI_LANGUAGE_NAME, > + out languagesCount, > + languagesBuffer, > + ref languagesBufferSize)) > + { > + return; > + } > + > + > + for (string language in languages) { > + aLocaleList.AppendElement(CanonicalizeLanguageTag(language)); > + } Something along these lines, I think (untested, not typing this on a Windows machine): ``` ULONG languagesCount, languagesBufferSize = 0; if (!GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &languagesCount, nullptr, &languagesBufferSize)) { return; } auto languagesBuffer = MakeUnique<WCHAR>(languagesBufferSize); if (GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, &languagesCount, languagesBuffer.get(), &languagesBufferSize)) { return; } const WCHAR* start = languagesBuffer.get(); while (*start) { // find the null terminator of a single language name const WCHAR* end = start + 1; while (*end) { ++end; } // convert to 8-bit and add it to our list NS_LossyConvertUTF16toASCII lang((const char16_t*)start, end - start); if (CanonicalizeLanguageTag(lang) && !aLocaleList.Contains(lang)) { aLocaleList.AppendElement(lang); } // point to the start of the next name, or the final null terminator start = end + 1; } // sanity-check: start should be left pointing at the final null character MOZ_ASSERT(start - languagesBuffer.get() == languagesBufferSize - 1, "bad languages list!"); ```

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4) > I also believe that we should return the langauge id part of the locale id, > and it's ok if macos returns zh-Hans-CN and windows only zh-CN - both just > have to be canonicalized and should work after fed into language negotiation. The problem I think we're going to run into, unless I'm missing something, is that we use zh-CN and zh-TW in Gecko to mean Chinese written in Simplified and Traditional characters respectively; but macOS will return things like zh-Hans-GB if I add Simplified Chinese as a secondary language, while my Regional settings are still set to GB. Will we be canonicalizing the available Gecko locale codes in such a way that implied script subtags get *added* (so zh-CN => zh-Hans-CN and zh-TW => zh-Hant-TW) before we get into negotiation of the requested (system) languages vs the available (gecko) localizations? If we don't incorporate these (implied) script subtags, how will we know whether zh-Hans-GB should result in the CN or TW variant of zh? Do we have language-negotiation code that knows to handle this internally? (Ideally, what I think we should do would be to rename Gecko's Chinese locales as zh-Hans and zh-Hant, given that the distinction between them is in fact a question of Simplified vs Traditional characters, i.e. the orthography/script, rather than the specific region. But I don't know how much pain and disruption that might involve...)

Thanks :jfkthame! I'll work on applying your feedback. I agree with the static - I just don't know how and where to instantiate the singleton for OSPreferences so that it lives as long as the app does. And I didn't touch caching yet - I agree we'll want to cache :) > Will we be canonicalizing the available Gecko locale codes in such a way that implied script subtags get *added* (so zh-CN => zh-Hans-CN and zh-TW => zh-Hant-TW) before we get into negotiation of the requested (system) languages vs the available (gecko) localizations? If we don't incorporate these (implied) script subtags, how will we know whether zh-Hans-GB should result in the CN or TW variant of zh? Do we have language-negotiation code that knows to handle this internally? That's a great question. I'll look more into mozLocaleService as this is where we'll have to solve it. In my naive hope, the language negotiation will work in a way that allows us to pass it a list of requested locale ids (say ['zh-Hans-CN']) and the list of available locales (which in this case will be a list of language tags we have resources for: ['zh-CN', 'de', ...]) and we will return a list of the best matching language tags for the request (in this case ['zh-CN']). I'm diving into ICU API for things like addLikelySubtags, minimizeSubtags etc. to figure out how to do this. If I fail, we may have to forcefully map OS locales in OSPreferences to match language tags that we use, but I'd love to try to avoid that first.

Attaching POC

Ok, since it seems we agree on a general direction, I decided to start small and expand. The first patch is basically the code from nsLocaleService moved into the OSPreferences API. If we can land this basic API, I'll file bugs to expand each implementation with more appropriate platform APIs, and later maybe add the event for system language changes.

Comment on attachment 8829692 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/106698/#review108240 ::: intl/locale/OSPreferences.h:38 (Diff revision 3) > + > + }; > +} // intl > +} // namespace mozilla > + > +static bool CanonicalizeLanguageTag(nsCString& aLoc) Make this a static member of OSPreferences, I think, so that it doesn't end up in the global namespace. So declare it (within the class) as static bool CanonicalizeLanguageTag(nsCString& aLoc); and then put the implementation in the .cpp file. ::: intl/locale/OSPreferences.h:45 (Diff revision 3) > + char langTag[512]; > + > + UErrorCode status = U_ZERO_ERROR; > + > + int32_t langTagLen = > + uloc_toLanguageTag(aLoc.get(), langTag, sizeof(langTag)-1, true, &status); Do we really want 'strict' behavior here? "When strict is FALSE, any locale fields which do not satisfy the BCP47 syntax requirement will be omitted from the result. When strict is TRUE, this function sets U_ILLEGAL_ARGUMENT_ERROR to the err if any locale fields do not satisfy the BCP47 syntax requirement." If the OS gives us locale codes that include non-BCP47-compliant extensions of some kind, it's probably better to just drop those fields than to fail altogether, which suggests passing 'false' rather than 'true' for the 'strict' param. ::: intl/locale/OSPreferences.h:47 (Diff revision 3) > + if (U_FAILURE(status)) > + return false; style nit: if-statement needs braces ::: intl/locale/OSPreferences.h:50 (Diff revision 3) > + langTag[langTagLen] = 0; > + > + aLoc.Assign(langTag); No need to do the explicit termination here; just call aLoc.Assign(langTag, langTagLen); (which will also be more efficient). ::: intl/locale/OSPreferences.cpp:12 (Diff revision 3) > + > +using namespace mozilla::intl; > + > +void OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList) > +{ > + // NOOP since dummy OSPreferences don't know how to retrieve any settings Should we maybe do a simple `getenv("LANG")` here or something? ::: intl/locale/mac/OSPreferences.cpp:8 (Diff revision 3) > +#include "nsWin32Locale.h" > + > +using namespace mozilla::intl; > + > +void OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList) > +{ > + MOZ_ASSERT(aLocaleList.IsEmpty()); > + > + nsAutoString locale; > + > + LCID win_lcid = GetSystemDefaultLCID(); > + NS_ENSURE_TRUE_VOID(win_lcid); > + nsWin32Locale::GetXPLocale(win_lcid, locale); This doesn't look like the right content for mac/OSPreferences.cpp :) ::: intl/locale/unix/OSPreferences.cpp:15 (Diff revision 3) > + > +void OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList) > +{ > + MOZ_ASSERT(aLocaleList.IsEmpty()); > + > + nsCString defaultLang(uloc_getDefault()); Use nsAutoCString here, to avoid heap allocation. ::: intl/locale/windows/OSPreferences.cpp:8 (Diff revision 3) > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "OSPreferences.h" > +#include <Carbon/Carbon.h> Ah, it looks like the mac and windows files are simply exchanged! ::: intl/locale/windows/OSPreferences.cpp:17 (Diff revision 3) > +void OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList) > +{ > + // Get string representation of user's current locale > + CFLocaleRef userLocaleRef = ::CFLocaleCopyCurrent(); > + CFStringRef userLocaleStr = ::CFLocaleGetIdentifier(userLocaleRef); > + ::CFRetain(userLocaleStr); This isn't needed, the string will survive at least until we release userLocaleRef. ::: intl/locale/windows/OSPreferences.cpp:29 (Diff revision 3) > + if (CanonicalizeLanguageTag(locale)) { > + aLocaleList.AppendElement(locale); I don't think this will compile as written - `locale` here is an ns(Auto)String, which is 16-bit, but `aLocaleList` is a list of `nsCString` (8-bit) strings. Oh, and that's also what Canonicalize... wants. We probably want something like NS_LossyConvertUTF16toASCII locale(reinterpret_cast<const char16_t*>(buffer.Elements()), buffer.Length()); to create an ASCII ("C") string, instead of the nsAutoString above. (Or we could use NS_ConvertUTF16toUTF8(...), but locale codes should be pure ASCII anyway so the lossy conversion will be fine.) ::: intl/locale/windows/OSPreferences.cpp:33 (Diff revision 3) > + > + if (CanonicalizeLanguageTag(locale)) { > + aLocaleList.AppendElement(locale); > + } > + > + ::CFRelease(userLocaleStr); And if you remove the CFRetain above, then this also needs to go.

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7) > I agree with the static - I just don't know how and where to instantiate the > singleton for OSPreferences so that it lives as long as the app does. I'd suggest using a static (class) method OSPreferences::GetInstance() which returns a singleton instance (stored in a class member variable), creating it on the first call. So in OSPreferences.h you'll have: public: static OSPreferences* GetInstance() { if (!sInstance) { sInstance = new OSPreferences; } return sInstance; } protected: static OSPreferences* sInstance; and in OSPreferences.cpp, you just need to declare the static member var: OSPreferences::OSPreferences* sInstance; Then any C++ users we have can just say things like AutoTArray<nsCString> systemLocales; OSPreferences::GetInstance()->GetSystemLocales(systemLocales); without needing to use the XPCOM service at all. Meanwhile, the implementation in mozIntlOSPreferences::GetSystemLocales() will do the same thing to get the list, and then set up its jsval return as in the POC patch. We might also want a Shutdown() method: public: static void Shutdown() { delete sInstance; } which can be called as OSPreferences::Shutdown() from somewhere in the XPCOM shutdown sequence (there are a bunch of such methods around already) to allow us to remove observers, etc. if necessary.

Comment on attachment 8829692 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/106698/#review108402 General comment: I think it'd be helpful to rename the platform-specific OSPreferences.cpp files as OSPreferences_mac.cpp, OSPreferences_win.cpp, etc. (even though they're also in separate directories) so that we don't have two files with the same (leaf) filename in the build. This looks pretty close by now, just minor cleanup/tweaks noted below; then it's probably ready to actually try building on the various platforms and see how it goes. ::: intl/locale/OSPreferences.h:16 (Diff revision 4) > + class OSPreferences > + { > + > + public: > + static OSPreferences* GetInstance(); > + static void Shutdown(); > + > + void GetSystemLocales(nsTArray<nsCString>& aRetVal); > + > + protected: > + static OSPreferences* sInstance; > + nsTArray<nsCString> mSystemLocales; > + > + static bool CanonicalizeLanguageTag(nsCString& aLoc); > + > + void ReadSystemLocales(nsTArray<nsCString>& aRetVal); > + > + }; A couple of C++ style nits here: we don't normally indent everything within `namespace {...}`, and we don't indent the `public`/`protected` keywords within the class declaration. ::: intl/locale/OSPreferences.cpp:28 (Diff revision 4) > +} > + > +void OSPreferences::GetSystemLocales(nsTArray<nsCString>& aRetVal) > +{ > + if (mSystemLocales.IsEmpty()) { > + ReadSystemLocales(mSystemLocales); I wonder, should we do something here to handle the case where `ReadSystemLocales()` fails to return anything usable? (Shouldn't happen, but it may be possible for a user on some systems to hack their system such that the APIs we use end up returning a code that Canonicalize rejects as ill-formed.) At some level, we probably want to provide a last-ditch fallback so that callers can rely on getting *something* usable from `GetSystemLocales()`. Would it make sense to just do if (mSystemLocales.IsEmpty()) { mSystemLocales.AppendElement("en-US"); } here so that we never return an empty list? ::: intl/locale/mac/OSPreferences.cpp:13 (Diff revision 4) > +#include <Carbon/Carbon.h> > + > +using namespace mozilla::intl; > + > +void OSPreferences::ReadSystemLocales(nsTArray<nsCString>& aLocaleList) > +{ For consistency, include the initial `MOZ_ASSERT(aLocaleList.IsEmpty());` like in the other implementations. ::: intl/locale/mac/OSPreferences.cpp:26 (Diff revision 4) > + CFRange range = ::CFRangeMake(0, size); > + ::CFStringGetCharacters(userLocaleStr, range, buffer.Elements()); > + buffer[size] = 0; > + > + // Convert the locale string to the format that Mozilla expects > + NS_LossyConvertUTF16toASCII locale(reinterpret_cast<const char16_t*>(buffer.Elements()), buffer.Length()); This line should be wrapped (maybe twice) to keep it under 80 chars, please. Hmm, on second thought `buffer.Length()` is wrong here, as that includes the null terminator. The actual string length is `size`. But wait: we don't need to null-terminate `buffer` at all, if we pass its length explicitly to the converting constructor here. So no need for `SetLength(size + 1)` above, just `size` will do; and remove the assignment of the terminator. ::: intl/locale/mac/OSPreferences.cpp:31 (Diff revision 4) > + NS_LossyConvertUTF16toASCII locale(reinterpret_cast<const char16_t*>(buffer.Elements()), buffer.Length()); > + > + if (CanonicalizeLanguageTag(locale)) { > + aLocaleList.AppendElement(locale); > + } > + You still need to `CFRelease(userLocaleRef)` here to avoid a leak; it was only the release of `userLocaleStr` that became redundant with removal of the explicit retain. ::: intl/locale/mac/OSPreferences.cpp:32 (Diff revision 4) > + > + if (CanonicalizeLanguageTag(locale)) { > + aLocaleList.AppendElement(locale); > + } > + > + NS_ASSERTION(mApplicationLocale, "Failed to create locale objects"); This assertion doesn't belong, looks like a hangover from the old code. ::: intl/locale/windows/OSPreferences.cpp:19 (Diff revision 4) > + MOZ_ASSERT(aLocaleList.IsEmpty()); > + > + nsAutoString locale; > + > + LCID win_lcid = GetSystemDefaultLCID(); > + NS_ENSURE_TRUE_VOID(win_lcid); I see this was copied from nsLocaleService.cpp, but I don't think it belongs here; AFAICS from MSDN, `GetSystemDefaultLCID()` doesn't have a return-zero-on-failure behavior. (We should probably convert this to simply call `GetSystemDefaultLocaleName` instead of `GetSystemDefaultLCID` + `GetXPLocale`, as we're no longer maintaining WinXP compatibility, AFAIK. But that can be left for a followup.) ::: intl/locale/windows/OSPreferences.cpp:20 (Diff revision 4) > + > + nsAutoString locale; > + > + LCID win_lcid = GetSystemDefaultLCID(); > + NS_ENSURE_TRUE_VOID(win_lcid); > + nsWin32Locale::GetXPLocale(win_lcid, locale); This returns a 16-bit string in `locale`, so you'll need to convert this to 8-bit ASCII for the Canonicalize and AppendElement below.

> Do we really want 'strict' behavior here? My original thinking was that if there's a bogus entry, we want to reject it, but I guess we can try to get anything meaningful out of it as well. > I wonder, should we do something here to handle the case where `ReadSystemLocales()` fails to return anything usable? I'm initially against it. This is supposed to return the system languages. There may be a case where there are no system languages available. This is not to be used by any piece of Gecko trying to retrieve app languages. In fact, I expect this to be used exclusively by LocaleService to negotiate fallback languages. LocaleService::GetAppLocales will have a last ditch effort (hopefully synced with uloc_DefaultLocale), as it will have to return at least one locale. But this code can return nothing and it's ok. I applied the rest of your feedback and hopefully this should be ready for try :)

One more thought. If you think it makes sense, we could turn the return value from GetSystemLocales to be a bool that returns false if no locale has been identified. That would be functionally equal to testing the return value for .`IsEmpty()` so I'm not sure if it's useful.

The try build looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b2cb29569b5

> static void Shutdown() > { > delete sInstance; > } Probably a good idea to add "sInstance = nullptr;" just in case there's a GetInstance() call after the Shutdown(). Even if that "shouldn't happen" it's better to have a leak than a use-after-free when it does. ;-)

Yes, good point. Zibi, that applies to the patch in bug 1332207 too.

:jfkthame - I'd like to land it without waiting for intl in android, so I added the config checks for now. Landing this will enable several parallel initiatives: - work on improving the use of platform specific APIs (I'll file bugs) - migration of nsChromeRegistry `intl.locale.matchOS` case to use it (that has to wait for android I believe) - bug 1308329 to add APIs for retrieving Date/Time format and hourCycle bit - and bug 1332207 to land LocaleService

updated the code based on your review from LocaleService. It's behind the ENABLE_INTL_API pref so I think it should be good to land soon. It'll enable me to work on bug 1308329 which I need for bug 1329904.

Comment on attachment 8833786 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/109940/#review111066 ::: intl/locale/OSPreferences.h:24 (Diff revision 1) > + * the host environment on topics such as: > + * - Internationalization > + * - Localization > + * - Regional preferences > + * > + * The API is meant to remain as simple as possible, relying information from s/relying/relaying/ ::: intl/locale/OSPreferences.h:35 (Diff revision 1) > + * concepts to unified Intl/L10n vocabulary used by Mozilla. > + * That means that we will format locale IDs, timezone names, currencies etc. > + * into a chosen format. > + * > + * Second is caching. This API does cache values and where possible will > + * hook into the environment for some event-driven cache invalidation. Maybe add a note to make it clear: "This means that on platforms that do not support a mechanism to notify apps about changes, new OS-level settings may not be reflected in Firefox until it is relaunched." ::: intl/locale/OSPreferences.h:60 (Diff revision 1) > + * > + * The return bool value indicates whether the function successfully > + * resolved at least one locale. > + * > + * Usage: > + * nsTArray<nsCString>& systemLocales; no '&' here, the caller wants to declare an actual variable, not a reference ::: intl/locale/OSPreferences.h:69 (Diff revision 1) > + > +protected: > + nsTArray<nsCString> mSystemLocales; > + > +private: > + static StaticAutoPtr<OSPreferences> sInstance; This is OK for now, if we later make this class provide the implemention of a JS API as well (like LocaleService) then it'll probably become a StaticRefPtr instead. ::: intl/locale/OSPreferences.cpp:9 (Diff revision 1) > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * This is a shared part of the OSPreferences API implementation. > + * It defines helper methods and public methods that are calling > + * platform specific private methods. s/platform specific/platform-specific/ ::: intl/locale/mac/OSPreferences_mac.cpp:17 (Diff revision 1) > + // Get string representation of user's current locale > + CFLocaleRef userLocaleRef = ::CFLocaleCopyCurrent(); > + CFStringRef userLocaleStr = ::CFLocaleGetIdentifier(userLocaleRef); This only gets the user's current locale; didn't we have some code to return the full list from CFLocaleCopyPreferredLanguages? That seems more like what this API wants. ::: intl/locale/moz.build:12 (Diff revision 1) > XPCSHELL_TESTS_MANIFESTS += ['tests/unit/xpcshell.ini'] > > +if CONFIG['ENABLE_INTL_API']: > + SOURCES += ['OSPreferences.cpp'] > + > + one blank line would be enough ::: intl/locale/tests/gtest/TestOSPreferences.cpp:21 (Diff revision 1) > + * decide how to handle this and special case and this test should make > + * it not happen without us noticing. > + */ > +TEST(Intl_Locale_OSPreferences, GetSystemLocales) { > + nsTArray<nsCString> systemLocales; > + OSPreferences::GetInstance()->GetSystemLocales(systemLocales); You could also assert that this returns 'true'. ::: intl/locale/windows/OSPreferences_win.cpp:19 (Diff revision 1) > + LCID win_lcid = GetSystemDefaultLCID(); > + nsWin32Locale::GetXPLocale(win_lcid, locale); Like the Mac version, I thought we wanted to use something like GetUserPreferredUILanguages to get an ordered list, not just a single entry?

Comment on attachment 8833786 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/109940/#review111066 > This is OK for now, if we later make this class provide the implemention of a JS API as well (like LocaleService) then it'll probably become a StaticRefPtr instead. yeah, I expect that we'll want the JS API for when we'll add regional settings later (to use in mozIntl). I'll do this in that bug. > This only gets the user's current locale; didn't we have some code to return the full list from CFLocaleCopyPreferredLanguages? That seems more like what this API wants. I wanted to focus this patch in introducing the API, I took the OS specific code directly from the current nsLocaleService. My intention is to file 4 platform-specific bugs to improve the handling per-platform once this lands. > Like the Mac version, I thought we wanted to use something like GetUserPreferredUILanguages to get an ordered list, not just a single entry? same answer. This bug is about the new API, seaprate bugs for improving the logic. Among other reasons, it'll make it much easier to track down any regression that may come out of the using the new platform-specific APIs or from the fact that we'll change which APIs we call. Is that ok?

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #30) > I wanted to focus this patch in introducing the API, I took the OS specific > code directly from the current nsLocaleService. > My intention is to file 4 platform-specific bugs to improve the handling > per-platform once this lands. ... > Is that ok? Yes, that's a good way to handle this. I'll look it over one more time shortly, but I expect it's good to go.

Comment on attachment 8833786 [details] Bug 1333184 - Introduce OSPreferences API. https://reviewboard.mozilla.org/r/109940/#review111246 OK, lgtm. ::: intl/locale/OSPreferences.cpp:19 (Diff revision 2) > + > +using namespace mozilla::intl; > + > +mozilla::StaticAutoPtr<OSPreferences> OSPreferences::sInstance; > + > +OSPreferences* OSPreferences::GetInstance() newline after the return type, please

Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b02d4bac8928 Introduce OSPreferences API. r=jfkthame