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

Description yosin 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.
Comment 1 yosin 2012-06-27 03:30:28 PDT
Created attachment 149716 [details]
Patch 1
Comment 2 yosin 2012-06-27 03:31:44 PDT
Comment on attachment 149716 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 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.
Comment 4 yosin 2012-06-27 22:35:32 PDT
Created attachment 149872 [details]
Patch 2
Comment 5 yosin 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.
Comment 6 Kent Tamura 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.
Comment 7 WebKit Review Bot 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
Comment 8 Kent Tamura 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.
Comment 9 yosin 2012-06-27 22:58:54 PDT
Created attachment 149876 [details]
Patch 3
Comment 10 yosin 2012-06-27 23:00:37 PDT
Created attachment 149877 [details]
Patch 4
Comment 11 yosin 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)
Comment 12 Kent Tamura 2012-06-27 23:21:44 PDT
Comment on attachment 149877 [details]
Patch 4

r- because of Comment #8
Comment 13 yosin 2012-06-27 23:37:04 PDT
Created attachment 149883 [details]
Patch 5
Comment 14 yosin 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
Comment 15 Kent Tamura 2012-06-27 23:45:10 PDT
Comment on attachment 149883 [details]
Patch 5

missing LocalizedDateICUTest.cpp
Comment 16 yosin 2012-06-27 23:46:44 PDT
Created attachment 149886 [details]
Patch 6
Comment 17 yosin 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
Comment 18 Kent Tamura 2012-06-28 00:27:52 PDT
Comment on attachment 149886 [details]
Patch 6

ok
Comment 19 yosin 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>
Comment 20 yosin 2012-06-28 00:30:26 PDT
All reviewed patches have been landed.  Closing bug.