[Mac][Chromium-Mac] Implement LocaleMac::dateFormat

• Component: Platform
• Status: RESOLVED FIXED

[Kent Tamura]

[Mac][Chromium-Mac] Implement LocaleMac::dateFormat

[Kent Tamura]

Created attachment 166627 [details]
Patch

[WebKit Review Bot]

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

[Kent Tamura]

Oops, the ChangeLog has no "Reviewed by" line.

[Kent Tamura]

Committed r130243: <http://trac.webkit.org/changeset/130243>

[Darin Adler]

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.

[Kent Tamura]

(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.