| Summary: | Caching number formatter instances in LocalizedNumber* implementations | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||
| Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Minor | CC: | abarth, ap, darin, dglazkov, eric, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Kent Tamura
2011-03-02 16:48:07 PST
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 |