RESOLVED FIXED 181600
FetchResponse should support ConsumeData callback on chunk data is received
https://bugs.webkit.org/show_bug.cgi?id=181600
Summary FetchResponse should support ConsumeData callback on chunk data is received
GSkachkov
Reported 2018-01-12 10:50:49 PST
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. ```
Attachments
Patch (11.29 KB, patch)
2018-01-16 13:18 PST, GSkachkov
no flags
Patch (15.37 KB, patch)
2018-01-29 15:55 PST, youenn fablet
no flags
Patch for landing (15.35 KB, patch)
2018-01-30 13:58 PST, youenn fablet
no flags
GSkachkov
Comment 1 2018-01-16 13:18:44 PST
Created attachment 331422 [details] Patch PoC
GSkachkov
Comment 2 2018-01-16 13:25:00 PST
youennf@gmail.com 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
Comment 3 2018-01-17 01:39:39 PST
(In reply to GSkachkov from comment #2) > youennf@gmail.com 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 4 2018-01-17 01:51:13 PST
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
Comment 5 2018-01-26 18:18:32 PST
youenn fablet
Comment 6 2018-01-29 15:55:12 PST
Alex Christensen
Comment 7 2018-01-30 13:43:24 PST
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 8 2018-01-30 13:46:46 PST
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
Comment 9 2018-01-30 13:58:12 PST
Created attachment 332700 [details] Patch for landing
WebKit Commit Bot
Comment 10 2018-01-30 20:23:54 PST
Comment on attachment 332700 [details] Patch for landing Clearing flags on attachment: 332700 Committed r227870: <https://trac.webkit.org/changeset/227870>
WebKit Commit Bot
Comment 11 2018-01-30 20:23:55 PST
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 12 2018-01-31 18:28:58 PST
Submitted web-platform-tests pull request: https://github.com/w3c/web-platform-tests/pull/9329
Note You need to log in before you can comment on or make changes to this bug.