73794 2011-12-04 20:09:56 -0800 Update KURL's copy copyASCII to avoid String::characters() 2011-12-06 09:52:16 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 73928 1 benjamin benjamin abarth darin dglazkov webkit.review.bot oldest_to_newest 514847 0 benjamin 2011-12-04 20:09:56 -0800 String::characters() unnecessarily converts the characters to 16bits. 514849 1 117828 benjamin 2011-12-04 20:15:30 -0800 Created attachment 117828 Patch 514874 2 117828 kling 2011-12-04 20:58:39 -0800 Comment on attachment 117828 Patch Neat! r=me 514897 3 117828 webkit.review.bot 2011-12-04 21:45:21 -0800 Comment on attachment 117828 Patch Attachment 117828 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10734563 New failing tests: media/event-attributes.html 515639 4 benjamin 2011-12-05 21:48:44 -0800 The test media/event-attributes.html is flaky, it has been blacklisted in 101850. I'll land this manually 515648 5 benjamin 2011-12-05 22:12:41 -0800 Committed r102094: <http://trac.webkit.org/changeset/102094> 515968 6 117828 darin 2011-12-06 09:33:03 -0800 Comment on attachment 117828 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117828&action=review > Source/WebCore/platform/KURL.cpp:261 > + if (string.isNull()) > + return; Why does null need a special case? If it needs one, why doesn’t empty need one? > Source/WebCore/platform/KURL.cpp:268 > + for (size_t i = 0; i < string.length(); i++) > + dest[i] = static_cast<char>(src[i]); Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial. 515973 7 benjamin 2011-12-06 09:37:23 -0800 (In reply to comment #6) > > Source/WebCore/platform/KURL.cpp:261 > > + if (string.isNull()) > > + return; > > Why does null need a special case? If it needs one, why doesn’t empty need one? You cannot call is8Bit() on a null string. I figured isEmpty() would be clearer so I asked on IRC to change isNull() to isEmpty() (that's the patch that landed). > > Source/WebCore/platform/KURL.cpp:268 > > + for (size_t i = 0; i < string.length(); i++) > > + dest[i] = static_cast<char>(src[i]); > > Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial. That is right, I'll fix that. 515974 8 117828 kling 2011-12-06 09:38:00 -0800 Comment on attachment 117828 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117828&action=review >> Source/WebCore/platform/KURL.cpp:261 >> + return; > > Why does null need a special case? If it needs one, why doesn’t empty need one? Benjamin and I talked about this on IRC, and it was landed with isEmpty() rather than isNull(). >> Source/WebCore/platform/KURL.cpp:268 >> + dest[i] = static_cast<char>(src[i]); > > Since this calls String::length() every time through the loop, I believe this may be slower than the old loop you were replacing. The length() function its not entirely trivial. Fudge! I missed that one, my bad. I suspect the compiler might very well move the nullity check of m_impl out of the loop though. But it would indeed be better to put it in a local. 117828 2011-12-04 20:15:30 -0800 2011-12-06 09:38:00 -0800 Patch bug-73794-20111204201529.patch text/plain 4451 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTAxOTY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTBjNWU0MGE0OGQ5MmQz OTFjZjQ4OGE0ZTEwOTZhYTI1NWQ2YmJmOC4uYzdkZmIwZjczODFlMGQyYjQxMDU2ZGZhOGQ0ZWI0 YTY3MDE0ZTFiMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIyIEBACiAyMDExLTEyLTA0ICBCZW5q YW1pbiBQb3VsYWluICA8YmVuamFtaW5Ad2Via2l0Lm9yZz4KIAorICAgICAgICBVcGRhdGUgS1VS TCdzIGNvcHkgY29weUFTQ0lJIHRvIGF2b2lkIFN0cmluZzo6Y2hhcmFjdGVycygpCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03Mzc5NAorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4gdGhlIFN0cmluZyBp cyBhbHJlYWR5IG9uIDggYml0cywgd2UgY2FuIHNpbXBseSBjb3B5IHRoZQorICAgICAgICBkYXRh LiBJbiB0aGUgMTYgYml0cyBjYXNlLCBldmVyeXRoaW5nIHJlbWFpbnMgdGhlIHNhbWUuCisKKyAg ICAgICAgKiBwbGF0Zm9ybS9LVVJMLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmNvcHlBU0NJSSk6 CisgICAgICAgIChXZWJDb3JlOjphcHBlbmRBU0NJSSk6CisgICAgICAgIChXZWJDb3JlOjpLVVJM Ojppbml0KToKKyAgICAgICAgKFdlYkNvcmU6OktVUkw6OnBhcnNlKToKKyAgICAgICAgKFdlYkNv cmU6OktVUkw6OmNvcHlUb0J1ZmZlcik6CisKKzIwMTEtMTItMDQgIEJlbmphbWluIFBvdWxhaW4g IDxiZW5qYW1pbkB3ZWJraXQub3JnPgorCiAgICAgICAgIEdldCByaWQgb2YgS1VSTDo6S1VSTChQ YXJzZWRVUkxTdHJpbmdUYWcsIGNvbnN0IGNoYXIqKTsKICAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczNzkyCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL0tVUkwuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5jcHAK aW5kZXggYzFjOGVlYmY1Yjk2NmRkMzdiODc0YTkyZDEwYmNhYmMyZWFkMGIyMS4uMjQ1YWYxYjEx ZjRkYTk4OGUyZDc4MjNiMGQzNmFlNmJjZDIzNmRjZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vS1VSTC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vS1VSTC5j cHAKQEAgLTI1NSwxNiArMjU1LDI0IEBAIHN0YXRpYyBpbmxpbmUgYm9vbCBpc1NjaGVtZUNoYXJh Y3Rlck1hdGNoSWdub3JpbmdDYXNlKGNoYXIgY2hhcmFjdGVyLCBjaGFyIHNjaGVtCiAvLyBDb3Bp ZXMgdGhlIHNvdXJjZSB0byB0aGUgZGVzdGluYXRpb24sIGFzc3VtaW5nIGFsbCB0aGUgc291cmNl IGNoYXJhY3RlcnMgYXJlCiAvLyBBU0NJSS4gVGhlIGRlc3RpbmF0aW9uIGJ1ZmZlciBtdXN0IGJl IGxhcmdlIGVub3VnaC4gTnVsbCBjaGFyYWN0ZXJzIGFyZSBhbGxvd2VkCiAvLyBpbiB0aGUgc291 cmNlIHN0cmluZywgYW5kIG5vIGF0dGVtcHQgaXMgbWFkZSB0byBudWxsLXRlcm1pbmF0ZSB0aGUg cmVzdWx0Lgotc3RhdGljIHZvaWQgY29weUFTQ0lJKGNvbnN0IFVDaGFyKiBzcmMsIGludCBsZW5n dGgsIGNoYXIqIGRlc3QpCitzdGF0aWMgdm9pZCBjb3B5QVNDSUkoY29uc3QgU3RyaW5nJiBzdHJp bmcsIGNoYXIqIGRlc3QpCiB7Ci0gICAgZm9yIChpbnQgaSA9IDA7IGkgPCBsZW5ndGg7IGkrKykK LSAgICAgICAgZGVzdFtpXSA9IHN0YXRpY19jYXN0PGNoYXI+KHNyY1tpXSk7CisgICAgaWYgKHN0 cmluZy5pc051bGwoKSkKKyAgICAgICAgcmV0dXJuOworCisgICAgaWYgKHN0cmluZy5pczhCaXQo KSkKKyAgICAgICAgbWVtY3B5KGRlc3QsIHN0cmluZy5jaGFyYWN0ZXJzOCgpLCBzdHJpbmcubGVu Z3RoKCkpOworICAgIGVsc2UgeworICAgICAgICBjb25zdCBVQ2hhciogc3JjID0gc3RyaW5nLmNo YXJhY3RlcnMxNigpOworICAgICAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IHN0cmluZy5sZW5n dGgoKTsgaSsrKQorICAgICAgICAgICAgZGVzdFtpXSA9IHN0YXRpY19jYXN0PGNoYXI+KHNyY1tp XSk7CisgICAgfQogfQogCiBzdGF0aWMgdm9pZCBhcHBlbmRBU0NJSShjb25zdCBTdHJpbmcmIGJh c2UsIGNvbnN0IGNoYXIqIHJlbCwgc2l6ZV90IGxlbiwgQ2hhckJ1ZmZlciYgYnVmZmVyKQogewog ICAgIGJ1ZmZlci5yZXNpemUoYmFzZS5sZW5ndGgoKSArIGxlbiArIDEpOwotICAgIGNvcHlBU0NJ SShiYXNlLmNoYXJhY3RlcnMoKSwgYmFzZS5sZW5ndGgoKSwgYnVmZmVyLmRhdGEoKSk7CisgICAg Y29weUFTQ0lJKGJhc2UsIGJ1ZmZlci5kYXRhKCkpOwogICAgIG1lbWNweShidWZmZXIuZGF0YSgp ICsgYmFzZS5sZW5ndGgoKSwgcmVsLCBsZW4pOwogICAgIGJ1ZmZlcltidWZmZXIuc2l6ZSgpIC0g MV0gPSAnXDAnOwogfQpAQCAtMzc3LDcgKzM4NSw3IEBAIHZvaWQgS1VSTDo6aW5pdChjb25zdCBL VVJMJiBiYXNlLCBjb25zdCBTdHJpbmcmIHJlbGF0aXZlLCBjb25zdCBUZXh0RW5jb2RpbmcmIGVu CiAgICAgaWYgKGFsbEFTQ0lJKSB7CiAgICAgICAgIGxlbiA9IHJlbC5sZW5ndGgoKTsKICAgICAg ICAgc3RyQnVmZmVyLnJlc2l6ZShsZW4gKyAxKTsKLSAgICAgICAgY29weUFTQ0lJKHJlbC5jaGFy YWN0ZXJzKCksIGxlbiwgc3RyQnVmZmVyLmRhdGEoKSk7CisgICAgICAgIGNvcHlBU0NJSShyZWws IHN0ckJ1ZmZlci5kYXRhKCkpOwogICAgICAgICBzdHJCdWZmZXJbbGVuXSA9IDA7CiAgICAgICAg IHN0ciA9IHN0ckJ1ZmZlci5kYXRhKCk7CiAgICAgfSBlbHNlIHsKQEAgLTQ3NiwxMCArNDg0LDgg QEAgdm9pZCBLVVJMOjppbml0KGNvbnN0IEtVUkwmIGJhc2UsIGNvbnN0IFN0cmluZyYgcmVsYXRp dmUsIGNvbnN0IFRleHRFbmNvZGluZyYgZW4KICAgICAgICAgICAgICAgICBjaGFyKiBidWZmZXJT dGFydCA9IGJ1ZmZlclBvczsKIAogICAgICAgICAgICAgICAgIC8vIGZpcnN0IGNvcHkgZXZlcnl0 aGluZyBiZWZvcmUgdGhlIHBhdGggZnJvbSB0aGUgYmFzZQotICAgICAgICAgICAgICAgIHVuc2ln bmVkIGJhc2VMZW5ndGggPSBiYXNlLm1fc3RyaW5nLmxlbmd0aCgpOwotICAgICAgICAgICAgICAg IGNvbnN0IFVDaGFyKiBiYXNlQ2hhcmFjdGVycyA9IGJhc2UubV9zdHJpbmcuY2hhcmFjdGVycygp OwotICAgICAgICAgICAgICAgIENoYXJCdWZmZXIgYmFzZVN0cmluZ0J1ZmZlcihiYXNlTGVuZ3Ro KTsKLSAgICAgICAgICAgICAgICBjb3B5QVNDSUkoYmFzZUNoYXJhY3RlcnMsIGJhc2VMZW5ndGgs IGJhc2VTdHJpbmdCdWZmZXIuZGF0YSgpKTsKKyAgICAgICAgICAgICAgICBDaGFyQnVmZmVyIGJh c2VTdHJpbmdCdWZmZXIoYmFzZS5tX3N0cmluZy5sZW5ndGgoKSk7CisgICAgICAgICAgICAgICAg Y29weUFTQ0lJKGJhc2UubV9zdHJpbmcsIGJhc2VTdHJpbmdCdWZmZXIuZGF0YSgpKTsKICAgICAg ICAgICAgICAgICBjb25zdCBjaGFyKiBiYXNlU3RyaW5nID0gYmFzZVN0cmluZ0J1ZmZlci5kYXRh KCk7CiAgICAgICAgICAgICAgICAgY29uc3QgY2hhciogYmFzZVN0cmluZ1N0YXJ0ID0gYmFzZVN0 cmluZzsKICAgICAgICAgICAgICAgICBjb25zdCBjaGFyKiBwYXRoU3RhcnQgPSBiYXNlU3RyaW5n U3RhcnQgKyBiYXNlLm1fcG9ydEVuZDsKQEAgLTEwNDYsNyArMTA1Miw3IEBAIHZvaWQgS1VSTDo6 cGFyc2UoY29uc3QgU3RyaW5nJiBzdHJpbmcpCiAgICAgY2hlY2tFbmNvZGVkU3RyaW5nKHN0cmlu Zyk7CiAKICAgICBDaGFyQnVmZmVyIGJ1ZmZlcihzdHJpbmcubGVuZ3RoKCkgKyAxKTsKLSAgICBj b3B5QVNDSUkoc3RyaW5nLmNoYXJhY3RlcnMoKSwgc3RyaW5nLmxlbmd0aCgpLCBidWZmZXIuZGF0 YSgpKTsKKyAgICBjb3B5QVNDSUkoc3RyaW5nLCBidWZmZXIuZGF0YSgpKTsKICAgICBidWZmZXJb c3RyaW5nLmxlbmd0aCgpXSA9ICdcMCc7CiAgICAgcGFyc2UoYnVmZmVyLmRhdGEoKSwgc3RyaW5n KTsKIH0KQEAgLTE3MTcsNyArMTcyMyw3IEBAIHZvaWQgS1VSTDo6Y29weVRvQnVmZmVyKENoYXJC dWZmZXImIGJ1ZmZlcikgY29uc3QKICAgICAvLyBGSVhNRTogVGhpcyB0aHJvd3MgYXdheSB0aGUg aGlnaCBieXRlcyBvZiBhbGwgdGhlIGNoYXJhY3RlcnMgaW4gdGhlIHN0cmluZyEKICAgICAvLyBU aGF0J3MgZmluZSBmb3IgYSB2YWxpZCBVUkwsIHdoaWNoIGlzIGFsbCBBU0NJSSwgYnV0IG5vdCBm b3IgaW52YWxpZCBVUkxzLgogICAgIGJ1ZmZlci5yZXNpemUobV9zdHJpbmcubGVuZ3RoKCkpOwot ICAgIGNvcHlBU0NJSShtX3N0cmluZy5jaGFyYWN0ZXJzKCksIG1fc3RyaW5nLmxlbmd0aCgpLCBi dWZmZXIuZGF0YSgpKTsKKyAgICBjb3B5QVNDSUkobV9zdHJpbmcsIGJ1ZmZlci5kYXRhKCkpOwog fQogCiBib29sIHByb3RvY29sSXMoY29uc3QgU3RyaW5nJiB1cmwsIGNvbnN0IGNoYXIqIHByb3Rv Y29sKQo=