Closed
Bug 943287
Opened 11 years ago
Closed 7 years ago
nsICollation implementations should not depend on nsIPlatformCharset
Categories
(Core :: Internationalization, enhancement)
Core
Internationalization
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.
Updated•11 years ago
|
Severity: normal → enhancement
Reporter | ||
Comment 1•9 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
Assignee | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
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•8 years 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•8 years ago
|
||
needinfo for questions in the previous comment.
Flags: needinfo?(m_kato)
Assignee | ||
Comment 7•8 years 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?
Reporter | ||
Comment 9•8 years 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•8 years 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•8 years ago
|
Attachment #8763187 -
Flags: review?(hsivonen)
Assignee | ||
Comment 11•7 years 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•7 years 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•7 years 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: encoding_rs
Assignee | ||
Comment 14•7 years 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20ff918e5ded49cd2a292f29bfd7e1d60899d4e7
Reporter | ||
Comment 16•7 years ago
|
||
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•7 years ago
|
Attachment #8763186 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8763187 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 20•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(m_kato)
Updated•7 years ago
|
Assignee: nobody → m_kato
You need to log in
before you can comment on or make changes to this bug.
Description
•