943287 - nsICollation implementations should not depend on nsIPlatformCharset

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Either nsICollation should be implemented on top of ICU or it should use Unicode system APIs exclusively on Windows and Unix.

Comment 1

[Henri Sivonen (:hsivonen)]

We already have an ICU-backed implementation in intl/locale/mac/nsCollationMacUC.cpp. We should adapt that code to be used with the built-in ICU on all platforms.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 943287 - Part 1. Rename nsICollationMac to nsICollationICU.

To share code on both macOS and UNIX, rename source file name and move it to intl/locale/

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Bug 943287 - Part 2. Use nsICollation of ICU version when ICU is built on UNIX.

Use ICU implemattio on UNIX with ICU support.  We don't turn on ICU + Windows yet because ucol.h (uset.h) seems to requrie C++ exception support on MSVC.

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

Comment 5

[Henri Sivonen (:hsivonen)]

https://reviewboard.mozilla.org/r/59482/#review56634

s/implemattio/implementation/ in the commit message.

::: intl/build/nsI18nModule.cpp:84
(Diff revision 1)
>      { &kNS_LANGUAGEATOMSERVICE_CID, false, nullptr, nsLanguageAtomServiceConstructor },
>      { &kNS_PLATFORMCHARSET_CID, false, nullptr, nsPlatformCharsetConstructor },
> -#ifdef XP_WIN
> +
> +#if defined(XP_WIN)
>      { &kNS_COLLATION_CID, false, nullptr, nsCollationWinConstructor },
> +#elif defined(ENABLE_INTL_API) || defined(USE_MAC_LOCALE)

Is the ` || defined(USE_MAC_LOCALE)` part actually a supported configuration without `defined(ENABLE_INTL_API)`? That is, does this code actually build on Mac with the Intl API turned off?

If this doesn't compile without the Intl API turned off on Mac, let's not check `USE_MAC_LOCALE`.

(Same thing in multiple places.)

