Bug 182008

Created attachment 332067 [details] Patch WiP for check progress and check tests

This looks good to me. Since there are two places that want to read the whole data as a shared buffer, I would try wrapping that code in a helper function. We might also reuse the same callback for loading data. In such a case Chunk should probably be defined outside ReadableStream sink class.

(In reply to youenn fablet from comment #2) > This looks good to me. Since there are two places that want to read the > whole data as a shared buffer, I would try wrapping that code in a helper > function. > > We might also reuse the same callback for loading data. > In such a case Chunk should probably be defined outside ReadableStream sink > class. Thanks for feedback, I'll back with fixed comments and test.

Created attachment 332396 [details] Patch Patch for review

Created attachment 332397 [details] Patch Rebased patch for review

Comment on attachment 332397 [details] Patch If possible, we should try to remove m_data from the sink implementation. We should also verify the case of empty buffers. Some additional nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=332397&action=review > Source/WebCore/Modules/cache/DOMCache.cpp:338 > + response->consumeBodyFromReadableStream([promise = WTFMove(promise), request = WTFMove(request), response = WTFMove(response), data = WTFMove(data), this](ExceptionOr<ReadableStreamChunk>&& result) mutable { You can probably use auto&& result here. > Source/WebCore/Modules/cache/DOMCache.cpp:340 > + ReadableStreamChunk chunk = result.returnValue(); auto as well I think. > Source/WebCore/Modules/cache/DOMCache.cpp:346 > + unsetPendingActivity(this); This is preexisting code but instead of setPendingActivity/unsetPendingActivity, we can have the lambda take a makePendingActivity(*this). See existing WebKit code for that pattern. > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:40 > +: m_callback { WTFMove(callback) } Change is not needed. > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:55 > m_data = SharedBuffer::create(buffer.data(), buffer.length()); We are probably no longer using m_data. We can probably remove it. This will simplify and optimize this method a bit. > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:60 > + // TODO: Check if we need incomplete TODO to remove? Or change to FIXME? > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:64 > + m_callback(ReadableStreamChunk { buffer.data(), buffer.length() }); Can BufferSource length be zero? If so, we might not be able to disambiguate this case with closing the ReadableStream. Passing a ReadableStreamChunk* or an optional might be clearer. > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-readable-stream-chunk-worker.js:38 > + .step(() => controller.enqueue(encoder.encode('chunk #3 '))) Might be easy to add encoder.encode('') in the middle.

Thanks for review! I hope to back with fixes during week.

<rdar://problem/36932534>

Created attachment 332491 [details] Patch Patch with fixed comments

Comment on attachment 332491 [details] Patch LGTM. Great to see a WPT test as part of the patch. Please submit a PR on WPT GitHub so that we do not lose this test when reimporting WPT tests in WebKit. View in context: https://bugs.webkit.org/attachment.cgi?id=332491&action=review > Source/WebCore/Modules/cache/DOMCache.cpp:336 > + RefPtr<SharedBuffer> data = SharedBuffer::create(); Use auto or maybe we can directly write data = SharedBuffer::create() in the capturing lambda? > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:107 > + RefPtr<SharedBuffer> data = SharedBuffer::create(); Ditto. > Source/WebCore/Modules/fetch/FetchResponse.h:34 > +#include "ReadableStreamSink.h" Probably do not need ReadableStreamSink.h. We might also just need to declare ReadableStreamChunk without having to include ReadableStreamChunk.h > Source/WebCore/Modules/streams/ReadableStreamChunk.h:26 > + Two lines. > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:54 > + if (m_callback) { Would be good to find a way to not need that check in the future. It is better to keep it for now since we have clearCallback(). > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:55 > + auto chunk = ReadableStreamChunk { buffer.data(), buffer.length() }; Can be written as ReadableStreamChunk chunk { ... } > Source/WebCore/Modules/streams/ReadableStreamSink.cpp:62 > + if (m_callback) It is probably better to keep moving m_callback so that we free it as soon as possible. > Source/WebCore/Modules/streams/ReadableStreamSink.h:32 > +#include "ReadableStreamChunk.h" Might just need to declare ReadableStreamChunk > Source/WebCore/Modules/streams/ReadableStreamSink.h:54 > static Ref<ReadableStreamToSharedBufferSink> create(Callback&& callback) { return adoptRef(*new ReadableStreamToSharedBufferSink(WTFMove(callback))); } Preexisting issue but can we make ReadableStreamToSharedBufferSink constructor explicit? > LayoutTests/imported/w3c/ChangeLog:9 > + * web-platform-tests/service-workers/service-worker/fetch-event-respond-with-readable-stream-chunk.https.html: Added. I like that such test is added there! > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-respond-with-readable-stream-chunk.https.html:26 > + t.add_cleanup(() => iframe.remove()) Can we do the following: var response = await iframe.contentWindow.fetch('body-stream'); assert_equals(await response.text(), '...); Then you can load any HTML as iframe. > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-readable-stream-chunk-iframe.html:24 > +}).then(text => parent.done(text)) Can we use directly response.text() if we keep the iframe? > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-readable-stream-chunk-worker.js:12 > + console.log('ABCD'); Do we need this console log? > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/resources/fetch-event-respond-with-readable-stream-chunk-worker.js:50 > + .run(1); Do we need this setTimeout(1)? Can we do something simpler like: async controller => { controller.enqueue(...); await Promise.resolve(); ... }

Created attachment 332579 [details] Patch for landing

Created attachment 332586 [details] Fixing GTK

Comment on attachment 332586 [details] Fixing GTK Clearing flags on attachment: 332586 Committed r227760: <https://trac.webkit.org/changeset/227760>

All reviewed patches have been landed. Closing bug.

Comment on attachment 332586 [details] Fixing GTK View in context: https://bugs.webkit.org/attachment.cgi?id=332586&action=review > Source/WebCore/Modules/cache/DOMCache.cpp:344 > + data->append(reinterpret_cast<const char*>(chunk->data), chunk->size); const_cast?

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