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

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.

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/

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/

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?

needinfo for questions in the previous comment.

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?

clear ni by comment #7

(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 on attachment 8763186 [details] Bug 943287 - Part 1. Rename nsICollationMac to nsICollationICU. Unsetting review request, since a revised patch is expected.

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

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

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.

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

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 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 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 on attachment 8850309 [details] Bug 943287 - Part 1. Rename nsCollation.cpp to nsCollationFactory.cpp. https://reviewboard.mozilla.org/r/122952/#review125374

Pushed by m_kato@ga2.so-net.ne.jp: 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

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