Bug 89965

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:

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	Details Formatted Diff Diff
Patch 2 (9.54 KB, patch) 2012-06-27 22:35 PDT, yosin	no flags	Details Formatted Diff Diff
Patch 3 (9.83 KB, patch) 2012-06-27 22:58 PDT, yosin	no flags	Details Formatted Diff Diff
Patch 4 (15.10 KB, patch) 2012-06-27 23:00 PDT, yosin	no flags	Details Formatted Diff Diff
Patch 5 (10.02 KB, patch) 2012-06-27 23:37 PDT, yosin	no flags	Details Formatted Diff Diff
Patch 6 (15.28 KB, patch) 2012-06-27 23:46 PDT, yosin	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

yosin

Comment 1 2012-06-27 03:30:28 PDT

Created attachment 149716 [details] Patch 1

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

Created attachment 149872 [details] Patch 2

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

Created attachment 149876 [details] Patch 3

yosin

Comment 10 2012-06-27 23:00:37 PDT

Created attachment 149877 [details] Patch 4

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

Created attachment 149883 [details] Patch 5

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

Created attachment 149886 [details] Patch 6

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.