Summary: | [Platform] Implement Date Time format parser | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||
Component: | Platform | Assignee: | yosin | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | rakuco, tkent, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
URL: | http://unicode.org/reports/tr35/tr35-6.html#Date_Format_Patterns | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 88970, 90236 | ||||||||||||||||
Attachments: |
|
Description
yosin
2012-06-26 02:37:02 PDT
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. |