Bug 98237 – Implement localizeValue for TimeInputType

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

We need to implement localizeValue for TimeInputType

[Keishi Hattori]

Created attachment 166822 [details]
Patch

[Kent Tamura]

Comment on attachment 166822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166822&action=review

> Source/WebCore/platform/text/Localizer.cpp:44
> +    DateTimeStringBuilder(Localizer*, const DateComponents&);

The first argument should be Localizer& to make sure it's non-null.

> Source/WebCore/platform/text/Localizer.cpp:57
> +    Localizer* m_localizer;

ditto.  should be Localizer&.

> Source/WebCore/platform/text/Localizer.cpp:58
> +    const DateComponents m_date;

should be "const DateComponents&"

> Source/WebCore/platform/text/Localizer.cpp:79
> +    StringBuilder builder;

Existence of builder and m_builder in a function is confusing.  Please rename builder to something else.

> Source/WebCore/platform/text/Localizer.cpp:83
> +    builder.append(String::number(number));
> +    for (size_t i = builder.length(); i < width; ++i)
> +        builder.append("0");

It looks wrong.

String numberString = String::number(number);
for (size_t i = numberString.length(); i < width; ++i)
    builder.append("0");
builder.append(numberString);

> Source/WebCore/platform/text/Localizer.cpp:111
> +        appendNumber(hour24, numberOfPatternCharacters);
> +        appendNumber(hour24, numberOfPatternCharacters);

Why do you add twice?

