[Webkit-unassigned] [Bug 191649] [Curl] Prevent Data URL Scheme to be called from NetworkDataTask.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=191649

--- Comment #3 from Basuke Suzuki <Basuke.Suzuki at sony.com> ---
Comment on attachment 363078
  --> https://bugs.webkit.org/attachment.cgi?id=363078
Patch

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

This is informal review.

> Source/WebKit/ChangeLog:3
> +        [Curl] Prevent Data URL Scheme to be called from NetworkDataTask.

Please make the title matched with the bug. Sorry I've changed the bug title to match what actually this patch does.

> Source/WebKit/ChangeLog:8
> +        Implement sync access to Data URL for curl port.

"Currently synchronous data url access is not handled in NetworkDataTask and passed to curl layer. Curl doesn't handle data url so that it just fails. Catching the synchronous data request at the beginning of the NetworkDataTask and handle it synchronously."

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:462
> +void NetworkDataTaskCurl::loadDataURL(const URL& url)

This method is just invoke DataURLDecode::decode with a big chunk of response handling lambda. How about moving out the lambda as an method such as respondToDataRequest() and move invocation of decode() to the constructor above? Callback chain makes following the flow of code harder. If it happens, it should be nice to be one place together to understand the overview of the structure.

ex)
    if (requestWithCredentials.url().protocolIsData()) {
        callOnMainThread([this, url = requestWithCredentials.url()] {
            DataURLDecoder::ScheduleContext scheduleContext;
            DataURLDecoder::decode(url, scheduleContext, [this, protectedThis = makeRef(*this), url](auto decodeResult) mutable {
                respondToDataRequest(url, decodeResult);
            });
        });
        return;
    }

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:484
> +        dataResponse.setSource(ResourceResponse::Source::Network);

This is out of topic from this bug, but I'm not sure this is correct. I know this comes from other part of code, but it can be new Source type.
https://github.com/webkit/webkit/blob/master/Source/WebCore/loader/ResourceLoader.cpp#L276

> LayoutTests/platform/wincairo-wk1/TestExpectations:17
> +fast/encoding/char-decoding.html [ Skip ]

You can just skip fast/encoding as it used to be because you didn't do any change on WK Legacy.
fast/encoding [ Skip ]

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190227/1f291ee3/attachment.html>