Created attachment 167047 [details] Patch

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

Created attachment 168641 [details] Patch for landing

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.

Committed r131293: <http://trac.webkit.org/changeset/131293>