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

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Depends on 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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

Updated

2 years ago
Assignee: nobody → gandalf
Blocks: 1334772
Status: NEW → ASSIGNED

Comment 2

2 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)
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

2 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)
Thanks! Updated to feedback. Restarting try run :)

Comment 13

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2fcd6a97a1b8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Depends on: 1348259

Updated

2 years ago
Depends on: 1348299

Updated

2 years ago
Depends on: 1348535
Depends on: 1349454
Depends on: 1349470
Blocks: 943283

Comment 16

2 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?
Yes. The default locale is now the product locale, not the OS locale
You need to log in before you can comment on or make changes to this bug.