Summary: | Update KURL's copy copyASCII to avoid String::characters() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||
Component: | WebCore Misc. | Assignee: | Benjamin Poulain <benjamin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, darin, dglazkov, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 73928 | ||||||
Attachments: |
|
Description
Benjamin Poulain
2011-12-04 20:09:56 PST
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. |