WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch 2
(36.44 KB, patch)
2012-06-28 01:24 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 3
(37.96 KB, patch)
2012-06-28 02:19 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 4
(37.22 KB, patch)
2012-06-28 18:33 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 5
(36.13 KB, patch)
2012-06-28 23:26 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 6
(35.98 KB, patch)
2012-06-29 00:11 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
yosin
Comment 1
2012-06-26 02:54:29 PDT
Created
attachment 149499
[details]
Patch 1
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
Created
attachment 149897
[details]
Patch 2
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
Created
attachment 149906
[details]
Patch 3
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
Created
attachment 150060
[details]
Patch 4
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
Created
attachment 150090
[details]
Patch 5
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
Created
attachment 150096
[details]
Patch 6
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.
Top of Page
Format For Printing
XML
Clone This Bug