[webkit-reviews] review granted: [Bug 229313] Intl.DateTimeFormat incorrectly parses patterns with 'h' literal : [Attachment 436610] 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 229313: Intl.DateTimeFormat incorrectly parses patterns with 'h' literal
https://bugs.webkit.org/show_bug.cgi?id=229313

Attachment 436610: Patch

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




--- Comment #4 from Ross Kirsling <ross.kirsling at sony.com> ---
Comment on attachment 436610
  --> https://bugs.webkit.org/attachment.cgi?id=436610
Patch

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

r=me with comments (about the comments :P)

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:313
> +    // 'ICU''s change' is "ICU's change" literal text, but even if we split
this text into two literal texts,

The use of quotes on this line confused me at first—it would be easier to
understand if you quote the first part too, maybe like `'ICU''s change'`.

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:346
> +    // 2. Literal text, which is output as-is when formatting, and must
closely match when
> +    //    parsing. Literal text can include: Any characters other than A..Z
and a..z,
> +    //    including spaces and punctuation. Any text between single vertical
quotes ('xxxx'),
> +    //    which may include A..Z and a..z as literal text. Two adjacent
single vertical quotes
> +    //    (''), which represent a literal single quote, either inside or
outside quoted text.

This is okay but since the last three sentences are intended as a bullet list,
it seems like you should write it as one.