55629 2011-03-02 16:48:07 -0800 Caching number formatter instances in LocalizedNumber* implementations 2011-03-02 22:17:57 -0800 1 1 1 Unclassified WebKit Forms 528+ (Nightly build) All All RESOLVED FIXED P2 Minor --- 1 tkent tkent abarth ap darin dglazkov eric webkit.review.bot oldest_to_newest 361176 0 tkent 2011-03-02 16:48:07 -0800 Discussed in Bug 42484. 361187 1 84488 tkent 2011-03-02 16:54:56 -0800 Created attachment 84488 Patch 361199 2 84488 darin 2011-03-02 17:04:18 -0800 Comment on attachment 84488 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. 361257 3 84488 tkent 2011-03-02 17:45:17 -0800 Comment on attachment 84488 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? 361260 4 tkent 2011-03-02 17:45:40 -0800 (In reply to comment #3) > Should we use RetainPtr<NSNumberFormatter> and leafRef() as well? leafRef() -> leakRef() 361268 5 darin 2011-03-02 17:57:44 -0800 (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. 361309 6 tkent 2011-03-02 19:16:55 -0800 Landed with some fixes: http://trac.webkit.org/changeset/80198 361358 7 webkit.review.bot 2011-03-02 22:17:57 -0800 http://trac.webkit.org/changeset/80203 might have broken GTK Linux 64-bit Debug 84488 2011-03-02 16:54:56 -0800 2011-03-02 17:45:17 -0800 Patch bug-55629-20110303095454.patch text/plain 5064 tkent U3VidmVyc2lvbiBSZXZpc2lvbjogODAxODIKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBiZWVhZTExYTFiZWQ0MTIw ZTllYTE3NjU1ODI0Zjk0ZmJlYTFhNmJjLi5jYjA3ZjcxYmE1NTNiYjc4OWE3YzY0MDIyMTY2ZDUz ZWM4NWYzYjI2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjUgQEAKKzIwMTEtMDMtMDIgIEtlbnQg VGFtdXJhICA8dGtlbnRAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIENhY2hpbmcgbnVtYmVyIGZvcm1hdHRlciBpbnN0YW5jZXMg aW4gTG9jYWxpemVkTnVtYmVyKiBpbXBsZW1lbnRhdGlvbnMKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU1NjI5CisKKyAgICAgICAgTm8gbmV3IHRlc3Rz LiBUaGlzIGNoYW5nZSBkb2Vzbid0IGNoYW5nZSBleGlzdGluZyBiZWhhdmlvciwgYW5kIGlzCisg ICAgICAgIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS90 ZXh0L0xvY2FsaXplZE51bWJlcklDVS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpjcmVhdGVGb3Jt YXR0ZXJGb3JDdXJyZW50TG9jYWxlKToKKyAgICAgICAgKFdlYkNvcmU6Om51bWJlckZvcm1hdHRl cik6IEludHJvZHVjZSBhIGZ1bmN0aW9uIHRvIHJldHVybiBhIHN0YXRpYworICAgICAgICAgIGlu c3RhbmNlIG9mIE51bWJlckZvcm1hdC4KKyAgICAgICAgKFdlYkNvcmU6OnBhcnNlTG9jYWxpemVk TnVtYmVyKTogVXNlIG51bWJlckZvcm1hdHRlcigpLgorICAgICAgICAoV2ViQ29yZTo6Zm9ybWF0 TG9jYWxpemVkTnVtYmVyKTogVXNlIG51bWJlckZvcm1hdHRlcigpLgorICAgICAgICAqIHBsYXRm b3JtL3RleHQvbWFjL0xvY2FsaXplZE51bWJlck1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6Om51 bWJlckZvcm1hdHRlcik6IEludHJvZHVjZSBhIGZ1bmN0aW9uIHRvIHJldHVybiBhIHN0YXRpYwor ICAgICAgICAgIGluc3RhbmNlIG9mIE5TTnVtYmVyRm9ybWF0dGVyLgorICAgICAgICAoV2ViQ29y ZTo6cGFyc2VMb2NhbGl6ZWROdW1iZXIpOiBVc2UgbnVtYmVyRm9ybWF0dGVyKCkuCisgICAgICAg IChXZWJDb3JlOjpmb3JtYXRMb2NhbGl6ZWROdW1iZXIpOiBVc2UgbnVtYmVyRm9ybWF0dGVyKCku CisKIDIwMTEtMDMtMDIgIEFsZXhleSBQcm9za3VyeWFrb3YgIDxhcEBhcHBsZS5jb20+CiAKICAg ICAgICAgRml4IGFzc2VydGlvbiBmYWlsdXJlcyBvbiBHdGsgYm90LgpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9Mb2NhbGl6ZWROdW1iZXJJQ1UuY3BwIGIvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9Mb2NhbGl6ZWROdW1iZXJJQ1UuY3BwCmluZGV4IDIxYzk4 YzBhOWE0NmEyNzMyZWJiOTcyNDgzMmM4NzQyZTFmNGNhNjMuLmJjZTA3NGEyMWY0MGU0ZjkxMjFm MDkyZjlhNGFlZDNjNGRhOGJmZTUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L3RleHQvTG9jYWxpemVkTnVtYmVySUNVLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS90ZXh0L0xvY2FsaXplZE51bWJlcklDVS5jcHAKQEAgLTQwLDE3ICs0MCwzMCBAQCB1c2luZyBu YW1lc3BhY2Ugc3RkOwogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKLXN0YXRpYyBpbmxpbmUgUGFz c093blB0cjxOdW1iZXJGb3JtYXQ+IGNyZWF0ZUZvcm1hdHRlckZvckN1cnJlbnRMb2NhbGUoKQor c3RhdGljIE51bWJlckZvcm1hdCogY3JlYXRlRm9ybWF0dGVyRm9yQ3VycmVudExvY2FsZSgpCiB7 CiAgICAgVUVycm9yQ29kZSBzdGF0dXMgPSBVX1pFUk9fRVJST1I7Ci0gICAgcmV0dXJuIGFkb3B0 UHRyKE51bWJlckZvcm1hdDo6Y3JlYXRlSW5zdGFuY2Uoc3RhdHVzKSk7CisgICAgTnVtYmVyRm9y bWF0KiBmb3JtYXR0ZXIgPSBOdW1iZXJGb3JtYXQ6OmNyZWF0ZUluc3RhbmNlKHN0YXR1cyk7Cisg ICAgaWYgKHN0YXR1cyAhPSBVX1pFUk9fRVJST1IpIHsKKyAgICAgICAgZGVsZXRlIGZvcm1hdHRl cjsKKyAgICAgICAgcmV0dXJuIDA7CisgICAgfQorICAgIHJldHVybiBmb3JtYXR0ZXI7Cit9CisK Ky8vIFRoaXMgbWlnaHQgcmV0dXJuIDAuCitzdGF0aWMgTnVtYmVyRm9ybWF0KiBudW1iZXJGb3Jt YXR0ZXIoKQoreworICAgIEFTU0VSVChpc01haW5UaHJlYWQoKSk7CisgICAgc3RhdGljIE51bWJl ckZvcm1hdCogZm9ybWF0dGVyID0gY3JlYXRlRm9ybWF0dGVyRm9yQ3VycmVudExvY2FsZSgpOwor ICAgIHJldHVybiBmb3JtYXR0ZXI7CiB9CiAKIGRvdWJsZSBwYXJzZUxvY2FsaXplZE51bWJlcihj b25zdCBTdHJpbmcmIG51bWJlclN0cmluZykKIHsKICAgICBpZiAobnVtYmVyU3RyaW5nLmlzRW1w dHkoKSkKICAgICAgICAgcmV0dXJuIG51bWVyaWNfbGltaXRzPGRvdWJsZT46OnF1aWV0X05hTigp OwotICAgIE93blB0cjxOdW1iZXJGb3JtYXQ+IGZvcm1hdHRlciA9IGNyZWF0ZUZvcm1hdHRlckZv ckN1cnJlbnRMb2NhbGUoKTsKKyAgICBOdW1iZXJGb3JtYXQqIGZvcm1hdHRlciA9IG51bWJlckZv cm1hdHRlcigpOwogICAgIGlmICghZm9ybWF0dGVyKQogICAgICAgICByZXR1cm4gbnVtZXJpY19s aW1pdHM8ZG91YmxlPjo6cXVpZXRfTmFOKCk7CiAgICAgVW5pY29kZVN0cmluZyBudW1iZXJVbmlj b2RlU3RyaW5nKG51bWJlclN0cmluZy5jaGFyYWN0ZXJzKCksIG51bWJlclN0cmluZy5sZW5ndGgo KSk7CkBAIC02Niw3ICs3OSw3IEBAIGRvdWJsZSBwYXJzZUxvY2FsaXplZE51bWJlcihjb25zdCBT dHJpbmcmIG51bWJlclN0cmluZykKIAogU3RyaW5nIGZvcm1hdExvY2FsaXplZE51bWJlcihkb3Vi bGUgbnVtYmVyKQogewotICAgIE93blB0cjxOdW1iZXJGb3JtYXQ+IGZvcm1hdHRlciA9IGNyZWF0 ZUZvcm1hdHRlckZvckN1cnJlbnRMb2NhbGUoKTsKKyAgICBOdW1iZXJGb3JtYXQqIGZvcm1hdHRl ciA9IG51bWJlckZvcm1hdHRlcigpOwogICAgIGlmICghZm9ybWF0dGVyKQogICAgICAgICByZXR1 cm4gU3RyaW5nKCk7CiAgICAgVW5pY29kZVN0cmluZyByZXN1bHQ7CmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L21hYy9Mb2NhbGl6ZWROdW1iZXJNYWMubW0gYi9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L21hYy9Mb2NhbGl6ZWROdW1iZXJNYWMubW0KaW5kZXgg ODZlMDdmM2UxNjA2NmM4ODM4NmY5NmNkODRjMGQwZDBkYWY0Y2YwZi4uMmM1MzM3ZDUyMGYwNjNj MmYxOGZiNTNlZTU5OGI4MjEwOTIwOTRhNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vdGV4dC9tYWMvTG9jYWxpemVkTnVtYmVyTWFjLm1tCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL3RleHQvbWFjL0xvY2FsaXplZE51bWJlck1hYy5tbQpAQCAtNDAsMTQgKzQwLDIz IEBAIHVzaW5nIG5hbWVzcGFjZSBzdGQ7CiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAorc3RhdGlj IE5TTnVtYmVyRm9ybWF0dGVyKiBudW1iZXJGb3JtYXR0ZXIoKQoreworICAgIEFTU0VSVChpc01h aW5UaHJlYWQoKSk7CisgICAgc3RhdGljIE5TTnVtYmVyRm9ybWF0dGVyICpmb3JtYXR0ZXI7Cisg ICAgaWYgKCFmb3JtYXR0ZXIpIHsKKyAgICAgICAgZm9ybWF0dGVyID0gW1tOU051bWJlckZvcm1h dHRlciBhbGxvY10gaW5pdF07CisgICAgICAgIFtmb3JtYXR0ZXIgc2V0TG9jYWxpemVzRm9ybWF0 OllFU107CisgICAgICAgIFtmb3JtYXR0ZXIgc2V0TnVtYmVyU3R5bGU6TlNOdW1iZXJGb3JtYXR0 ZXJEZWNpbWFsU3R5bGVdOworICAgIH0KKyAgICByZXR1cm4gZm9ybWF0dGVyOworfQorCiBkb3Vi bGUgcGFyc2VMb2NhbGl6ZWROdW1iZXIoY29uc3QgU3RyaW5nJiBudW1iZXJTdHJpbmcpCiB7CiAg ICAgaWYgKG51bWJlclN0cmluZy5pc0VtcHR5KCkpCiAgICAgICAgIHJldHVybiBudW1lcmljX2xp bWl0czxkb3VibGU+OjpxdWlldF9OYU4oKTsKLSAgICBSZXRhaW5QdHI8TlNOdW1iZXJGb3JtYXR0 ZXI+IGZvcm1hdHRlcihBZG9wdE5TLCBbW05TTnVtYmVyRm9ybWF0dGVyIGFsbG9jXSBpbml0XSk7 Ci0gICAgW2Zvcm1hdHRlci5nZXQoKSBzZXRMb2NhbGl6ZXNGb3JtYXQ6WUVTXTsKLSAgICBbZm9y bWF0dGVyLmdldCgpIHNldE51bWJlclN0eWxlOk5TTnVtYmVyRm9ybWF0dGVyRGVjaW1hbFN0eWxl XTsKLSAgICBOU051bWJlciAqbnVtYmVyID0gW2Zvcm1hdHRlci5nZXQoKSBudW1iZXJGcm9tU3Ry aW5nOm51bWJlclN0cmluZ107CisgICAgTlNOdW1iZXIgKm51bWJlciA9IFtudW1iZXJGb3JtYXR0 ZXIoKSBudW1iZXJGcm9tU3RyaW5nOm51bWJlclN0cmluZ107CiAgICAgaWYgKCFudW1iZXIpCiAg ICAgICAgIHJldHVybiBudW1lcmljX2xpbWl0czxkb3VibGU+OjpxdWlldF9OYU4oKTsKICAgICBy ZXR1cm4gW251bWJlciBkb3VibGVWYWx1ZV07CkBAIC01NiwxMCArNjUsNyBAQCBkb3VibGUgcGFy c2VMb2NhbGl6ZWROdW1iZXIoY29uc3QgU3RyaW5nJiBudW1iZXJTdHJpbmcpCiBTdHJpbmcgZm9y bWF0TG9jYWxpemVkTnVtYmVyKGRvdWJsZSBpbnB1dE51bWJlcikKIHsKICAgICBSZXRhaW5QdHI8 TlNOdW1iZXI+IG51bWJlcihBZG9wdE5TLCBbW05TTnVtYmVyIGFsbG9jXSBpbml0V2l0aERvdWJs ZTppbnB1dE51bWJlcl0pOwotICAgIFJldGFpblB0cjxOU051bWJlckZvcm1hdHRlcj4gZm9ybWF0 dGVyKEFkb3B0TlMsIFtbTlNOdW1iZXJGb3JtYXR0ZXIgYWxsb2NdIGluaXRdKTsKLSAgICBbZm9y bWF0dGVyLmdldCgpIHNldExvY2FsaXplc0Zvcm1hdDpZRVNdOwotICAgIFtmb3JtYXR0ZXIuZ2V0 KCkgc2V0TnVtYmVyU3R5bGU6TlNOdW1iZXJGb3JtYXR0ZXJEZWNpbWFsU3R5bGVdOwotICAgIHJl dHVybiBTdHJpbmcoW2Zvcm1hdHRlci5nZXQoKSBzdHJpbmdGcm9tTnVtYmVyOm51bWJlci5nZXQo KV0pOworICAgIHJldHVybiBTdHJpbmcoW251bWJlckZvcm1hdHRlcigpIHN0cmluZ0Zyb21OdW1i ZXI6bnVtYmVyLmdldCgpXSk7CiB9CiAKIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUK