[webkit-reviews] review granted: [Bug 209775] [ECMA-402] Implement Intl.ListFormat (after ICU ulistfmt_openForType gets stable) : [Attachment 412239] Patch

[bugzilla-daemon at webkit.org]

Ross Kirsling <ross.kirsling at sony.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 209775: [ECMA-402] Implement Intl.ListFormat (after ICU
ulistfmt_openForType gets stable)
https://bugs.webkit.org/show_bug.cgi?id=209775

Attachment 412239: Patch

https://bugs.webkit.org/attachment.cgi?id=412239&action=review




--- Comment #14 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 412239
  --> https://bugs.webkit.org/attachment.cgi?id=412239
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.