Closed Bug 943287 Opened 6 years ago Closed 3 years ago

nsICollation implementations should not depend on nsIPlatformCharset

Categories

(Core :: Internationalization, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hsivonen, Assigned: m_kato, Mentored)

References

Details

Attachments

(4 files, 2 obsolete files)

Either nsICollation should be implemented on top of ICU or it should use Unicode system APIs exclusively on Windows and Unix.
Severity: normal → enhancement
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.
Mentor: hsivonen
Depends on: 1214169
Duplicate of this bug: 346644
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/
Attachment #8763186 - Flags: review?(hsivonen)
Attachment #8763187 - Flags: review?(hsivonen)
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.
Flags: needinfo?(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?
clear ni by comment #7
Flags: needinfo?(m_kato)
(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.
Attachment #8763186 - Flags: review?(hsivonen)
Attachment #8763187 - Flags: review?(hsivonen)
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.
Flags: needinfo?(m_kato)
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.
Blocks: encoding_rs
(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.
Attachment #8763186 - Attachment is obsolete: true
Attachment #8763187 - Attachment is obsolete: true
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
Attachment #8850311 - Flags: review?(hsivonen) → review+
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!
Attachment #8850310 - Flags: review?(hsivonen) → review+
Comment on attachment 8850309 [details]
Bug 943287 - Part 1. Rename nsCollation.cpp to nsCollationFactory.cpp.

https://reviewboard.mozilla.org/r/122952/#review125374
Attachment #8850309 - Flags: review?(hsivonen) → review+
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
Flags: needinfo?(m_kato)
Assignee: nobody → m_kato
You need to log in before you can comment on or make changes to this bug.