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

Assignee

Description

•

6 years ago

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)

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

Zibi Braniecki [:zbraniecki][:gandalf]

Assignee

Updated

•

6 years ago

Assignee: nobody → gandalf

Blocks: 1334772

Status: NEW → ASSIGNED

Jonathan Kew [:jfkthame]

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 on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

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

Comment hidden (mozreview-request)

Comment on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

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

Comment hidden (mozreview-request)

Comment on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

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

Comment hidden (mozreview-request)

Comment on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119490/diff/4-5/

Zibi Braniecki [:zbraniecki][:gandalf]

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 on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119490/diff/5-6/

Comment hidden (mozreview-request)

Comment on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119490/diff/6-7/

Jonathan Kew [:jfkthame]

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)

Comment on attachment 8846463 [details]
Bug 1346674 - Migrate all uses of nsILocaleService::GetApplicationLocale to mozILocaleService::GetAppLocale.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119490/diff/7-8/

Zibi Braniecki [:zbraniecki][:gandalf]

Assignee

Comment 12

•

6 years ago

Thanks! Updated to feedback. Restarting try run :)

Jonathan Kew [:jfkthame]

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+

Pulsebot

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

Carsten Book [:Tomcat]

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

Alice0775 White

Updated

•

6 years ago

Depends on: 1348259

Alice0775 White

Updated

•

6 years ago

Depends on: 1348299

Alice0775 White

Updated

•

6 years ago

Depends on: 1348535

Makoto Kato [:m_kato]

Updated

•

6 years ago

Depends on: 1349454

Makoto Kato [:m_kato]

Updated

•

6 years ago

Depends on: 1349470

Makoto Kato [:m_kato]

Updated

•

6 years ago

Blocks: 943283

Wladimir Palant

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?

Zibi Braniecki [:zbraniecki][:gandalf]

Assignee

Comment 17

•

6 years ago

Yes. The default locale is now the product locale, not the OS locale

Kohei Yoshino

Updated

•

6 years ago

Depends on: 1373885

Kohei Yoshino

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

Florian Scholz (Open Web Docs)

Comment 19

•

6 years ago

Added to https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript

Keywords: dev-doc-needed → dev-doc-complete

Ryan VanderMeulen [:RyanVM]

Updated

•

6 years ago

Blocks: 999003