> Source/WebCore/platform/text/Localizer.cpp:126
> +    case DateTimeFormat::FieldTypeSecond:
> +        appendNumber(m_date.second(), numberOfPatternCharacters);
> +        return;
> +    case DateTimeFormat::FieldTypeFractionalSecond: {
> +        int fractionalSecond = m_date.millisecond();
> +        if (numberOfPatternCharacters > 3)
> +            fractionalSecond *= pow(10, numberOfPatternCharacters - 3);
> +        String fractionalSecondString = String::format("%03d", fractionalSecond).left(numberOfPatternCharacters);
> +        m_builder.append(m_localizer->convertToLocalizedNumber(fractionalSecondString));
> +        return;

We should serialize millisecond part of the DateComponents with non-zero millisecond even if the format has no fractional second field.
So, you can ignore DateTimeFormat::FieldTypeFractionalSecond case, and can do everything for second and millisecond in the DateTimeFormat::FieldTypeSecond block above.

> Source/WebCore/platform/text/Localizer.cpp:329
> +{
> +    DateTimeStringBuilder builder(this, date);

should reject non-time types.

> Source/WebCore/platform/text/Localizer.h:104
> +    enum FormatType { UnspecifiedFormatType, ShortFormatType, MediumFormatType };

Enum names should be FormatTypeUnspecified, FormatTypeShort, FormatTypeMedium.

> Source/WebKit/chromium/tests/LocaleMacTest.cpp:157
> +    EXPECT_STREQ("1:23:00 PM", formatTime("en_US", 13, 23).utf8().data());
> +    EXPECT_STREQ("13:23:00", formatTime("fr_FR", 13, 23).utf8().data());
> +    EXPECT_STREQ("13:23:00", formatTime("ja_JP", 13, 23).utf8().data());
> +    EXPECT_STREQ("12:00:00 AM", formatTime("en_US", 00, 00).utf8().data());
> +    EXPECT_STREQ("00:00:00", formatTime("fr_FR", 00, 00).utf8().data());
> +    EXPECT_STREQ("0:00:00", formatTime("ja_JP", 00, 00).utf8().data());

We should have tests for:
  - the short format.
  - non-short format + non-zero second time.
  - non-zero millisecond time.
  - native digits such as "ar", "fa" locales

[Keishi Hattori]

Comment on attachment 166822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166822&action=review

>> Source/WebCore/platform/text/Localizer.cpp:44
>> +    DateTimeStringBuilder(Localizer*, const DateComponents&);
> 
> The first argument should be Localizer& to make sure it's non-null.

Done.

>> Source/WebCore/platform/text/Localizer.cpp:57
>> +    Localizer* m_localizer;
> 
> ditto.  should be Localizer&.

Done.

>> Source/WebCore/platform/text/Localizer.cpp:58
>> +    const DateComponents m_date;
> 
> should be "const DateComponents&"

Done.

>> Source/WebCore/platform/text/Localizer.cpp:79
>> +    StringBuilder builder;
> 
> Existence of builder and m_builder in a function is confusing.  Please rename builder to something else.

Done. Renamed to zeroPaddedStringBuilder.

>> Source/WebCore/platform/text/Localizer.cpp:83
>> +        builder.append("0");
> 
> It looks wrong.
> 
> String numberString = String::number(number);
> for (size_t i = numberString.length(); i < width; ++i)
>     builder.append("0");
> builder.append(numberString);

Done.

>> Source/WebCore/platform/text/Localizer.cpp:111
>> +        appendNumber(hour24, numberOfPatternCharacters);
> 
> Why do you add twice?

Done.

>> Source/WebCore/platform/text/Localizer.cpp:126
>> +        return;
> 
> We should serialize millisecond part of the DateComponents with non-zero millisecond even if the format has no fractional second field.
> So, you can ignore DateTimeFormat::FieldTypeFractionalSecond case, and can do everything for second and millisecond in the DateTimeFormat::FieldTypeSecond block above.

Done.

>> Source/WebCore/platform/text/Localizer.cpp:329
>> +    DateTimeStringBuilder builder(this, date);
> 
> should reject non-time types.

Done.

>> Source/WebKit/chromium/tests/LocaleMacTest.cpp:157
>> +    EXPECT_STREQ("0:00:00", formatTime("ja_JP", 00, 00).utf8().data());
> 
> We should have tests for:
>   - the short format.
>   - non-short format + non-zero second time.
>   - non-zero millisecond time.
>   - native digits such as "ar", "fa" locales

Done.

[Keishi Hattori]

Created attachment 166853 [details]
Patch

[Early Warning System Bot]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14126714

[Peter Beverloo (cr-android ews)]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14130700

[Build Bot]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14118844

[Gyuyoung Kim]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14142004

[Early Warning System Bot]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14148002

[Kent Tamura]

Comment on attachment 166853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166853&action=review

> Source/WebCore/platform/text/Localizer.cpp:126
> +            float second = m_date.second() + m_date.millisecond() / 1000.0;
> +            String zeroPaddedSecondString = zeroPadString(String::format("%.03f", second), numberOfPatternCharacters + 4);

The variable 'second' should be double because
 - printf accepts only double for floating point numbers
 - m_date.millisecond() / 1000.0 is double

[Build Bot]

Comment on attachment 166853 [details]
Patch

Attachment 166853 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14137545

[Keishi Hattori]

Created attachment 166862 [details]
Patch

[Build Bot]

Comment on attachment 166862 [details]
Patch

Attachment 166862 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14118854

[Keishi Hattori]

Created attachment 166864 [details]
Just added if around DateTimeStringBuilder

[Build Bot]

Comment on attachment 166864 [details]
Just added if around DateTimeStringBuilder

Attachment 166864 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14152083

[Keishi Hattori]

Created attachment 166866 [details]
Added UNUSED_PARAM for Localizer::formatDateTime

[Kent Tamura]

Comment on attachment 166866 [details]
Added UNUSED_PARAM for Localizer::formatDateTime

ok.
Please make sure this won't break builds.

[Peter Beverloo (cr-android ews)]

Comment on attachment 166866 [details]
Added UNUSED_PARAM for Localizer::formatDateTime

Attachment 166866 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14123718

[Build Bot]

Comment on attachment 166866 [details]
Added UNUSED_PARAM for Localizer::formatDateTime

Attachment 166866 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14125762

[Keishi Hattori]

Created attachment 166880 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 166880 [details]
Patch

Attachment 166880 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14130747

[Keishi Hattori]

Created attachment 167020 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167020 [details]
Patch

Clearing flags on attachment: 167020

Committed r130357: <http://trac.webkit.org/changeset/130357>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.