WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
182008
FetchResponse should support ConsumeData callback on chunk data is received: handling ReadableStream bodies
https://bugs.webkit.org/show_bug.cgi?id=182008
Summary
FetchResponse should support ConsumeData callback on chunk data is received: ...
GSkachkov
Reported
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
GSkachkov
Comment 1
2018-01-23 13:23:38 PST
Created
attachment 332067
[details]
Patch WiP for check progress and check tests
youenn fablet
Comment 2
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.
GSkachkov
Comment 3
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.
GSkachkov
Comment 4
2018-01-26 11:52:44 PST
Created
attachment 332396
[details]
Patch Patch for review
GSkachkov
Comment 5
2018-01-26 11:55:37 PST
Created
attachment 332397
[details]
Patch Rebased patch for review
youenn fablet
Comment 6
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.
GSkachkov
Comment 7
2018-01-26 12:53:22 PST
Thanks for review! I hope to back with fixes during week.
Radar WebKit Bug Importer
Comment 8
2018-01-26 18:17:43 PST
<
rdar://problem/36932534
>
GSkachkov
Comment 9
2018-01-28 09:32:47 PST
Created
attachment 332491
[details]
Patch Patch with fixed comments
youenn fablet
Comment 10
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(); ... }
youenn fablet
Comment 11
2018-01-29 14:22:40 PST
Created
attachment 332579
[details]
Patch for landing
youenn fablet
Comment 12
2018-01-29 15:07:22 PST
Created
attachment 332586
[details]
Fixing GTK
WebKit Commit Bot
Comment 13
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
>
WebKit Commit Bot
Comment 14
2018-01-29 15:39:28 PST
All reviewed patches have been landed. Closing bug.
Saam Barati
Comment 15
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?
youenn fablet
Comment 16
2018-01-31 16:15:28 PST
Submitted web-platform-tests pull request:
https://github.com/w3c/web-platform-tests/pull/9327
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug