Bug 98383 - Improve code of LocaleMac.mm
Summary: Improve code of LocaleMac.mm
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: 98116
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-04 01:05 PDT by Kent Tamura
Modified: 2012-10-15 03:00 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.68 KB, patch)
2012-10-04 01:23 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (12.74 KB, patch)
2012-10-15 00:40 PDT, Kent Tamura
tkent: 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-04 01:05:06 PDT
Darin suggested some improvements in https://bugs.webkit.org/show_bug.cgi?id=98116#c5 .

- Use isNull() instead of isEmpty()
- create*Formatter should return RetainPtr
- Explicit String conversion is not needed.
Comment 1 Kent Tamura 2012-10-04 01:23:30 PDT
Created attachment 167047 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-10-08 16:05:23 PDT
Comment on attachment 167047 [details]
Patch

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

> Source/WebCore/platform/text/mac/LocaleMac.mm:115
> +RetainPtr<NSDateFormatter> LocaleMac::createShortDateFormatter()

Is returning RetainPtr the modern pattern?  That will cause an extra ref-churn as the temporary RetainPtr is created/destroyed, but can be OK.
Comment 3 Kent Tamura 2012-10-08 19:09:56 PDT
Comment on attachment 167047 [details]
Patch

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

>> Source/WebCore/platform/text/mac/LocaleMac.mm:115
>> +RetainPtr<NSDateFormatter> LocaleMac::createShortDateFormatter()
> 
> Is returning RetainPtr the modern pattern?  That will cause an extra ref-churn as the temporary RetainPtr is created/destroyed, but can be OK.

Darin wrote we should do so in https://bugs.webkit.org/show_bug.cgi?id=98116#c5

> 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 4 Darin Adler 2012-10-09 09:38:53 PDT
Comment on attachment 167047 [details]
Patch

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

Changes seem fine. Additional improvement is possible, though. And I spotted another leak in this code.

> Source/WebCore/platform/text/mac/LocaleMac.h:72
> +    RetainPtr<NSDateFormatter> createShortDateFormatter();

Now that these functions return RetainPtr, they don’t need create in their names; the word create was there because of the Core Foundation “create/copy” function naming rule. I think the function could now be named just shortDateFormatter because it’s not creating a new one each time, so the create in the name is actively misleading.

> Source/WebCore/platform/text/mac/LocaleMac.h:83
> +    RetainPtr<NSDateFormatter> createTimeFormatter();
> +    RetainPtr<NSDateFormatter> createShortTimeFormatter();

Same comment as above.

> Source/WebCore/platform/text/mac/LocaleMac.mm:73
> +static RetainPtr<NSDateFormatter> createDateTimeFormatter(NSLocale* locale, NSDateFormatterStyle dateStyle, NSDateFormatterStyle timeStyle)

In this case, it’s OK to keep create in the name since the function does create a new one each time.

> Source/WebCore/platform/text/mac/LocaleMac.mm:80
>      [formatter setCalendar:[[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar]];

This line leaks an NSCalendar object every time it’s called.

> Source/WebCore/platform/text/mac/LocaleMac.mm:81
> +    return RetainPtr<NSDateFormatter>(AdoptNS, formatter);

Better to just call the adoptNS function rather than using the constructor form.

    return adoptNS(formatter);

Unless you tried that and had some kind of problem with it. We should remove/deprecate the more complicated and wordy constructor form.

>>> Source/WebCore/platform/text/mac/LocaleMac.mm:115
>>> +RetainPtr<NSDateFormatter> LocaleMac::createShortDateFormatter()
>> 
>> Is returning RetainPtr the modern pattern?  That will cause an extra ref-churn as the temporary RetainPtr is created/destroyed, but can be OK.
> 
> Darin wrote we should do so in https://bugs.webkit.org/show_bug.cgi?id=98116#c5

It’s better to return RetainPtr because otherwise we were often getting object lifetime wrong. If we see problems with churn, the best fix is to create PassRetainPtr.
Comment 5 Kent Tamura 2012-10-15 00:40:44 PDT
Created attachment 168641 [details]
Patch for landing
Comment 6 Kent Tamura 2012-10-15 00:43:02 PDT
Comment on attachment 167047 [details]
Patch

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

>> Source/WebCore/platform/text/mac/LocaleMac.h:72
>> +    RetainPtr<NSDateFormatter> createShortDateFormatter();
> 
> Now that these functions return RetainPtr, they don’t need create in their names; the word create was there because of the Core Foundation “create/copy” function naming rule. I think the function could now be named just shortDateFormatter because it’s not creating a new one each time, so the create in the name is actively misleading.

ok, I renamed it to shortDateFormatter().

>> Source/WebCore/platform/text/mac/LocaleMac.h:83
>> +    RetainPtr<NSDateFormatter> createShortTimeFormatter();
> 
> Same comment as above.

Ditto.

>> Source/WebCore/platform/text/mac/LocaleMac.mm:80
>>      [formatter setCalendar:[[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar]];
> 
> This line leaks an NSCalendar object every time it’s called.

I added RetainPtr<NSCalendar> data member to LocaleMac.

>> Source/WebCore/platform/text/mac/LocaleMac.mm:81
>> +    return RetainPtr<NSDateFormatter>(AdoptNS, formatter);
> 
> Better to just call the adoptNS function rather than using the constructor form.
> 
>     return adoptNS(formatter);
> 
> Unless you tried that and had some kind of problem with it. We should remove/deprecate the more complicated and wordy constructor form.

Done.
Comment 7 Kent Tamura 2012-10-15 03:00:45 PDT
Committed r131293: <http://trac.webkit.org/changeset/131293>