Closed
Bug 1346674
Opened 6 years ago
Closed 6 years ago
Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file)
As part of the bug 1334772 to unify locale selection in the app, I'd like to move the last calls (gfx and js) away from nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. This is actually a meaningful change because the old one was actually using OS locale for platforms other than Windows. The new one is using the app locale for all platforms.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8846463 [details] Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/119490/#review121466 I think all these C++ call sites can be migrated away from the COM interface altogether, and just call LocaleService::GetInstance()->GetAppLocale() to get what they need. ::: gfx/thebes/gfxAndroidPlatform.cpp:135 (Diff revision 1) > do { // to allow 'break' to abandon this block if a call fails > nsresult rv; > - nsCOMPtr<nsILocaleService> ls = > - do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv); > + nsCOMPtr<mozILocaleService> ls = > + do_GetService(MOZ_LOCALESERVICE_CONTRACTID, &rv); > if (NS_FAILED(rv)) { > break; > } > - nsCOMPtr<nsILocale> appLocale; > - rv = ls->GetApplicationLocale(getter_AddRefs(appLocale)); > + nsAutoCString appLocale; > + rv = ls->GetAppLocale(appLocale); > if (NS_FAILED(rv)) { > break; > } > - nsString localeStr; > + const nsACString& lang = nsDependentCSubstring(localeStr, 0, 2); > - rv = appLocale-> > - GetCategory(NS_LITERAL_STRING(NSILOCALE_MESSAGE), localeStr); > - if (NS_FAILED(rv)) { > - break; > - } > - const nsAString& lang = nsDependentSubstring(localeStr, 0, 2); > if (lang.EqualsLiteral("ja")) { > sIsJapanese = true; > } > } while (false); See the similar code in gfxPlatformFontList.cpp; get rid of the COM usage here and just call the C++ service directly. ::: gfx/thebes/gfxDWriteFontList.cpp:276 (Diff revision 1) > - nsCOMPtr<nsILocaleService> ls = do_GetService(NS_LOCALESERVICE_CONTRACTID, > + nsCOMPtr<mozILocaleService> ls = do_GetService(MOZ_LOCALESERVICE_CONTRACTID, > &rv); > - nsCOMPtr<nsILocale> locale; > - rv = ls->GetApplicationLocale(getter_AddRefs(locale)); > + nsAutoCString locale; > + rv = ls->GetAppLocale(locale); > - nsString localeName; De-COM-taminate. ::: gfx/thebes/gfxPlatform.cpp:70 (Diff revision 1) > #include "gfxUtils.h" // for NextPowerOfTwo > > #include "nsUnicodeRange.h" > #include "nsServiceManagerUtils.h" > #include "nsTArray.h" > -#include "nsILocaleService.h" > +#include "mozILocaleService.h" Is this needed at all? Offhand, it looks redundant. ::: gfx/thebes/gfxPlatformFontList.cpp:14 (Diff revision 1) > #include "gfxTextRun.h" > #include "gfxUserFontSet.h" > > #include "nsCRT.h" > #include "nsGkAtoms.h" > -#include "nsILocaleService.h" > +#include "mozILocaleService.h" If you include "mozilla/intl/LocaleService.h" instead, you can bypass COM altogether (see below). ::: gfx/thebes/gfxPlatformFontList.cpp:1093 (Diff revision 1) > do { // to allow 'break' to abort this block if a call fails > nsresult rv; > - nsCOMPtr<nsILocaleService> ls = > - do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv); > + nsCOMPtr<mozILocaleService> ls = > + do_GetService(MOZ_LOCALESERVICE_CONTRACTID, &rv); > if (NS_FAILED(rv)) > break; > > - nsCOMPtr<nsILocale> appLocale; > - rv = ls->GetApplicationLocale(getter_AddRefs(appLocale)); > + nsAutoCString appLocale; > + rv = ls->GetAppLocale(appLocale); > if (NS_FAILED(rv)) > break; > > - nsString localeStr; > + NS_ConvertUTF8toUTF16 localeStr(appLocale); > - rv = appLocale-> > - GetCategory(NS_LITERAL_STRING(NSILOCALE_MESSAGE), localeStr); > - if (NS_FAILED(rv)) > - break; > > const nsAString& lang = Substring(localeStr, 0, 2); > if (lang.EqualsLiteral("ja")) { > AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_Japanese); > } else if (lang.EqualsLiteral("zh")) { > const nsAString& region = Substring(localeStr, 3, 2); > if (region.EqualsLiteral("CN")) { > AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseCN); > } else if (region.EqualsLiteral("TW")) { > AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseTW); > } else if (region.EqualsLiteral("HK")) { > AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseHK); > } > } else if (lang.EqualsLiteral("ko")) { > AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_Korean); > } > } while (0); This can be further simplified by getting rid of the COM stuff and 8/16-bit string conversion altogether: ``` nsAutoCString localeStr; mozilla::intl::LocaleService::GetInstance()->GetAppLocale(localeStr); const nsACString& lang = Substring(localeStr, 0, 2); if (lang.EqualsLiteral("ja")) { AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_Japanese); } else if (lang.EqualsLiteral("zh")) { const nsACString& region = Substring(localeStr, 3, 2); if (region.EqualsLiteral("CN")) { AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseCN); } else if (region.EqualsLiteral("TW")) { AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseTW); } else if (region.EqualsLiteral("HK")) { AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_ChineseHK); } } else if (lang.EqualsLiteral("ko")) { AppendPrefLang(tempPrefLangs, tempLen, eFontPrefLang_Korean); } ``` ::: intl/locale/nsLanguageAtomService.cpp:52 (Diff revision 1) > do { > if (!mLocaleLanguage) { > - nsCOMPtr<nsILocaleService> localeService; > - localeService = do_GetService(NS_LOCALESERVICE_CONTRACTID); > + nsCOMPtr<mozILocaleService> localeService; > + localeService = do_GetService(MOZ_LOCALESERVICE_CONTRACTID); > if (!localeService) { > res = NS_ERROR_FAILURE; > break; > } > > - nsCOMPtr<nsILocale> locale; > - res = localeService->GetApplicationLocale(getter_AddRefs(locale)); > + nsAutoCString locale; > + res = localeService->GetAppLocale(locale); > if (NS_FAILED(res)) > break; > > - nsAutoString loc; > + NS_ConvertUTF8toUTF16 loc(locale); > - res = locale->GetCategory(NS_LITERAL_STRING(NSILOCALE_MESSAGE), loc); > - if (NS_FAILED(res)) > - break; > > ToLowerCase(loc); // use lowercase for all language atoms > mLocaleLanguage = NS_Atomize(loc); > } > } while (0); de-COM-taminate ::: js/xpconnect/src/XPCLocale.cpp:167 (Diff revision 1) > - nsCOMPtr<nsILocaleService> localeService = > - do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv); > + nsCOMPtr<mozILocaleService> localeService = > + do_GetService(MOZ_LOCALESERVICE_CONTRACTID, &rv); > if (NS_SUCCEEDED(rv)) { > - nsCOMPtr<nsILocale> appLocale; > - rv = localeService->GetApplicationLocale(getter_AddRefs(appLocale)); > + nsAutoCString appLocale; > + rv = localeService->GetAppLocale(appLocale); > if (NS_SUCCEEDED(rv)) { > - nsAutoString localeStr; > + NS_ConvertUTF8toUTF16 localeStr(appLocale); and here ::: js/xpconnect/src/XPCLocale.cpp:252 (Diff revision 1) > - nsCOMPtr<nsILocaleService> localeService = > - do_GetService(NS_LOCALESERVICE_CONTRACTID); > + nsCOMPtr<mozILocaleService> localeService = > + do_GetService(MOZ_LOCALESERVICE_CONTRACTID); > if (!localeService) > return false; > > - nsCOMPtr<nsILocale> appLocale; > - nsresult rv = localeService->GetApplicationLocale(getter_AddRefs(appLocale)); > + nsCString appLocaleStr; > + nsresult rv = localeService->GetAppLocale(appLocaleStr); > if (NS_FAILED(rv)) > return false; and here
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Review feedback applied! I'm working on it on top of bug 1346819.
Depends on: 1346819
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8846463 [details] Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/119490/#review121718 ::: gfx/thebes/gfxAndroidPlatform.cpp:136 (Diff revision 7) > static bool sIsJapanese = false; > > if (!sInitialized) { > sInitialized = true; > > do { // to allow 'break' to abandon this block if a call fails There aren't any `break` statements for potential failures here any longer, so the `do { ... } while (0);` wrapper can just be removed. ::: gfx/thebes/gfxAndroidPlatform.cpp:140 (Diff revision 7) > > do { // to allow 'break' to abandon this block if a call fails > - nsresult rv; > - nsCOMPtr<nsILocaleService> ls = > - do_GetService(NS_LOCALESERVICE_CONTRACTID, &rv); > - if (NS_FAILED(rv)) { > + nsAutoCString appLocale; > + LocaleService::GetInstance()->GetAppLocaleAsLangTag(appLocale); > + > + const nsACString& lang = nsDependentCSubstring(localeStr, 0, 2); The string we're working with (just above) is called `appLocale` here, not `localeStr`. However, I think this line can be written more directly as const nsDependentCSubstring lang(appLocale, 0, 2); (I see you just kept the existing assign-to-a-reference pattern, but let's clean it up while we're touching this code.) ::: gfx/thebes/gfxPlatformFontList.cpp:1094 (Diff revision 7) > } > p++; > } > } > > do { // to allow 'break' to abort this block if a call fails Again, the `do ... while(0)` is unnecessary now. ::: intl/locale/nsLanguageAtomService.cpp:56 (Diff revision 7) > - nsCOMPtr<nsILocale> locale; > + NS_ConvertUTF8toUTF16 loc(locale); > - res = localeService->GetApplicationLocale(getter_AddRefs(locale)); > - if (NS_FAILED(res)) > - break; > - > - nsAutoString loc; > - res = locale->GetCategory(NS_LITERAL_STRING(NSILOCALE_MESSAGE), loc); > - if (NS_FAILED(res)) > - break; > > ToLowerCase(loc); // use lowercase for all language atoms > mLocaleLanguage = NS_Atomize(loc); There's a version of `NS_Atomize` that takes a UTF-8 string (as a `const nsACString&`), so you should be able to simply do ToLowerCase(locale); mLocaleLanguage = NS_Atomize(locale); without needing to convert to a 16-bit string first. ::: js/xpconnect/src/XPCLocale.cpp:247 (Diff revision 7) > return JS_SetDefaultLocale(cx, "en-US"); > } > > // No pref has been found, so get the default locale from the > // application's locale. > - nsCOMPtr<nsILocaleService> localeService = > + nsCString appLocaleStr; nsAutoCString
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
Thanks! Updated to feedback. Restarting try run :)
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8846463 [details] Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. https://reviewboard.mozilla.org/r/119490/#review121932
Attachment #8846463 -
Flags: review?(jfkthame) → review+
Comment 14•6 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fcd6a97a1b8 Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. r=jfkthame
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2fcd6a97a1b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 16•6 years ago
|
||
It seems that the change to js/xpconnect/src/XPCLocale.cpp caused a behavior change: on my Linux system with LC_TIME=de_DE.UTF-8 and en-US Firefox nightly, I now get the US date format via Date.toLocaleString() and the like rather than the German date format that it used to be. Was this change intentional?
Assignee | ||
Comment 17•6 years ago
|
||
Yes. The default locale is now the product locale, not the OS locale
Comment 18•6 years ago
|
||
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/intl-api-now-uses-browser-locale-by-default-on-all-platforms/
Keywords: dev-doc-needed,
site-compat
Comment 19•6 years ago
|
||
Added to https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•