[webkit-reviews] review denied: [Bug 71602] [Mac] Improve the conversion from KURL to CFURL : [Attachment 119826] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 71602: [Mac] Improve the conversion from KURL to CFURL
https://bugs.webkit.org/show_bug.cgi?id=71602

Attachment 119826: Patch
https://bugs.webkit.org/attachment.cgi?id=119826&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=119826&action=review


This looks great. review- because of the CSTring.h typo, but also we need to
figure out if CFURL does anything with the passed-in character encoding later
on after the URL is created.

> Source/WebCore/platform/cf/KURLCFNet.cpp:30
> +#include <wtf/text/CSTring.h>

We don’t need this include any more. But also, it will break the build on case
sensitive file systems because of the capital T.

> Source/WebCore/platform/cf/KURLCFNet.cpp:69
> +    if (urlString.is8Bit())
> +	   return CFURLCreateAbsoluteURLWithBytes(0, urlString.characters8(),
urlString.length(), kCFStringEncodingISOLatin1, 0, true);
> +    return CFURLCreateAbsoluteURLWithBytes(0, reinterpret_cast<const
UInt8*>(urlString.characters16()), urlString.length(),
kCFStringEncodingUnicode, 0, true);

I believe the encoding passed in to this function is used by CFURL for more
than just interpreting the bytes at creation time; I think it has some effect
on the handling of escape sequences later. I’ll have to research this before
I’m sure this change is safe.

What about URL strings that are 16-bit strings, but happen to be all ASCII?
Aren’t those common? Or maybe we always create a string with 8-bit characters
now.

> Source/WebCore/platform/cf/KURLCFNet.cpp:76
> +    // Note: This function does not handle empty URLs because CFURL does not
handle empty URLs.
> +    // See KURL::createCFURL() in KURLMac.mm.

I think it’s confusing to say “does not handle empty URLs”; it raises but does
not answer this question: What does this function do when the URL is empty?
Does it crash? Create some other kind of URL?