Bug 98116 - [Mac][Chromium-Mac] Implement LocaleMac::dateFormat
Summary: [Mac][Chromium-Mac] Implement LocaleMac::dateFormat
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 98109
Blocks: 97997 98383
  Show dependency treegraph
 
Reported: 2012-10-01 22:52 PDT by Kent Tamura
Modified: 2012-10-04 01:25 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.36 KB, patch)
2012-10-02 00:18 PDT, Kent Tamura
morrita: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-10-01 22:52:42 PDT
[Mac][Chromium-Mac] Implement LocaleMac::dateFormat
Comment 1 Kent Tamura 2012-10-02 00:18:43 PDT
Created attachment 166627 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-02 18:20:32 PDT
Comment on attachment 166627 [details]
Patch

Rejecting attachment 166627 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14129438
Comment 3 Kent Tamura 2012-10-02 18:21:58 PDT
Oops, the ChangeLog has no "Reviewed by" line.
Comment 4 Kent Tamura 2012-10-02 18:25:16 PDT
Committed r130243: <http://trac.webkit.org/changeset/130243>
Comment 5 Darin Adler 2012-10-03 07:48:29 PDT
Comment on attachment 166627 [details]
Patch

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

> Source/WebCore/platform/text/mac/LocaleMac.mm:257
> +    if (!m_dateFormat.isEmpty())

I suggest using isNull, more efficient, here.

> Source/WebCore/platform/text/mac/LocaleMac.mm:260
> +    RetainPtr<NSDateFormatter> formatter(AdoptNS, createShortDateFormatter());
> +    m_dateFormat = String([formatter.get() dateFormat]);

The explicit String() here is unneeded, I believe.

I would write this:

    if (m_dateFormat.isNull())
        m_dateFormat = [adoptNS(createShortDateFormatter()).get() dateFormat];
    return m_dateFormat;

And later createShortDateFormatter should be changed to return a RetainPtr instead of a raw pointer, which would eliminate the need for the adoptNS call here.
Comment 6 Kent Tamura 2012-10-04 01:25:24 PDT
(In reply to comment #5)
> > Source/WebCore/platform/text/mac/LocaleMac.mm:257
> > +    if (!m_dateFormat.isEmpty())
> 
> I suggest using isNull, more efficient, here.
> 
> > Source/WebCore/platform/text/mac/LocaleMac.mm:260
> > +    RetainPtr<NSDateFormatter> formatter(AdoptNS, createShortDateFormatter());
> > +    m_dateFormat = String([formatter.get() dateFormat]);
> 
> The explicit String() here is unneeded, I believe.
> 
> I would write this:
> 
>     if (m_dateFormat.isNull())
>         m_dateFormat = [adoptNS(createShortDateFormatter()).get() dateFormat];
>     return m_dateFormat;
> 
> And later createShortDateFormatter should be changed to return a RetainPtr instead of a raw pointer, which would eliminate the need for the adoptNS call here.

Thank you for the suggestions!
I filed Bug 98383, and uploaded a patch for them.