REGRESSION(r262341) URL::createCFURL should produce a CFURL that uses UTF-8 to decode its percent-encoded sequences

[Alex Christensen]

REGRESSION(r262341) URL::createCFURL should produce a CFURL that uses UTF-8 to decode its percent-encoded sequences

[Alex Christensen]

Created attachment 404268 [details]
Patch

[Alex Christensen]

Created attachment 404281 [details]
Patch

[Darin Adler]

Comment on attachment 404281 [details]
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.

[Alex Christensen]

Comment on attachment 404281 [details]
Patch

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

>> Source/WTF/wtf/cf/URLCF.cpp:62
>>          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.

That works great, and doesn't do horrible things like my original patch does.  ASCII checks are considerably faster than parsing URLs, so I don't anticipate the perf hit to be too bad.

[Alex Christensen]

Created attachment 404299 [details]
Patch

[EWS]

Committed r264382: <https://trac.webkit.org/changeset/264382>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404299 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65572000>