[webkit-reviews] review denied: [Bug 109530] PagePopupController.formatMonth should support short month format : [Attachment 188497] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 109530: PagePopupController.formatMonth should support short month format
https://bugs.webkit.org/show_bug.cgi?id=109530

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

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188497&action=review


> Source/WebCore/page/PagePopupController.idl:37
> -    [Conditional=CALENDAR_PICKER] DOMString formatMonth(in long year, in
long zeroBaseMonth);
> +    [Conditional=CALENDAR_PICKER] DOMString formatMonth(in long year, in
long zeroBaseMonth, in boolean useShortMonth);

We had better introduce a separated function like formatShortMonth(year,
month).

> Source/WebCore/platform/text/PlatformLocale.cpp:357
> +	   builder.build(formatType == FormatTypeShort ? shortMonthFormat() :
monthFormat());

nit: We had better explain this behavior on the formatDateTime declaration in
PlatformLocale.h.

> Source/WebCore/platform/text/PlatformLocale.h:71
>      // Returns a year-month format in Unicode TR35 LDML.
>      virtual String monthFormat() = 0;
>  
> +    // Returns a year-month format in Unicode TR35 LDML.
> +    virtual String shortMonthFormat() = 0;

We should mention difference between them.

> Source/WebCore/platform/text/mac/LocaleMac.mm:213
> +    // Gets a format for "MMMM" because Windows API always provides formats
for
> +    // "MMMM" in some locales.
> +    m_shortMonthFormat = [NSDateFormatter dateFormatFromTemplate:@"yyyyMMM"
options:0 locale:m_locale.get()];

incorrect comment.