Bug 89965

Summary: [Platform] Implement functions for localized time format information
Product: WebKit Reporter: yosin
Component: PlatformAssignee: 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 Flags
Patch 1
none
Patch 2
none
Patch 3
none
Patch 4
none
Patch 5
none
Patch 6 none

yosin
Reported 2012-06-26 03:00:24 PDT
For implementing "time" input type, we would like to have following functions: * localizedTimeFormatText() returns time format with second, e.g. "h:mm:ss a" * localizedShortTimeFormatText() returns time format without second, e.g. "h:mm a" * periosLabels() returns localized strings of AM/PM.
Attachments
Patch 1 (14.81 KB, patch)
2012-06-27 03:30 PDT, yosin
no flags
Patch 2 (9.54 KB, patch)
2012-06-27 22:35 PDT, yosin
no flags
Patch 3 (9.83 KB, patch)
2012-06-27 22:58 PDT, yosin
no flags
Patch 4 (15.10 KB, patch)
2012-06-27 23:00 PDT, yosin
no flags
Patch 5 (10.02 KB, patch)
2012-06-27 23:37 PDT, yosin
no flags
Patch 6 (15.28 KB, patch)
2012-06-27 23:46 PDT, yosin
no flags
yosin
Comment 1 2012-06-27 03:30:28 PDT
yosin
Comment 2 2012-06-27 03:31:44 PDT
Comment on attachment 149716 [details] Patch 1 Could you review this patch? Thanks in advance.
Kent Tamura
Comment 3 2012-06-27 20:28:02 PDT
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.
yosin
Comment 4 2012-06-27 22:35:32 PDT
yosin
Comment 5 2012-06-27 22:40:59 PDT
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.
Kent Tamura
Comment 6 2012-06-27 22:52:17 PDT
(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.
WebKit Review Bot
Comment 7 2012-06-27 22:56:29 PDT
Comment on attachment 149872 [details] Patch 2 Attachment 149872 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13099932
Kent Tamura
Comment 8 2012-06-27 22:56:57 PDT
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.
yosin
Comment 9 2012-06-27 22:58:54 PDT
yosin
Comment 10 2012-06-27 23:00:37 PDT
yosin
Comment 11 2012-06-27 23:03:02 PDT
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)
Kent Tamura
Comment 12 2012-06-27 23:21:44 PDT
Comment on attachment 149877 [details] Patch 4 r- because of Comment #8
yosin
Comment 13 2012-06-27 23:37:04 PDT
yosin
Comment 14 2012-06-27 23:38:24 PDT
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
Kent Tamura
Comment 15 2012-06-27 23:45:10 PDT
Comment on attachment 149883 [details] Patch 5 missing LocalizedDateICUTest.cpp
yosin
Comment 16 2012-06-27 23:46:44 PDT
yosin
Comment 17 2012-06-27 23:47:43 PDT
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
Kent Tamura
Comment 18 2012-06-28 00:27:52 PDT
Comment on attachment 149886 [details] Patch 6 ok
yosin
Comment 19 2012-06-28 00:30:19 PDT
Comment on attachment 149886 [details] Patch 6 Clearing flags on attachment: 149886 Committed r121415: <http://trac.webkit.org/changeset/121415>
yosin
Comment 20 2012-06-28 00:30:26 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.