[webkit-reviews] review denied: [Bug 98237] Implement localizeValue for TimeInputType : [Attachment 166822] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 98237: Implement localizeValue for TimeInputType
https://bugs.webkit.org/show_bug.cgi?id=98237

Attachment 166822: Patch
https://bugs.webkit.org/attachment.cgi?id=166822&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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