Summary: | [Platform] Implement functions for localized time format information | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||
Component: | Platform | Assignee: | yosin | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dglazkov, tkent, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 89967 | ||||||||||||||||
Bug Blocks: | 88970 | ||||||||||||||||
Attachments: |
|
Description
yosin
2012-06-26 03:00:24 PDT
Created attachment 149716 [details]
Patch 1
Comment on attachment 149716 [details]
Patch 1
Could you review this patch?
Thanks in advance.
Comment on attachment 149716 [details] Patch 1 View in context: https://bugs.webkit.org/attachment.cgi?id=149716&action=review > Source/WebCore/ChangeLog:10 > + 1. localizedTimeFormatText() > + 2. localizedShortTimeFormatText() Why do you need both of non-short and short formats? > Source/WebCore/ChangeLog:11 > + 2. periodLabels() The name periodLabels() is too short and not concrete. How about timeAMPMabels(), or separate it into timeAMLabel() and timePMLabel()? > Source/WebKit/chromium/ChangeLog:32 > +2012-06-27 Yoshifumi Inoue <yosin@chromium.org> > + > + Need a short description and bug URL (OOPS!) > + > + Reviewed by NOBODY (OOPS!). > + > + * WebKit.gypi: > + Please remove it. Created attachment 149872 [details]
Patch 2
Comment on attachment 149872 [details]
Patch 2
Could you review this patch?
Thanks in advance.
= Changes since last review =
* Replace INPUT_TIME_FIELDS => INPUT_TYPE_TIME_MULTIPLE_FIELDS
* Replace periodLabels => timeAMPMLabels
** Thanks for suggestion. This is more explicit.
** For consistency of month names and weekday names, returning vector seems to be better.
* Having TimeFormat and ShortTimeFormat for displaying only two fields hour and minute when step >= 60. There is no way to remove second field from "h:m:s" pattern string. We don't know whether ":" after "m" belongs minute or second field.
(In reply to comment #5) > * Having TimeFormat and ShortTimeFormat for displaying only two fields hour and minute when step >= 60. There is no way to remove second field from "h:m:s" pattern string. We don't know whether ":" after "m" belongs minute or second field. Please write this in ChangeLog. Comment on attachment 149872 [details] Patch 2 Attachment 149872 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13099932 Comment on attachment 149872 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=149872&action=review > Source/WebCore/platform/text/LocaleICU.cpp:529 > + m_mediumTimeFormat = openDateFormat(UDAT_MEDIUM, UDAT_NONE); > + m_localizedTimeFormatText = getDateFormatPattern(m_mediumTimeFormat); > + > + m_shortTimeFormat = openDateFormat(UDAT_SHORT, UDAT_NONE); > + m_localizedShortTimeFormatText = getDateFormatPattern(m_shortTimeFormat); WebCore::localized*TimeFormatText() ask LDML format strings, but they are ICU format strings. Probably you have an assumption that ICU returns LDML-compatible formt strings. We need a comment. Created attachment 149876 [details]
Patch 3
Created attachment 149877 [details]
Patch 4
Comment on attachment 149877 [details]
Patch 4
Could you review this patch?
Thanks in advance.
= Changes since last review =
* Add a reason why we have both localizedDateFormat and localizedShortDateFormat
* Add LocalizedDateICUTest.cpp (it was in Patch 1, but not in Patch 2)
Comment on attachment 149877 [details] Patch 4 r- because of Comment #8 Created attachment 149883 [details]
Patch 5
Comment on attachment 149883 [details] Patch 5 Could you review this patch? Thanks in advance. = Changes since the last review = * Put comment ICU/LDML pattern compatibility as pointed by comment#8 into LocaleICU.cpp Comment on attachment 149883 [details]
Patch 5
missing LocalizedDateICUTest.cpp
Created attachment 149886 [details]
Patch 6
Comment on attachment 149886 [details]
Patch 6
Could you review this patch?
Thanks in advance and sorry for redundant iteration.
= Changes since the last review =
* Include LocalizedDateICUTest.cpp
Comment on attachment 149886 [details]
Patch 6
ok
Comment on attachment 149886 [details] Patch 6 Clearing flags on attachment: 149886 Committed r121415: <http://trac.webkit.org/changeset/121415> All reviewed patches have been landed. Closing bug. |