FetchResponse should support ConsumeData callback on chunk data is received

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[GSkachkov]

FetchResponse should support ConsumeData callback on chunk data is received. 
We need this to support WebAssembly Streaming API, also will be used for ServiceWorkers

Suggestion from Youenn Fablet:
```
A FetchResponse can be either synthetic or created by a call to fetch().
If created by a call to fetch(), you will often be able to retrieve it from FetchLoader.
But if synthetic, or if it is cloned, the data might come from a blob, a text or a stream object.
The API need to handle all cases properly.

We probably need to move from a one-time called ConsumeDataCallback to a ConsumeDataCallback called for each chunk and a call with nullptr for instance to say this is complete.
I would then tend to store this callback in FetchBodyConsumer for the cases where it cannot be answered synchronously: ReadableStream, blob and loading.
```

[GSkachkov]

Created attachment 331422 [details]
Patch

PoC

[GSkachkov]

[e-mail] Could you please look if I'm implementing in correct way?

Also there is a question what is correct way to test chunks handler functions?
Currently I'm using part of the patch that is related to WebAssembly stream API, and check in debug if chunk handler is invoked.

[youenn fablet]

(In reply to GSkachkov from comment #2)
> [e-mail] Could you please look if I'm implementing in correct way?
> 
> Also there is a question what is correct way to test chunks handler
> functions?

I would start by using the chunk strategy in ServiceWorkerFetch.cpp.
You can tweak LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-readable-stream-worker.js to enqueue two chunks for instance.

[youenn fablet]

Comment on attachment 331422 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchResponse.h:100
> +    void consumeBodyByChunksFromReadableStream(ConsumeDataByChunksCallback&&);

Instead of adding  consumeBodyByChunksFromReadableStream, I would change the way consumeBodyFromReadableStream works.
Currently, we are calling only once ConsumeDataCallback with an aggregated chunk. We could call it several times for each chunk.

That would require to change the users of consumeBodyFromReadableStream:
- ServiceWorkerFetch would send an IPC message for each callback call, the last being a didFinish message, the others sending chunks.
- DOMCache would aggregate the content in a single body inside the callback.

Given that, ConsumeDataCallback could be changed in something like a WTF::Function<void(ExceptionOr<Chunk>>, Chunk being a struct { const uint8_t*, size_t }.

I would also tend to split this patch in two patches, one for handling ReadableStream bodies and one for handling loaded bodies.

[Radar WebKit Bug Importer]

<rdar://problem/36932547>

[youenn fablet]

Created attachment 332593 [details]
Patch

[Alex Christensen]

Comment on attachment 332593 [details]
Patch

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

> Source/WebCore/Modules/cache/DOMCache.cpp:277
> +            response.consumeBodyReceivedByChunk([taskHandler = WTFMove(taskHandler), recordPosition, data = SharedBuffer::create()](auto&& result) mutable {

I think it makes the code significantly easier to read to have the type of result here.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-body-loaded-in-chunk-worker.js:6
> +    event.respondWith(fetch("../../../fetch/api/resources/trickle.py?count=4&delay=50"));

Does the delay do anything significant here?

[youenn fablet]

Comment on attachment 332593 [details]
Patch

Thanks for the review.

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

>> Source/WebCore/Modules/cache/DOMCache.cpp:277
>> +            response.consumeBodyReceivedByChunk([taskHandler = WTFMove(taskHandler), recordPosition, data = SharedBuffer::create()](auto&& result) mutable {
> 
> I think it makes the code significantly easier to read to have the type of result here.

OK, will add it back.

>> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-body-loaded-in-chunk-worker.js:6
>> +    event.respondWith(fetch("../../../fetch/api/resources/trickle.py?count=4&delay=50"));
> 
> Does the delay do anything significant here?

This is just a way to 'ensure' that network process will send response body as chunks to the service worker process.
Then the service worker process will in turn send each chunk before response being finished loading.

[youenn fablet]

Created attachment 332700 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 332700 [details]
Patch for landing

Clearing flags on attachment: 332700

Committed r227870: <https://trac.webkit.org/changeset/227870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Submitted web-platform-tests pull request: https://github.com/w3c/web-platform-tests/pull/9329