1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Spin-off from bug 1334772. All callsites of nsCollation::CreateCollation get nsLocaleService::GetApplicationLocale which is an API that returns system locale for all platforms except of Windows where it returns an obsolete "user locale".

We want to migrate it to use the new LocaleService, and in the process, we can turn it to use it implicitly, since there doesn't seem to be any reason for the callsites to ask for a collation in a locale different than the current app locale.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8832746 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

This patch relies on bug 1332207.

It does the following:

 - it removes locale param from CreateCollation - all callsites to this function retrieve it from the same place which we intend to replace with LocaleService, so this patch makes it implicit. If we ever have a need to differentiate, we can re-add it.
 - it centralizes locale retrieval in the common nsCollation, instead of doing this per-platform
 - it removes some initialization code from per-platform implementations to just use the locale from LocaleService
 - it also removes the ConvertLocaletoICU from mac implementation because ICU handles language tags on input just fine ("en-US" vs "en_US").

This both, removes calls to GetApplicationLocale from callsites of nsCollation::CreateCollation, and it removes it from the nsCollation code itself.

side note - I believe we should migrate all of this to use ICU (and we seem to have code for that - mac implementation uses ICU).

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8832746 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8832746 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8832746 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

f? - I'll need a bit of help with mac/win code, but it should be pretty close to what we may want.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8832746 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

https://reviewboard.mozilla.org/r/108966/#review110230

Generally looks like a good direction to be going; some comments below (not a full review yet, but maybe enough to help it move forward a bit further).

::: dom/xslt/xslt/txXPathResultComparator.cpp:32
(Diff revision 3)
>      nsresult rv = init(aLanguage);
>      if (NS_FAILED(rv))
>          NS_ERROR("Failed to initialize txResultStringComparator");
>  }
>  
>  nsresult txResultStringComparator::init(const nsAFlatString& aLanguage)

Did you intend to make us ignore the `aLanguage` parameter here? That looks like it would break support for the 'lang' attribute on xsl:sort.

Fixing this will mean passing the language (or locale) to CreateCollation, so I think you need to restore that parameter in some form (maybe as a string rather than a locale object).

::: intl/locale/mac/nsCollationMacUC.h:38
(Diff revision 3)
>    nsresult CleanUpCollator(void);
>  
>  private:
>    bool mInit;
>    bool mHasCollator;
> -  char* mLocaleICU;
> +  char* mLocale;

While we're touching this, I think it'd be better to use an nsCString rather than a raw pointer here; that will let us get rid of the associated manual memory management.

::: intl/locale/mac/nsCollationMacUC.cpp:129
(Diff revision 3)
> -    rv = localeService->GetApplicationLocale(getter_AddRefs(appLocale));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -    locale = appLocale;
> -  }
>  
> -  rv = ConvertLocaleICU(locale, &mLocaleICU);
> +  mLocale = locale;

This won't currently compile, as it's trying to assign a (16-bit) string to a raw `char*` pointer, which would be bad even if the compiler allowed it.

But if mLocale becomes an nsCString, and the parameter to Initialize becomes an 8-bit string (const nsACString&, maybe) then we should be good.

::: intl/locale/nsCollation.cpp:38
(Diff revision 3)
>    }
>  
> -  inst->Initialize(locale);
> +  nsAutoCString appLocale;
> +  mozilla::intl::LocaleService::GetInstance()->GetAppLocale(appLocale);
> +
> +  inst->Initialize(NS_ConvertUTF8toUTF16(appLocale));

It's probably going to work out cleaner (fewer 8/16 bit conversions) to make the Initialize() method take an 8-bit string, as that's what we're getting from GetAppLocale, and that's also what we'll need to pass to ICU internally.

::: intl/locale/nsICollation.idl:18
(Diff revision 3)
> -     * @param locale
> -     *        The locale for which to create the collation or null to use
> -     *        user preference.
> -     * @return A collation for the given locale.
>       */
> -    nsICollation CreateCollation(in nsILocale locale);
> +    nsICollation CreateCollation();

