RESOLVED FIXED 89963
[Platform] Implement Date Time format parser
https://bugs.webkit.org/show_bug.cgi?id=89963
Summary [Platform] Implement Date Time format parser
yosin
Reported 2012-06-26 02:37:02 PDT
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
Attachments
Patch 1 (29.01 KB, patch)
2012-06-26 02:54 PDT, yosin
no flags
Patch 2 (36.44 KB, patch)
2012-06-28 01:24 PDT, yosin
no flags
Patch 3 (37.96 KB, patch)
2012-06-28 02:19 PDT, yosin
no flags
Patch 4 (37.22 KB, patch)
2012-06-28 18:33 PDT, yosin
no flags
Patch 5 (36.13 KB, patch)
2012-06-28 23:26 PDT, yosin
no flags
Patch 6 (35.98 KB, patch)
2012-06-29 00:11 PDT, yosin
no flags
yosin
Comment 1 2012-06-26 02:54:29 PDT
yosin
Comment 2 2012-06-26 02:57:06 PDT
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 3 2012-06-26 19:29:40 PDT
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 4 2012-06-26 21:18:20 PDT
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?
yosin
Comment 5 2012-06-28 01:24:41 PDT
WebKit Review Bot
Comment 6 2012-06-28 01:26:59 PDT
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.
yosin
Comment 7 2012-06-28 02:19:24 PDT
yosin
Comment 8 2012-06-28 02:50:18 PDT
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}
yosin
Comment 9 2012-06-28 18:33:30 PDT
yosin
Comment 10 2012-06-28 18:34:55 PDT
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 11 2012-06-28 18:53:18 PDT
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?
yosin
Comment 12 2012-06-28 23:26:34 PDT
yosin
Comment 13 2012-06-28 23:28:21 PDT
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 14 2012-06-29 00:04:56 PDT
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.
yosin
Comment 15 2012-06-29 00:11:09 PDT
yosin
Comment 16 2012-06-29 00:12:54 PDT
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 17 2012-06-29 00:21:18 PDT
Comment on attachment 150096 [details] Patch 6 ok
yosin
Comment 18 2012-06-29 00:27:03 PDT
Comment on attachment 150096 [details] Patch 6 Clearing flags on attachment: 150096 Committed r121525: <http://trac.webkit.org/changeset/121525>
yosin
Comment 19 2012-06-29 00:27:11 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.