We need to implement localizeValue for TimeInputType
Created attachment 166822 [details] Patch
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
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.
Created attachment 166853 [details] Patch
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14126714
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14130700
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14118844
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14142004
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14148002
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
Comment on attachment 166853 [details] Patch Attachment 166853 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14137545
Created attachment 166862 [details] Patch
Comment on attachment 166862 [details] Patch Attachment 166862 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14118854
Created attachment 166864 [details] Just added if around DateTimeStringBuilder
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
Created attachment 166866 [details] Added UNUSED_PARAM for Localizer::formatDateTime
Comment on attachment 166866 [details] Added UNUSED_PARAM for Localizer::formatDateTime ok. Please make sure this won't break builds.
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
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
Created attachment 166880 [details] Patch
Comment on attachment 166880 [details] Patch Attachment 166880 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14130747
Created attachment 167020 [details] Patch
Comment on attachment 167020 [details] Patch Clearing flags on attachment: 167020 Committed r130357: <http://trac.webkit.org/changeset/130357>
All reviewed patches have been landed. Closing bug.