Bug 220981 – Load data URLs in the web process also for synchronous loads

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

In r271879 I removed the support for data URLs in the network process for soup, assuming data URLs were always loaded from the web process, but that's not the case for synchronous loads. We don't need to go to the network process for sync loads.

[Carlos Garcia Campos]

Created attachment 418418 [details]
Patch

[Carlos Garcia Campos]

It seems it's failing to find <WebCore/DataURLDecoder.h>. Aren't forwarding headers automatic in mac? I added the header to Source/WebCore/Headers.cmake

[Carlos Garcia Campos]

*** Bug 221023 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 418525 [details]
Patch

[Carlos Garcia Campos]

Created attachment 418526 [details]
Patch

[Carlos Garcia Campos]

Created attachment 418528 [details]
Patch

[Alex Christensen]

Comment on attachment 418528 [details]
Patch

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

> Source/WebCore/platform/network/DataURLDecoder.cpp:221
> +    bool success = task->process();

I don't think we need a local variable here.  I think the function logic would look simpler like this:

if (!task->process())
    return WTF::nullopt;
if (task->isBase64) {
    if (!decodeBase64(*task, mode))
        return WTF::nullopt;
} else
    decodeEscaped(*task);
return WTFMove(task->result);

Also, this whole thing is duplicate code.  I think this function should be called decodeSynchronously and decode should call it.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:547
> +    result.response.setHTTPStatusCode(200);

This is adding duplicate code with ResourceLoader::loadDataURL.  Could we make them share code?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:601
> +        auto syncLoadResult = loadDataURLSynchronously(request);

WebLoaderStrategy::loadResourceSynchronously should return a SyncLoadResult instead of taking error, response, and data as out params.

[Carlos Garcia Campos]

Comment on attachment 418528 [details]
Patch

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

>> Source/WebCore/platform/network/DataURLDecoder.cpp:221
>> +    bool success = task->process();
> 
> I don't think we need a local variable here.  I think the function logic would look simpler like this:
> 
> if (!task->process())
>     return WTF::nullopt;
> if (task->isBase64) {
>     if (!decodeBase64(*task, mode))
>         return WTF::nullopt;
> } else
>     decodeEscaped(*task);
> return WTFMove(task->result);
> 
> Also, this whole thing is duplicate code.  I think this function should be called decodeSynchronously and decode should call it.

Ok, I'll moved the common parts, the whole function can't be used from the async function because the task creation is different.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:547
>> +    result.response.setHTTPStatusCode(200);
> 
> This is adding duplicate code with ResourceLoader::loadDataURL.  Could we make them share code?

Yes.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:601
>> +        auto syncLoadResult = loadDataURLSynchronously(request);
> 
> WebLoaderStrategy::loadResourceSynchronously should return a SyncLoadResult instead of taking error, response, and data as out params.

SyncLoadResult is private. We could make it public and update all the callers, but I think that refactoring is out of scope of this bug.

[Carlos Garcia Campos]

Created attachment 418633 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r272009: <https://trac.webkit.org/changeset/272009>

[Radar WebKit Bug Importer]

<rdar://problem/73704517>