String::characters() unnecessarily converts the characters to 16bits.
Created attachment 117828 [details] Patch
Comment on attachment 117828 [details] Patch Neat! r=me
Comment on attachment 117828 [details] Patch Attachment 117828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10734563 New failing tests: media/event-attributes.html
The test media/event-attributes.html is flaky, it has been blacklisted in 101850. I'll land this manually
Committed r102094: <http://trac.webkit.org/changeset/102094>
Comment on attachment 117828 [details] 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.
(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.
Comment on attachment 117828 [details] 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.