Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Description • 8 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 2 • 8 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 5 • 8 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 6 • 8 years ago
mozreview-review

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 9 • 8 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 13 • 8 years ago
mozreview-review

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 15 • 8 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 16 • 8 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 18 • 8 years ago

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.

	Jonathan Kew [:jfkthame]
	Comment 19 • 8 years ago
mozreview-review

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?

	Jonathan Kew [:jfkthame]
	Comment 20 • 8 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 22 • 8 years ago

(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.

	Jonathan Kew [:jfkthame]
	Comment 23 • 8 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 25 • 8 years ago

yeah, removed in rev9 :)

	Pulsebot
	Comment 26 • 8 years ago

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de2b0d90e2da
Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale. r=jfkthame

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 27 • 8 years ago

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

	Pulsebot
	Comment 28 • 8 years ago

Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5a9559ded976
Followup to fix wError bustage that prompted a CLOSED TREE a=bustage

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 8 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 31 • 8 years ago

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

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

	Jonathan Kew [:jfkthame]
	Comment 32 • 8 years ago

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

	Jonathan Kew [:jfkthame]
	Comment 33 • 8 years ago

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.

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 35 • 8 years ago

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

	Zibi Braniecki [:zbraniecki][:gandalf] Assignee
	Comment 36 • 8 years ago

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

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

	Jonathan Kew [:jfkthame]
	Comment 37 • 8 years ago
mozreview-review

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. :)

	Pulsebot
	Comment 38 • 8 years ago

Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fad6f3a8b03
Migrate nsCollation::CreateCollection to use LocaleService::GetAppLocale. r=jfkthame

	Carsten Book [:Tomcat]
	Comment 39 • 8 years ago
bugherder

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