::: intl/locale/mac/moz.build:13
(Diff revision 1)
>      'nsDateTimeFormatMac.cpp',
>      'nsMacCharset.cpp',
>  ]
>  
> +SOURCES += [
> +    '../nsCollationICU.cpp',

It's seem weird to use `../` in the `moz.build` files under `mac/` and `unix/`.

Is there a reason not to hoist the logic to the `moz.build` of the parent where the `.cpp` file itself lives to keep the `.cpp` and the `moz.build` stuff that goes with it in the same directory?

Comment 6

[Henri Sivonen (:hsivonen)]

needinfo for questions in the previous comment.

Comment 7

[Makoto Kato [:m_kato]]

https://reviewboard.mozilla.org/r/59482/#review56634

> Is the ` || defined(USE_MAC_LOCALE)` part actually a supported configuration without `defined(ENABLE_INTL_API)`? That is, does this code actually build on Mac with the Intl API turned off?
> 
> If this doesn't compile without the Intl API turned off on Mac, let's not check `USE_MAC_LOCALE`.
> 
> (Same thing in multiple places.)

I think that this isn't support --without-intl on Mac.   OK, I will fix.

> It's seem weird to use `../` in the `moz.build` files under `mac/` and `unix/`.
> 
> Is there a reason not to hoist the logic to the `moz.build` of the parent where the `.cpp` file itself lives to keep the `.cpp` and the `moz.build` stuff that goes with it in the same directory?

It is no reason to use moz.build on per platform directory.  Should I merge <platform>/moz.build into ./moz.build?

Comment 8

[Makoto Kato [:m_kato]]

clear ni by comment #7

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Makoto Kato [:m_kato] from comment #7)
> https://reviewboard.mozilla.org/r/59482/#review56634
> 
> > Is the ` || defined(USE_MAC_LOCALE)` part actually a supported configuration without `defined(ENABLE_INTL_API)`? That is, does this code actually build on Mac with the Intl API turned off?
> > 
> > If this doesn't compile without the Intl API turned off on Mac, let's not check `USE_MAC_LOCALE`.
> > 
> > (Same thing in multiple places.)
> 
> I think that this isn't support --without-intl on Mac.   OK, I will fix.
> 
> > It's seem weird to use `../` in the `moz.build` files under `mac/` and `unix/`.
> > 
> > Is there a reason not to hoist the logic to the `moz.build` of the parent where the `.cpp` file itself lives to keep the `.cpp` and the `moz.build` stuff that goes with it in the same directory?
> 
> It is no reason to use moz.build on per platform directory.  Should I merge
> <platform>/moz.build into ./moz.build?

Yes, please. Thanks.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8763186 [details]
Bug 943287 - Part 1. Rename nsICollationMac to nsICollationICU.

Unsetting review request, since a revised patch is expected.

Comment 11

[Makoto Kato [:m_kato]]

Since I landed ICU on all platform.  After landing bug 1329841, we always require ICU.  So I update this fix to use ICU on all platform

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Makoto Kato [:m_kato] (PTO until 3/20) from comment #11)
> Since I landed ICU on all platform.  After landing bug 1329841, we always
> require ICU.  So I update this fix to use ICU on all platform

Well, it looks like we, again, don't have ICU on Android.

Do you have a guess on whether you might land this for non-Android platforms? The platform charset is always UTF-8 on Android, so knowing that legacy nsCollation is Android only would simplify things for bug 1261841.

Comment 13

[Henri Sivonen (:hsivonen)]

I'm going to proceed with bug 1261841 on the assumption that we can move Linux and Windows to ICU and leave the legacy nsCollation to Android only.

Comment 14

[Makoto Kato [:m_kato]]

(In reply to Henri Sivonen (:hsivonen) from comment #12)
> (In reply to Makoto Kato [:m_kato] (PTO until 3/20) from comment #11)
> > Since I landed ICU on all platform.  After landing bug 1329841, we always
> > require ICU.  So I update this fix to use ICU on all platform
> 
> Well, it looks like we, again, don't have ICU on Android.
> 
> Do you have a guess on whether you might land this for non-Android
> platforms? The platform charset is always UTF-8 on Android, so knowing that
> legacy nsCollation is Android only would simplify things for bug 1261841.

Bionic supports C locale only, so nsICollation doesn't work correctly on Android now.  I will send review to you with new fix tomorrow.

Comment 15

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20ff918e5ded49cd2a292f29bfd7e1d60899d4e7

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Rough patch for making the remaining legacy collation code Android only (UTF-8 only)

Not sure if this patch belongs here or in another bug number, but attaching it here at least as a backup if for no other reason.

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Bug 943287 - Part 1. Rename nsCollation.cpp to nsCollationFactory.cpp.

nsCollation.cpp implements nsCollationFactory and nsCollation utils classes.
To remove nsIPlatfromCharset dependency, I would like to remove nsCollaction utils class.  So at first, I would like to rename it to nsCollactionFactory.cpp. (then, nsICollation implementation will be nsCollation.cpp)

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

Comment 18

[Makoto Kato [:m_kato]]

Attachment: Bug 943287 - Part 2. Use ICU version of nsICollation on all platform.

nsICollation for macOS version uses ICU, so we should use it on all platform since nightly and aurora can use ICU on all platform.

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

Comment 19

[Makoto Kato [:m_kato]]

Attachment: Bug 943287 - Part 3. nsICollation fallback version for Android.

Beta and release channel of Android still turn off ICU.  So we should add UTF-8 version of nsICollation for fallback.
Some people want to keep --enale-intl-api=no for faster build on developer environment, so even if non-Android, it will be required.

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

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 8850311 [details]
Bug 943287 - Part 3. nsICollation fallback version for Android.

https://reviewboard.mozilla.org/r/122956/#review125370

r+ provided that the two nits are fixed by calling the obviously locale-unawere functions instead of calling what seems locale-aware but actually isn't anyway.

::: intl/locale/nsCollationAndroid.cpp:45
(Diff revision 1)
> -  }
> -
> -  // convert unicode to charset
> -  char *str1, *str2;
>  
> -  res = mCollation->UnicodeToChar(stringNormalized1, &str1);
> +  *result = strcoll(NS_ConvertUTF16toUTF8(stringNormalized1).get(),

Let's do a strcmp() for clarity, since that's what Bionic does behind the scenes anyway due to no locale support:
https://github.com/android/platform_bionic/blob/76fcad2a6f6b6633c49f4f0b703ef490d2d127fd/libc/upstream-netbsd/lib/libc/string/strcoll.c#L51

::: intl/locale/nsCollationAndroid.cpp:66
(Diff revision 1)
> -    res = mCollation->NormalizeString(stringIn, stringNormalized);
> -    if (NS_FAILED(res))
> -      return res;
> -  } else {
> -    stringNormalized = stringIn;
> -  }
> +    ToLowerCase(stringNormalized);
> +  }
> +
> +  // call strxfrm to generate a key
> +  NS_ConvertUTF16toUTF8 str(stringNormalized);
> +  size_t len = strxfrm(nullptr, str.get(), 0) + 1;

Let's just to a memcpy() for clarity, because that's what Android's strxfrm() does behind the scenes, since there's no locale support:
https://github.com/android/platform_bionic/blob/76fcad2a6f6b6633c49f4f0b703ef490d2d127fd/libc/upstream-netbsd/lib/libc/string/strxfrm.c#L36

Comment 21

[Henri Sivonen (:hsivonen)]

Comment on attachment 8850310 [details]
Bug 943287 - Part 2. Use ICU version of nsICollation on all platform.

https://reviewboard.mozilla.org/r/122954/#review125372

Looks good. Thank you!

Comment 22

[Henri Sivonen (:hsivonen)]

Comment on attachment 8850309 [details]
Bug 943287 - Part 1. Rename nsCollation.cpp to nsCollationFactory.cpp.

https://reviewboard.mozilla.org/r/122952/#review125374

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5148d608e50f
Part 1. Rename nsCollation.cpp to nsCollationFactory.cpp. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccff1229648
Part 2. Use ICU version of nsICollation on all platform. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc2abb9359fc
Part 3. nsICollation fallback version for Android. r=hsivonen

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5148d608e50f
https://hg.mozilla.org/mozilla-central/rev/3ccff1229648
https://hg.mozilla.org/mozilla-central/rev/bc2abb9359fc