FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[GSkachkov]

FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies according to suggestion from 181600

[GSkachkov]

Created attachment 332067 [details]
Patch

WiP for check progress and check tests

[youenn fablet]

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.

[GSkachkov]

(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.

[GSkachkov]

Created attachment 332396 [details]
Patch

Patch for review

[GSkachkov]

Created attachment 332397 [details]
Patch

Rebased patch for review

[youenn fablet]

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.

[GSkachkov]

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

[Radar WebKit Bug Importer]

<rdar://problem/36932534>

[GSkachkov]

Created attachment 332491 [details]
Patch

Patch with fixed comments

[youenn fablet]

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();
  ...
}

[youenn fablet]

Created attachment 332579 [details]
Patch for landing

[youenn fablet]

Created attachment 332586 [details]
Fixing GTK

[WebKit Commit Bot]

Comment on attachment 332586 [details]
Fixing GTK

Clearing flags on attachment: 332586

Committed r227760: <https://trac.webkit.org/changeset/227760>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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?

[youenn fablet]

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