As noted above, I think we still need a method that allows an explicit locale code to be passed in.

You can probably do something like

    nsICollation CreateCollation([optional] in string locale);

to allow callers to omit it if they just want the app locale.

::: intl/locale/nsICollation.idl:40
(Diff revision 3)
>  
>    // case insensitive collation
>    const long kCollationCaseInSensitive = (kCollationCaseInsensitiveAscii | kCollationAccentInsenstive);
>  
>    // init this interface to a specified locale (should only be called by collation factory)
> -  void initialize(in nsILocale locale);
> +  void initialize(in AString locale);

An 8-bit string type may be better here.

::: intl/locale/unix/nsCollationUnix.cpp:58
(Diff revision 3)
> -    nsPosixLocale::GetPlatformLocale(localeStr, mLocale);
> -
> -    nsCOMPtr <nsIPlatformCharset> platformCharset = do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &res);
> +  nsCOMPtr <nsIPlatformCharset> platformCharset = do_GetService(NS_PLATFORMCHARSET_CONTRACTID, &res);
> -    if (NS_SUCCEEDED(res)) {
> +  if (NS_SUCCEEDED(res)) {
> -      nsAutoCString mappedCharset;
> +    nsAutoCString mappedCharset;
> -      res = platformCharset->GetDefaultCharsetForLocale(localeStr, mappedCharset);
> +    res = platformCharset->GetDefaultCharsetForLocale(locale, mappedCharset);

If we make the parameter to Initialize an 8-bit string (as suggested above), we'll need to add a conversion here for GetDefault...; but this will all go away soon anyhow if we move to the ICU impl everywhere, I expect.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks :jfkthame!

Updated the patch to your feedback.

Because all but one callers don't need to pass the locale, I decided to actually split the CreateCollation into two methods to avoid having to pass something from  C++ for the cases which don't need to pass locale.

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

https://reviewboard.mozilla.org/r/110248/#review111484

There's some nice simplification going on here. :)

Just a few minor things that I think can be cleaned up a little before landing.

::: dom/xslt/xslt/txXPathResultComparator.cpp:43
(Diff revision 5)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    rv = colFactory->CreateCollation(locale, getter_AddRefs(mCollation));
> +    if (aLanguage.IsEmpty()) {
> +      rv = colFactory->CreateCollation(getter_AddRefs(mCollation));
> +    } else {
> +      rv = colFactory->CreateCollationForLocale(NS_ConvertUTF16toUTF8(aLanguage), getter_AddRefs(mCollation));

wrap the long line, please

::: intl/locale/nsCollation.cpp:23
(Diff revision 5)
> -nsresult nsCollationFactory::CreateCollation(nsILocale* locale, nsICollation** instancePtr)
> +nsresult nsCollationFactory::CreateCollation(nsICollation** instancePtr)
>  {
>    // Create a collation interface instance.
>    //
>    nsICollation *inst;
>    nsresult res;
>    
>    res = CallCreateInstance(kCollationCID, &inst);
>    if (NS_FAILED(res)) {
>      return res;
>    }
>  
> +  nsAutoCString appLocale;
> +  mozilla::intl::LocaleService::GetInstance()->GetAppLocale(appLocale);
> +  inst->Initialize(appLocale);
> +
> +  *instancePtr = inst;
> +
> +  return res;
> +}

To reduce duplication, you should be able to replace this with
```
nsresult
nsCollationFactory::CreateCollation(nsICollation** instancePtr)
{
  nsAutoCString appLocale;
  mozilla::intl::LocaleService::GetInstance()->GetAppLocale(appLocale);

  return CreateCollationForLocale(appLocale, instancePtr);
}
```

::: intl/locale/windows/nsCollationWin.cpp:40
(Diff revision 5)
>  
>    nsresult res;
>  
>    mCollation = new nsCollation;
>  
> +  nsAutoString wideLocale = NS_ConvertUTF8toUTF16(locale);

This does a redundant copy (unless the compiler is smart enough to optimize it away), as NS_ConvertUTF8toUTF16 is not actually a helper function (despite how it looks)... it's a subclass of nsAutoString that has a constructor that does the conversion. So here, you're constructing a (temporary) NS_ConvertUTF8toUTF16 and then copying it into the declared nsAutoString variable.

So the proper idiom would be

    NS_ConvertUTF8toUTF16 wideLocale(locale);

which directly creates the desired wide string.

But since locale codes are ASCII-only, you can actually go for the simpler variant

    NS_ConvertASCIItoUTF16 wideLocale(locale);

as you don't need full UTF-8 decoding.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

Thanks!

:jfkthame, can you help me with this one error on Mac - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3b07869cb03&selectedJob=75077983

I'm trying to debug it but may not have enough experience to understand why there's any difference.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah, I think I got it. There's more cleanups we can do in JS code now! Updating the patch

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

I think I fixed it and in the process I fixed a few more occurrences that I was able to find using searchfox.

Here's the diff: https://reviewboard.mozilla.org/r/110248/diff/6-7/

Better safe than sorry, so I'll wait for you to look at them before pushing this.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

https://reviewboard.mozilla.org/r/110248/#review111654

::: intl/locale/tests/unit/test_collation_mac_icu.js:25
(Diff revision 7)
>      var collator = Cc["@mozilla.org/intl/collation-factory;1"].
>        createInstance(Ci.nsICollationFactory).
> -      CreateCollation(localeSvc.newLocale(locale));
> +      CreateCollationForLocale(localeSvc.newLocale(locale));

With the new API, shouldn't you just be passing `locale` (a string) here, rather than creating an nsILocale object?

Comment 20

[Jonathan Kew [:jfkthame]]

Huh, interesting... mozreview reset r+, although I didn't touch the flag, I intended to leave it as r? pending a reply to the above.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Jonathan Kew (:jfkthame) from comment #19)
> With the new API, shouldn't you just be passing `locale` (a string) here,
> rather than creating an nsILocale object?

D'uh, of course I should! Glad I asked for review again :) Fix pushed to MR. I'll run one more try before I land it.

Comment 23

[Jonathan Kew [:jfkthame]]

So then we no longer need the localeSvc at all in that function, right?

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834237 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

yeah, removed in rev9 :)

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de2b0d90e2da
Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale. r=jfkthame

Comment 27

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: bustage-fix.diff

So, apparently we have linter tests that run only on opt builds. Fun.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a9559ded976
Followup to fix wError bustage that prompted a CLOSED TREE a=bustage

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And with the followup fix, builds were busted like https://treeherder.mozilla.org/logviewer.html#?job_id=75285498&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/cddb9e197d81

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834593 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

carrying over the r+ since the fix is very minor.

Comment 32

[Jonathan Kew [:jfkthame]]

Attachment: Make the rv variable debug-only to avoid set-but-not-used warning

:zibi, I think you want to do something like this (though I haven't verified it on try).

Comment 33

[Jonathan Kew [:jfkthame]]

Comment on attachment 8834595 [details] [diff] [review]
Make the rv variable debug-only to avoid set-but-not-used warning

Review of attachment 8834595 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xul/templates/nsXULContentUtils.cpp
@@ +126,4 @@
>          nsCOMPtr<nsICollationFactory> colFactory =
>              do_CreateInstance(NS_COLLATIONFACTORY_CONTRACTID);
>          if (colFactory) {
> +            DebugOnly<nsresult> rv(colFactory->CreateCollation(&gCollation));

Actually, we more commonly write it as

    DebugOnly<nsresult> rv = colFactory->CreateCollation(&gCollation);

So prefer that form, please.

Comment 34

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834593 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

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

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thanks! Landed and triggered a try build for both opt and debug linux64.

Comment 36

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8834593 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

carrying over r+ - the try build is green for opt

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 8834593 [details]
Bug 1335983 - Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale.

https://reviewboard.mozilla.org/r/110454/#review111734

If tryserver is happy, I'm happy. :)

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fad6f3a8b03
Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale. r=jfkthame

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9fad6f3a8b03