[webkit-reviews] review denied: [Bug 214314] REGRESSION(r262341) URL::createCFURL should produce a CFURL that uses UTF-8 to decode its percent-encoded sequences : [Attachment 404281] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 214314: REGRESSION(r262341) URL::createCFURL should produce a CFURL that
uses UTF-8 to decode its percent-encoded sequences
https://bugs.webkit.org/show_bug.cgi?id=214314

Attachment 404281: Patch

https://bugs.webkit.org/attachment.cgi?id=404281&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 404281
  --> https://bugs.webkit.org/attachment.cgi?id=404281
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404281&action=review

> Source/WTF/wtf/cf/URLCF.cpp:56
> +struct PartialCFURL {
> +    uintptr_t unused1;
> +    uintptr_t unused2;
> +    uint32_t unused3;
> +    CFStringEncoding encoding;
> +};

I suppose this is OK for the short term, but for the future need CFURL API or
at *least* SPI. Please make *sure* you get the ball rolling on that. This seems
like an accident waiting to happen! I’d prefer that this technique be put into
its own separate named function, not be done inline in the middle of
URL::createCFURL. If we knew what the API/SPI was going to be, maybe we could
name our function based on that.

But I have a suggestion that allows us to avoid this whole mess, with a small
additional performance cost.

> Source/WTF/wtf/cf/URLCF.cpp:62
> +    if (LIKELY(m_string.is8Bit())) {
>	   cfURL = adoptCF(CFURLCreateAbsoluteURLWithBytes(nullptr,
reinterpret_cast<const UInt8*>(m_string.characters8()), m_string.length(),
kCFStringEncodingISOLatin1, nullptr, true));

Instead of the change here, I propose we instead do this:

    if (LIKELY(m_string.is8Bit() && m_string.isAllASCII()))
	cfURL = adoptCF(CFURLCreateAbsoluteURLWithBytes(nullptr,
reinterpret_cast<const UInt8*>(m_string.characters8()), m_string.length(),
kCFStringEncodingUTF8, nullptr, true));
    else
	...

Later if we get some API or SPI, we can optimize further using that, but we
don’t need to do this change just to get correct behavior.