[webkit-reviews] review granted: [Bug 209776] [ECMA-402] Intl.DateTimeFormat dateStyle/timeStyle missing in WebKit : [Attachment 402470] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 209776: [ECMA-402] Intl.DateTimeFormat dateStyle/timeStyle missing in
WebKit
https://bugs.webkit.org/show_bug.cgi?id=209776

Attachment 402470: Patch

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




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 402470
  --> https://bugs.webkit.org/attachment.cgi?id=402470
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402470&action=review

I probably should’ve waited to review until the test results from EWS were in.
R=me assuming everything passes.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:641
> +	   auto toUDateFormatStyle = [](const String& style) {

We often use the word parse for functions that interpret a string and return a
parsed form of it. I think it would be good to name this parse instead of to.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:642
> +	       if (style.isNull())

I don’t think we need to optimize the null case. I suggest leaving out this if
statement.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:644
> +	       if (style == "full"_s)

I am not sure that String == ASCIILiteral is properly implemented/optimized. In
the past, in fact, sometimes it involved  creating and then destroying a
String. Could you check on this?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:680
> +	   UDateTimePatternGenerator* generator =
udatpg_open(dataLocaleWithExtensions.data(), &status);

auto?

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:748
> +    auto toDateTimeStyle = [](const String& style) {

Ditto.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:749
> +	   if (style.isNull())

Ditto.