Bug 98229 - Introduce Localizer::dateTimeFormatWithSecond and dateTimeFormatWithoutSecond
Summary: Introduce Localizer::dateTimeFormatWithSecond and dateTimeFormatWithoutSecond
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 98485
Blocks:
  Show dependency tree / graph
 
Reported: 2012-10-02 21:21 PDT by Kent Tamura
Modified: 2012-10-04 23:25 PDT (History)
5 users (show)

See Also:

Attachments
Patch (6.63 KB, patch)
2012-10-02 21:28 PDT, Kent Tamura 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-10-02 21:21:02 PDT 
Introduce Localizer::dateTimeFormatWithSecond and dateTimeFormatWithoutSecond
Comment 1 Kent Tamura 2012-10-02 21:28:07 PDT 
Created attachment 166805 [details]
Patch
Comment 2 Kentaro Hara 2012-10-02 21:31:54 PDT 
Comment on attachment 166805 [details]
Patch

OK
Comment 3 WebKit Review Bot 2012-10-02 21:54:31 PDT 
Comment on attachment 166805 [details]
Patch

Clearing flags on attachment: 166805

Committed r130247: <http://trac.webkit.org/changeset/130247>
Comment 4 WebKit Review Bot 2012-10-02 21:54:34 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 5 Darin Adler 2012-10-03 09:10:27 PDT 
Comment on attachment 166805 [details]
Patch

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

> Source/WebCore/platform/text/Localizer.cpp:210
> +String Localizer::dateTimeFormatWithSecond()

Should be “with seconds” rather than “with second”

> Source/WebCore/platform/text/Localizer.cpp:212
> +    // FIXME: We should retreive the separator and the order from the system.

Spelling is retrieve, not retreive.

> Source/WebCore/platform/text/Localizer.cpp:217
> +    return builder.toString();

Seems inefficient to create a new string every time. Other functions in this class have caching to avoid recomputing strings. Is the performance of this going to be OK?

> Source/WebCore/platform/text/Localizer.cpp:220
> +String Localizer::dateTimeFormatWithoutSecond()

Should be “without seconds” rather than “with second”

> Source/WebCore/platform/text/Localizer.cpp:222
> +    // FIXME: We should retreive the separator and the order from the system.

Spelling is retrieve, not retreive.

> Source/WebCore/platform/text/Localizer.cpp:227
> +    return builder.toString();

Same “create a new string every time” question.
Comment 6 Kent Tamura 2012-10-04 23:25:05 PDT 
(In reply to comment #5)
> (From update of attachment 166805 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166805&action=review
> 
> > Source/WebCore/platform/text/Localizer.cpp:210
> > +String Localizer::dateTimeFormatWithSecond()
> 
> Should be “with seconds” rather than “with second”
> 
> > Source/WebCore/platform/text/Localizer.cpp:212
> > +    // FIXME: We should retreive the separator and the order from the system.
> 
> Spelling is retrieve, not retreive.
> 
> > Source/WebCore/platform/text/Localizer.cpp:217
> > +    return builder.toString();
> 
> Seems inefficient to create a new string every time. Other functions in this class have caching to avoid recomputing strings. Is the performance of this going to be OK?
> 
> > Source/WebCore/platform/text/Localizer.cpp:220
> > +String Localizer::dateTimeFormatWithoutSecond()
> 
> Should be “without seconds” rather than “with second”
> 
> > Source/WebCore/platform/text/Localizer.cpp:222
> > +    // FIXME: We should retreive the separator and the order from the system.
> 
> Spelling is retrieve, not retreive.
> 
> > Source/WebCore/platform/text/Localizer.cpp:227
> > +    return builder.toString();
> 
> Same “create a new string every time” question.

Thank you for the comments.  I filed Bug 98485 and uploaded a patch.