[webkit-reviews] review denied: [Bug 89963] [Platform][DateTime][Forms] Implement Date Time format parser : [Attachment 149499] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 89963: [Platform][DateTime][Forms] Implement Date Time format parser
https://bugs.webkit.org/show_bug.cgi?id=89963

Attachment 149499: Patch 1
https://bugs.webkit.org/attachment.cgi?id=149499&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149499&action=review


> Source/WebCore/ChangeLog:3
> +	   [Platform][DateTime][Forms] Implement Date Time format parser

nit: three [] looks too busy.  It seems they are unnecessary.

> Source/WebCore/platform/text/DateTimeFormat.h:49
> +class DateTimeFormat {

This class represents a parser.  So the name should be 'DateTimeFormatParser'
or something.

> Source/WebCore/platform/text/DateTimeFormat.h:113
> +	   FormatTypeUnicode,

FormatTypeUnicode is confusing because ICU is for Unicode.  maybe
FormatTypeLDML.

> Source/WebCore/platform/text/DateTimeFormat.h:120
> +	   virtual void gotField(FieldType, int) = 0;
> +	   virtual void gotLiteral(const String&) = 0;

To me, 'got' sounds strange.  handleField, visitField, didVisitField, or
something might be better.

> Source/WebCore/platform/text/DateTimeFormat.h:126
> +    explicit DateTimeFormat(TokenHandler&, FormatType = FormatTypeUnicode);
> +
> +    // Returns true if succeeded, false if failed.
> +    bool parse(const String&) const;

Do we need to create an object of DateTimeFormat?

static bool parse(const String& format, TokenHandler&, FormatType)
isn't enough?