Bug 182008 - FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies
Summary: FetchResponse should support ConsumeData callback on chunk data is received: ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: GSkachkov
URL:
Keywords: InRadar
Depends on:
Blocks: 181600
  Show dependency treegraph
 
Reported: 2018-01-23 13:04 PST by GSkachkov
Modified: 2018-01-31 16:15 PST (History)
6 users (show)

See Also:


Attachments
Patch (11.17 KB, patch)
2018-01-23 13:23 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (23.20 KB, patch)
2018-01-26 11:52 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (23.21 KB, patch)
2018-01-26 11:55 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (23.29 KB, patch)
2018-01-28 09:32 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch for landing (22.00 KB, patch)
2018-01-29 14:22 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing GTK (22.02 KB, patch)
2018-01-29 15:07 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2018-01-23 13:04:08 PST
FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies according to suggestion from 181600
Comment 1 GSkachkov 2018-01-23 13:23:38 PST
Created attachment 332067 [details]
Patch

WiP for check progress and check tests
Comment 2 youenn fablet 2018-01-23 16:52:07 PST
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.
Comment 3 GSkachkov 2018-01-23 23:36:04 PST
(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.
Comment 4 GSkachkov 2018-01-26 11:52:44 PST
Created attachment 332396 [details]
Patch

Patch for review
Comment 5 GSkachkov 2018-01-26 11:55:37 PST
Created attachment 332397 [details]
Patch

Rebased patch for review
Comment 6 youenn fablet 2018-01-26 12:29:11 PST
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.
Comment 7 GSkachkov 2018-01-26 12:53:22 PST
Thanks for review! I hope to back with fixes during week.
Comment 8 Radar WebKit Bug Importer 2018-01-26 18:17:43 PST
<rdar://problem/36932534>
Comment 9 GSkachkov 2018-01-28 09:32:47 PST
Created attachment 332491 [details]
Patch

Patch with fixed comments
Comment 10 youenn fablet 2018-01-28 10:12:50 PST
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();
  ...
}
Comment 11 youenn fablet 2018-01-29 14:22:40 PST
Created attachment 332579 [details]
Patch for landing
Comment 12 youenn fablet 2018-01-29 15:07:22 PST
Created attachment 332586 [details]
Fixing GTK
Comment 13 WebKit Commit Bot 2018-01-29 15:39:26 PST
Comment on attachment 332586 [details]
Fixing GTK

Clearing flags on attachment: 332586

Committed r227760: <https://trac.webkit.org/changeset/227760>
Comment 14 WebKit Commit Bot 2018-01-29 15:39:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Saam Barati 2018-01-29 16:16:22 PST
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?
Comment 16 youenn fablet 2018-01-31 16:15:28 PST
Submitted web-platform-tests pull request: https://github.com/w3c/web-platform-tests/pull/9327