Bug 109530

Summary: PagePopupController.formatMonth should support short month format
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: FormsAssignee: Keishi Hattori <keishi>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 109439    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Properly add shortMonthFormat() to the LocaleNone class none

Description Keishi Hattori 2013-02-11 18:01:44 PST
We need to support short month format for use in the calendar picker.
Comment 1 Keishi Hattori 2013-02-15 00:28:44 PST
Created attachment 188497 [details]
Patch
Comment 2 Kent Tamura 2013-02-15 00:44:13 PST
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.
Comment 3 Keishi Hattori 2013-02-15 03:19:35 PST
Created attachment 188524 [details]
Patch
Comment 4 Kent Tamura 2013-02-15 03:26:34 PST
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
Comment 5 Kent Tamura 2013-02-15 03:28:00 PST
you posted a patch to a wrong bug.
Comment 6 Keishi Hattori 2013-02-15 04:12:45 PST
Created attachment 188533 [details]
Patch
Comment 7 Kent Tamura 2013-02-15 04:58:02 PST
Comment on attachment 188533 [details]
Patch

ok
Comment 8 WebKit Review Bot 2013-02-15 06:06:47 PST
Comment on attachment 188533 [details]
Patch

Clearing flags on attachment: 188533

Committed r142988: <http://trac.webkit.org/changeset/142988>
Comment 9 WebKit Review Bot 2013-02-15 06:06:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Alberto Garcia 2013-02-19 04:25:33 PST
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.
Comment 11 Keishi Hattori 2013-02-19 05:01:30 PST
(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.
Comment 12 Alberto Garcia 2013-02-19 05:07:49 PST
(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.
Comment 13 Keishi Hattori 2013-02-19 05:12:23 PST
(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!
Comment 14 Keishi Hattori 2013-02-19 05:18:36 PST
Comment on attachment 189051 [details]
Properly add shortMonthFormat() to the LocaleNone class

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