Bug 109530 - PagePopupController.formatMonth should support short month format
Summary: PagePopupController.formatMonth should support short month format
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks: 109439
  Show dependency treegraph
 
Reported: 2013-02-11 18:01 PST by Keishi Hattori
Modified: 2013-02-19 05:18 PST (History)
3 users (show)

See Also:


Attachments
Patch (14.35 KB, patch)
2013-02-15 00:28 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (17.52 KB, patch)
2013-02-15 03:19 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (14.56 KB, patch)
2013-02-15 04:12 PST, Keishi Hattori
no flags Details | Formatted Diff | Diff
Properly add shortMonthFormat() to the LocaleNone class (1.43 KB, patch)
2013-02-19 04:25 PST, Alberto Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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