Bug 89963

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

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?

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

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

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

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

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.