[webkit-reviews] review granted: [Bug 182008] FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies : [Attachment 332491] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted GSkachkov <gskachkov at gmail.com>'s
request for review:
Bug 182008: FetchResponse should support ConsumeData callback on chunk data is
received: handling ReadableStream bodies
https://bugs.webkit.org/show_bug.cgi?id=182008

Attachment 332491: Patch

https://bugs.webkit.org/attachment.cgi?id=332491&action=review




--- Comment #10 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 332491
  --> https://bugs.webkit.org/attachment.cgi?id=332491
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-read
able-stream-chunk.https.html: Added.

I like that such test is added there!

>
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetc
h-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/reso
urces/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/reso
urces/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/reso
urces/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();
  ...
}