Discussed in Bug 42484.
Created attachment 84488 [details] Patch
Comment on attachment 84488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84488&action=review > Source/WebCore/platform/text/LocalizedNumberICU.cpp:43 > +static NumberFormat* createFormatterForCurrentLocale() This should return a PassOwnPtr. > Source/WebCore/platform/text/LocalizedNumberICU.cpp:51 > + NumberFormat* formatter = NumberFormat::createInstance(status); > + if (status != U_ZERO_ERROR) { > + delete formatter; > + return 0; > + } > + return formatter; If this used OwnPtr then you would not need the explicit delete in the error case. > Source/WebCore/platform/text/LocalizedNumberICU.cpp:58 > + static NumberFormat* formatter = createFormatterForCurrentLocale(); This should explicitly call leakPtr. > Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:51 > + if (!formatter) { > + formatter = [[NSNumberFormatter alloc] init]; > + [formatter setLocalizesFormat:YES]; > + [formatter setNumberStyle:NSNumberFormatterDecimalStyle]; > + } This code should go into a separate create function, just like the ICU version.
Comment on attachment 84488 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84488&action=review Thank you for reviewing! >> Source/WebCore/platform/text/mac/LocalizedNumberMac.mm:51 >> + } > > This code should go into a separate create function, just like the ICU version. Should we use RetainPtr<NSNumberFormatter> and leafRef() as well?
(In reply to comment #3) > Should we use RetainPtr<NSNumberFormatter> and leafRef() as well? leafRef() -> leakRef()
(In reply to comment #3) > Should we use RetainPtr<NSNumberFormatter> and leakRef() as well? Yes. That one is not as important as the OwnPtr case, but I won’t go into a long lecture about exactly why I think so.
Landed with some fixes: http://trac.webkit.org/changeset/80198
http://trac.webkit.org/changeset/80203 might have broken GTK Linux 64-bit Debug