Bug 55629 - Caching number formatter instances in LocalizedNumber* implementations
Summary: Caching number formatter instances in LocalizedNumber* implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-02 16:48 PST by Kent Tamura
Modified: 2011-03-02 22:17 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.95 KB, patch)
2011-03-02 16:54 PST, Kent Tamura
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-03-02 16:48:07 PST
Discussed in Bug 42484.
Comment 1 Kent Tamura 2011-03-02 16:54:56 PST
Created attachment 84488 [details]
Patch
Comment 2 Darin Adler 2011-03-02 17:04:18 PST
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 3 Kent Tamura 2011-03-02 17:45:17 PST
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?
Comment 4 Kent Tamura 2011-03-02 17:45:40 PST
(In reply to comment #3)
> Should we use RetainPtr<NSNumberFormatter> and leafRef() as well?

leafRef() -> leakRef()
Comment 5 Darin Adler 2011-03-02 17:57:44 PST
(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.
Comment 6 Kent Tamura 2011-03-02 19:16:55 PST
Landed with some fixes: http://trac.webkit.org/changeset/80198
Comment 7 WebKit Review Bot 2011-03-02 22:17:57 PST
http://trac.webkit.org/changeset/80203 might have broken GTK Linux 64-bit Debug