Closed Bug 1346674 Opened 8 years ago Closed 8 years ago

Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
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)

Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
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.
Assignee: nobody → gandalf
Blocks: 1334772
Status: NEW → ASSIGNED
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
Review feedback applied! I'm working on it on top of bug 1346819.
Depends on: 1346819
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
Thanks! Updated to feedback. Restarting try run :)
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+
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fcd6a97a1b8 Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/2fcd6a97a1b8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348259
Depends on: 1348299
Depends on: 1348535
Depends on: 1349454
Depends on: 1349470
Blocks: 943283
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?
Yes. The default locale is now the product locale, not the OS locale
Depends on: 1373885
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: