Bug 109530 – PagePopupController.formatMonth should support short month format

• Component: Forms
• Status: RESOLVED FIXED

[Keishi Hattori]

We need to support short month format for use in the calendar picker.

[Keishi Hattori]

Created attachment 188497 [details]
Patch

[Kent Tamura]

Comment on attachment 188497 [details]
Patch

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.

[Keishi Hattori]

Created attachment 188524 [details]
Patch

[Kent Tamura]

Comment on attachment 188524 [details]
Patch

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

ok

> Source/WebCore/page/PagePopupController.idl:36
> +    void setValue(in DOMString value);

nit: in is unnecessary

[Kent Tamura]

you posted a patch to a wrong bug.

[Keishi Hattori]

Created attachment 188533 [details]
Patch

[Kent Tamura]

Comment on attachment 188533 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 188533 [details]
Patch

Clearing flags on attachment: 188533

Committed r142988: <http://trac.webkit.org/changeset/142988>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alberto Garcia]

Created attachment 189051 [details]
Properly add shortMonthFormat() to the LocaleNone class

The shortMonthFormat() was not correctly added to the LocaleNone class, here's the fix.

[Keishi Hattori]

(In reply to comment #10)
> Created an attachment (id=189051) [details]
> Properly add shortMonthFormat() to the LocaleNone class
> 
> The shortMonthFormat() was not correctly added to the LocaleNone class, here's the fix.

Oops, sorry. I'm not a reviewer but every bug should have only one patch. If this is a build fix I can land it as unreviewed.

[Alberto Garcia]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=189051) [details] [details]
> > Properly add shortMonthFormat() to the LocaleNone class
> > 
> > The shortMonthFormat() was not correctly added to the LocaleNone class, here's the fix.
> 
> Oops, sorry. I'm not a reviewer but every bug should have only one patch. If this is a build fix I can land it as unreviewed.

It's a build fix, and I can create a new bug if you want.

[Keishi Hattori]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Created an attachment (id=189051) [details] [details] [details]
> > > Properly add shortMonthFormat() to the LocaleNone class
> > > 
> > > The shortMonthFormat() was not correctly added to the LocaleNone class, here's the fix.
> > 
> > Oops, sorry. I'm not a reviewer but every bug should have only one patch. If this is a build fix I can land it as unreviewed.
> 
> It's a build fix, and I can create a new bug if you want.

In that case I'll land this right now. Thanks for making it!

[Keishi Hattori]

Comment on attachment 189051 [details]
Properly add shortMonthFormat() to the LocaleNone class

landed. http://trac.webkit.org/changeset/143320