[webkit-reviews] review denied: [Bug 101878] Add support for week/month to Locale::formatDateTime() : [Attachment 173592] 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 101878: Add support for week/month to Locale::formatDateTime()
https://bugs.webkit.org/show_bug.cgi?id=101878

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173592&action=review


> Source/WebCore/ChangeLog:10
> +	   No new tests. Added Chromium tests LocaleMacTest.formatWeek and
LocaleMacTest.formatMonth.

nit: "No new tests." is unnecessary.

> Source/WebCore/platform/text/PlatformLocale.cpp:102
>      case DateTimeFormat::FieldTypeMonth:

We need to support FieldTypeMonthStandAlone too.  Some locales such as Russian
use it.

> Source/WebCore/platform/text/PlatformLocale.cpp:360
> -    if (date.type() != DateComponents::Time && date.type() !=
DateComponents::Date)
> +    if (date.type() != DateComponents::Time && date.type() !=
DateComponents::Date && date.type() != DateComponents::Week && date.type() !=
DateComponents::Month)
>	   return String();

nit:
if (date.type() == DateComponents::DateTime || date.type() ==
DateComponents::DateTimeLocal || date.type() == DateComponents::Invalid)

is shorter.

> Source/WebCore/platform/text/PlatformLocale.cpp:371
>	   builder.build(formatType == FormatTypeShort ? shortTimeFormat() :
timeFormat());
>      else if (date.type() == DateComponents::Date)
>	   builder.build(dateFormat());
> +    else if (date.type() == DateComponents::Week)
> +	   builder.build(weekFormatInLDML());
> +    else if (date.type() == DateComponents::Month)
> +	   builder.build(monthFormat());

nit: It's time to apply switch-case.