Bug 98109 - Adding Localizer::dateFormat() for multiple fields date/datetime input UI
Summary: Adding Localizer::dateFormat() for multiple fields date/datetime input UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 97997 98116 98117 98118 98123
  Show dependency treegraph
 
Reported: 2012-10-01 21:23 PDT by yosin
Modified: 2012-10-02 00:55 PDT (History)
1 user (show)

See Also:


Attachments
Patch 1 (2.88 KB, patch)
2012-10-01 21:31 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (2.95 KB, patch)
2012-10-01 21:32 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (6.98 KB, patch)
2012-10-01 22:20 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (7.23 KB, patch)
2012-10-01 23:46 PDT, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-10-01 21:23:24 PDT
To implement multiple fields date/datetime input UI, we need to have Localizer::dateFormat().
Comment 1 yosin 2012-10-01 21:31:16 PDT
Created attachment 166599 [details]
Patch 1
Comment 2 yosin 2012-10-01 21:32:59 PDT
Created attachment 166601 [details]
Patch 2
Comment 3 yosin 2012-10-01 21:33:38 PDT
Comment on attachment 166601 [details]
Patch 2

Could you review this patch?
Thanks in advance.
Comment 4 Kent Tamura 2012-10-01 21:51:11 PDT
Comment on attachment 166601 [details]
Patch 2

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

> Source/WebCore/platform/text/Localizer.cpp:193
> +    if (!m_localizedDateFormatText.isEmpty())
> +        return m_localizedDateFormatText;
> +    m_localizedDateFormatText = "dd/mm/yyyy";
> +    return m_localizedDateFormatText;

I think we had better return an empty string or make dateFormat pure virtual because this code won't be used.

> Source/WebCore/platform/text/Localizer.h:57
> +    // Returns time format in Unicode TR35 LDML[1] containing day of month,

time -> date

> Source/WebCore/platform/text/Localizer.h:120
> +    String m_localizedDateFormatText;

should be m_dateFormat if you need this member.
Comment 5 yosin 2012-10-01 22:20:00 PDT
Created attachment 166605 [details]
Patch 3
Comment 6 yosin 2012-10-01 23:32:46 PDT
Comment on attachment 166605 [details]
Patch 3

Could you review this patch?
Thanks in advance.

Note: This patch was compiled on Mac EWS, although failed on unrelated tests:

Unexpected flakiness: text-only failures (6)
  compositing/iframes/overlapped-nested-iframes.html [ Failure Pass ]
  fast/loader/javascript-url-in-object.html [ Failure Pass ]
  http/tests/cookies/single-quoted-value.html [ Failure Pass ]
  http/tests/security/cookies/third-party-cookie-blocking-user-action.html [ Failure Pass ]
  http/tests/security/sandboxed-iframe-origin-add.html [ Failure Pass ]
  media/media-controller-playback.html [ Failure Pass ]

Unexpected flakiness: timeouts (1)
  http/tests/webarchive/test-preload-resources.html [ Timeout Pass ]


Regressions: Unexpected text-only failures : (1)
  platform/mac/editing/spelling/removing-underline-after-accepting-autocorrection-using-punctuation.html [ Failure ]
Comment 7 Kent Tamura 2012-10-01 23:36:06 PDT
Comment on attachment 166605 [details]
Patch 3

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

> Source/WebCore/platform/text/Localizer.h:60
> +    // Returns date format in Unicode TR35 LDML[1] containing day of month,
> +    // month, and year, e.g. "dd/mm/yyyy"
> +    // [1] LDML http://unicode.org/reports/tr35/tr35-6.html#Date_Format_Patterns
> +    virtual String dateFormat() = 0;

Would you move this below localizedDecimalSeparator() please?
We'd like to categorize number-related functions and date/time related functions.

Also, http://unicode.org/reports/tr35/tr35-6.html is not the latest version of TR35.  Is it ok?
Comment 8 yosin 2012-10-01 23:46:50 PDT
Created attachment 166619 [details]
Patch 4
Comment 9 yosin 2012-10-01 23:48:42 PDT
Comment on attachment 166619 [details]
Patch 4

Clearing flags on attachment: 166619

Committed r130127: <http://trac.webkit.org/changeset/130127>
Comment 10 yosin 2012-10-01 23:48:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Kent Tamura 2012-10-02 00:31:24 PDT
Comment on attachment 166619 [details]
Patch 4

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

> Source/WebCore/platform/text/LocaleNone.cpp:81
> +    return ASCIILiteral("dd/mm/yyyyy");

should be dd/MM/yyyy, right?