Bug 89965 – [Platform] Implement functions for localized time format information

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

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.

Created attachment 149716 [details]
Patch 1

Comment on attachment 149716 [details]
Patch 1

Could you review this patch?
Thanks in advance.

[Kent Tamura]

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
> [phone]  Yoshifumi Inoue  <[e-mail]>
> +
> +        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.

[Kent Tamura]

(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 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 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)

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

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.