Bug 89963 – [Platform] Implement Date Time format parser

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

To implement input type "time", we would like to have date time format text (Unicode TR35 LDML) parser for handling localized date time format.

Definition of date format pattern in LDML is found in
http://unicode.org/reports/tr35/tr35-6.html#Date_Format_Patterns

Created attachment 149499 [details]
Patch 1

Comment on attachment 149499 [details]
Patch 1

Could you review this patch?
Thanks in advance.

* This patch is part of time input type implementation.
* Usage of DateTimeFormat class is found in WebCore/html/shadow/DateTimeEditElement.cpp in attachment of https://bugs.webkit.org/show_bug.cgi?id=88970

[Kent Tamura]

Comment on attachment 149499 [details]
Patch 1

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

> Source/WebCore/ChangeLog:9
> +        This patch introduces Unicode TR35 LDML date time format parser for
> +        implementing input type "time" with localized time format.

You had better mention ICU support, and plans for other platforms such as Windows and OS X.

> Source/WebCore/platform/text/DateTimeFormat.h:34
> +#if ENABLE(TIME_INPUT_FIELDS)

I'm not sure if ENABLE_TIME_INPUT_FIELDS is a nice concrete name.  If we made super-verbose one, it would be ENABLE_INPUT_TYPE_TIME_INLINE_EDITABLE_MULTIPLE_FIELDS?

[Kent Tamura]

Comment on attachment 149499 [details]
Patch 1

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?

Created attachment 149897 [details]
Patch 2

[WebKit Review Bot]

Attachment 149897 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit/chromium/tests/DateTimeFormatTest.cpp:33:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/chromium/tests/DateTimeFormatTest.cpp:344:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/text/DateTimeFormat.cpp:33:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/text/DateTimeFormat.h:34:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 149906 [details]
Patch 3

Comment on attachment 149906 [details]
Patch 3

Could you review this patch?
Thanks in advance.

* Local build on CR-Linux
* Local build on CR-Mac
* Local build on Mac

= Changes since the last review =
* Make DateTimeFormat::parse static
* Update makefiles for adding DateTimeFormat.{cpp,h}

Created attachment 150060 [details]
Patch 4

Comment on attachment 150060 [details]
Patch 4

Could you review this patch?
Thanks in advance.

* Local build on CR-Linux
* Local build on CR-Mac
* Local build on Mac

= Changes since the last review =
* Make DateTimeFormat::parse static
* Update makefiles for adding DateTimeFormat.{cpp,h}
* Update copyright text

[Kent Tamura]

Comment on attachment 150060 [details]
Patch 4

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

> Source/WebCore/platform/text/DateTimeFormat.cpp:30
> +#include "config.h"
> +
> +#if ENABLE(INPUT_TYPE_TIME_MULTIPLE_FIELDS)
> +
> +#include "DateTimeFormat.h"

Blank lines are not needed.

> Source/WebCore/platform/text/DateTimeFormat.cpp:99
> +    if (ch >= 'A' && ch <= 'Z')
> +        return upperCaseToFieldTypeMap[ch - 'A'];
> +
> +    if (ch >= 'a' && ch <= 'z')

Use isASCIILower() and isASCIIUpper defined in wtf/ASCIICType.h

> Source/WebCore/platform/text/DateTimeFormat.h:106
> +        FormatTypeICU,

Do we need to support ICU format?  You assume ICU provides LDML-compatible format strings in another bug, right?

Created attachment 150090 [details]
Patch 5

Comment on attachment 150090 [details]
Patch 5

Could you review this patch?
Thanks in advance.

= Changes since last review =
* Remove blank lines around #if ENABLE(...)
* Use isASCIIUpperCase and isASCIILowerCase
* Remove ICU syntax support

[Kent Tamura]

Comment on attachment 150090 [details]
Patch 5

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

> Source/WebCore/platform/text/DateTimeFormat.cpp:219
> +        default:
> +            LOG_ERROR("Invliad state=%d", state);
> +            ASSERT_NOT_REACHED();
> +            return false;

Please remove default: section.  It's harmful.

> Source/WebCore/platform/text/DateTimeFormat.cpp:247
> +    default:
> +        LOG_ERROR("Invliad state=%d", state);
> +        ASSERT_NOT_REACHED();
> +        return false;

ditto.
Please add ASSERT_NOT_REACHED() at outside of the switch statement.

> Source/WebCore/platform/text/DateTimeFormat.h:100
> +        virtual void visitField(FieldType, int) = 0;

Please add argument name for 'int', or add a comment for it.

> Source/WebKit/chromium/tests/DateTimeFormatTest.cpp:196
> +        virtual void visitField(FieldType fieldType, int count)

add OVERRIDE

> Source/WebKit/chromium/tests/DateTimeFormatTest.cpp:201
> +        virtual void visitLiteral(const String& string)

ditto.

Created attachment 150096 [details]
Patch 6

Comment on attachment 150096 [details]
Patch 6

Could you review this patch?
Thanks in advance.

= Changes since the last change =
* Remove default case from DateTimeFormat::parse().
* Add OVERRIDE to DateTimeFormatTest::TokenHandler

[Kent Tamura]

Comment on attachment 150096 [details]
Patch 6

ok

Comment on attachment 150096 [details]
Patch 6

Clearing flags on attachment: 150096

Committed r121525: <http://trac.webkit.org/changeset/121525>

All reviewed patches have been landed.  Closing bug.