Bug 55629 – Caching number formatter instances in LocalizedNumber* implementations

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Discussed in Bug 42484.

[Kent Tamura]

Created attachment 84488 [details]
Patch

[Darin Adler]

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.

[Kent Tamura]

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?

[Kent Tamura]

(In reply to comment #3)
> Should we use RetainPtr<NSNumberFormatter> and leafRef() as well?

leafRef() -> leakRef()

[Darin Adler]

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

[Kent Tamura]

Landed with some fixes: http://trac.webkit.org/changeset/80198

[WebKit Review Bot]

http://trac.webkit.org/changeset/80203 might have broken GTK Linux 64-bit Debug