Bug 73794 – Update KURL's copy copyASCII to avoid String::characters()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Poulain]

String::characters() unnecessarily converts the characters to 16bits.

[Benjamin Poulain]

Created attachment 117828 [details]
Patch

[Andreas Kling]

Comment on attachment 117828 [details]
Patch

Neat! r=me

[WebKit Review Bot]

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

[Benjamin Poulain]

The test media/event-attributes.html is flaky, it has been blacklisted in 101850.
I'll land this manually

[Benjamin Poulain]

Committed r102094: <http://trac.webkit.org/changeset/102094>

[Darin Adler]

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.

[Benjamin Poulain]

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

[Andreas Kling]

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.