If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsICollation implementations should not depend on nsIPlatformCharset

RESOLVED FIXED in Firefox 55

Status

()

Core
Internationalization
--
enhancement
RESOLVED FIXED
4 years ago
28 days ago

People

(Reporter: hsivonen, Assigned: m_kato, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
Severity: normal → enhancement
(Reporter)

Comment 1

2 years ago
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@hsivonen.fi
(Assignee)

Updated

2 years ago
Depends on: 1214169
(Assignee)

Updated

2 years ago
Duplicate of this bug: 346644
(Assignee)

Comment 3

a year ago
Created attachment 8763186 [details]
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/
Attachment #8763186 - Flags: review?(hsivonen)
Attachment #8763187 - Flags: review?(hsivonen)
(Assignee)

Comment 4

a year ago
Created attachment 8763187 [details]
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/
(Reporter)

Comment 5

a year ago
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?
(Reporter)

Comment 6

a year ago
needinfo for questions in the previous comment.
Flags: needinfo?(m_kato)
(Assignee)

Comment 7

a year ago
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?
(Assignee)

Comment 8

a year ago
clear ni by comment #7
Flags: needinfo?(m_kato)
(Reporter)

Comment 9

a year ago
(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.
(Reporter)

Comment 10

a year ago
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)
(Reporter)

Updated

a year ago
Attachment #8763187 - Flags: review?(hsivonen)
(Assignee)

Comment 11

7 months ago
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
(Reporter)

Comment 12

6 months ago
(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)
(Reporter)

Comment 13

6 months ago
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: 1261841
(Assignee)

Comment 14

6 months ago
(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.
(Assignee)

Comment 15

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20ff918e5ded49cd2a292f29bfd7e1d60899d4e7
(Reporter)

Comment 16

6 months ago
Created attachment 8849902 [details] [diff] [review]
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.
(Assignee)

Updated

6 months ago
Attachment #8763186 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8763187 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

6 months ago
mozreview-review
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+
(Reporter)

Comment 21

6 months ago
mozreview-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+
(Reporter)

Comment 22

6 months ago
mozreview-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+

Comment 23

6 months ago
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

Comment 24

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5148d608e50f
https://hg.mozilla.org/mozilla-central/rev/3ccff1229648
https://hg.mozilla.org/mozilla-central/rev/bc2abb9359fc
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

6 months ago
Flags: needinfo?(m_kato)

Updated

a month ago
Depends on: 1391628
Assignee: nobody → m_kato
You need to log in before you can comment on or make changes to this bug.