Summary: | [ECMA-402] Implement Intl.ListFormat | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shane Carr <sffc> | ||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andy, annulen, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, mmaxfield, msaboff, ross.kirsling, ryuan.choi, saam, sergio, sffc, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 213425 | ||||||||||
Attachments: |
|
Description
Shane Carr
2020-03-30 14:43:21 PDT
Grepped ICU code with listPatterns and maybe we should use ulistformatter.h Note, 1. ulistformatter.h has almost 1:1 APIs for the Intl.ListFormat, so maybe just piping. 2. formatToParts may be implemented via UFormattedValue, so we should require ICU 64~. https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ulistformatter_8h.html#a7635c9427da25367b659a9369c2a237d > This API may be changed in the future versions and was introduced in ICU 67 Hmm, it seems that this APIs are not stable state right now. ulistfmt_open is stable since ICU 55, but it only supports wide conjunction ("and"). The C++ API had supported other types for some time, but it was not added to the C API until ICU 67. We are not using C++ APIs due to strong ABI compatibility requirement to use the system ICU (This is my understanding). We can implement a part of Intl.ListFormat (type "conjunction" case), but it is a bit worrying about whether this breaks feature detections. For example, if the web uses, if (Intl.ListFormat) { ... } and, if the code uses disjunction style, we will have a bad time. I think making ulistfmt_openForType stable API in ICU side is blocking us for now. @Ross what do you think of? (In reply to Yusuke Suzuki from comment #7) > @Ross what do you think of? Yeah, I agree with you. I suppose we could implement conjunction-only behind a flag but it would feel wrong to ship without disjunction. Talked with Michael offline, and we agree that we will implement it once ICU ulistfmt_openForType gets stable. Created attachment 412234 [details]
Patch
Created attachment 412238 [details]
Patch
Comment on attachment 412238 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412238&action=review > Source/JavaScriptCore/runtime/IntlListFormat.h:45 > +struct UListFormatterDeleter { > + JS_EXPORT_PRIVATE void operator()(UListFormatter*); > +}; Do not use ICUDeleter since it requires ulistfmt_close, and we do not want to include <unicode/ulistformatter.h> in headers (since we enable draft APIs). Created attachment 412239 [details]
Patch
Comment on attachment 412239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412239&action=review r=me overall, but I wish we didn't have to scatter the HAVEs quite so much. > Source/JavaScriptCore/runtime/IntlListFormat.cpp:54 > +void UListFormatterDeleter::operator()(UListFormatter* formatter) I suppose you should add your explanation about this as a comment. > Source/JavaScriptCore/runtime/IntlListFormat.cpp:124 > +#if HAVE(ICU_U_LIST_FORMATTER) I asked about this for DisplayNames too (bug 209779 comment 24), but it feels sad to have to #if all of these functions if the ListFormat constructor won't even be exposed in this situation... > Source/JavaScriptCore/runtime/IntlListFormat.cpp:156 > + throwTypeError(globalObject, scope, "Failed to initialize Intl.ListFormat since used feature is not supported in the linked ICU version"_s); nit: s/used/this/ probably reads better > JSTests/stress/intl-listformat.js:22 > +if (typeof Intl.ListFormat !== 'undefined') { Seems like you could just return at the very top, in lieu of a //@ line. Comment on attachment 412239 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=412239&action=review Thanks! >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:54 >> +void UListFormatterDeleter::operator()(UListFormatter* formatter) > > I suppose you should add your explanation about this as a comment. Added >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:124 >> +#if HAVE(ICU_U_LIST_FORMATTER) > > I asked about this for DisplayNames too (bug 209779 comment 24), but it feels sad to have to #if all of these functions if the ListFormat constructor won't even be exposed in this situation... I think this way minifies the if-def changes. For example, if we do not expose ListFormat at all, we need to have this ifdef in, VM.cpp IntlListFormatConstructor.h / IntlListFormatConstructor.cpp IntlListFormatPrototype.h / IntlListFormatPrototype.cpp IntlListFormat.h / IntlListFormat.cpp IntlObject.cpp So, we scatters this ifdef in many files. On the other hand, in this way, we need ifdef only in IntlListFormat.cpp and IntlObject.cpp. >> Source/JavaScriptCore/runtime/IntlListFormat.cpp:156 >> + throwTypeError(globalObject, scope, "Failed to initialize Intl.ListFormat since used feature is not supported in the linked ICU version"_s); > > nit: s/used/this/ probably reads better Fixed. >> JSTests/stress/intl-listformat.js:22 >> +if (typeof Intl.ListFormat !== 'undefined') { > > Seems like you could just return at the very top, in lieu of a //@ line. Unfortunately, JS cannot `return` in top-level... I'll extract this to `function test`, and call it when Intl.ListFormat is defined. Committed r268956: <https://trac.webkit.org/changeset/268956> |