Bug 162892 - [Fetch API] Use ReadableStream pull to transfer binary data to stream when application needs it
Summary: [Fetch API] Use ReadableStream pull to transfer binary data to stream when ap...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-04 01:49 PDT by youenn fablet
Modified: 2016-10-06 02:46 PDT (History)
1 user (show)

See Also:


Attachments
Patch (8.83 KB, patch)
2016-10-04 02:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (8.82 KB, patch)
2016-10-06 02:12 PDT, 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 youenn fablet 2016-10-04 01:49:42 PDT
When stream is not pulling data, it might be best to just store it in WebCore.
If there is a need for it, knowing how much data is stored in WebCore could allow us to request the network process to stop sending us data until we actually need it.
Comment 1 youenn fablet 2016-10-04 02:02:53 PDT
Created attachment 290584 [details]
Patch
Comment 2 Alex Christensen 2016-10-04 07:57:05 PDT
Comment on attachment 290584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290584&action=review

> Source/WebCore/Modules/fetch/FetchResponse.cpp:290
> +    ASSERT(m_readableStreamSource);

This has two call sites, one of which checks m_readableStreamSource right before calling.  Would it hurt anything to move the check to here?

> Source/WebCore/Modules/fetch/FetchResponse.cpp:300
> +        if (!m_readableStreamSource->enqueue(body().consumer().takeAsArrayBuffer())) {

Should we assert m_readableStreamSource at the beginning of this function, or just if body.consumer.hasData?
Comment 3 youenn fablet 2016-10-04 08:05:05 PDT
Comment on attachment 290584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=290584&action=review

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:290
>> +    ASSERT(m_readableStreamSource);
> 
> This has two call sites, one of which checks m_readableStreamSource right before calling.  Would it hurt anything to move the check to here?

Yeah, not sure whether this is assert is all that useful.

This assert is just there in case we use closeStream() somewhere else in the future.
It does not hurt to have it and might help future debugging.
That said, closeStream() is a private method.

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:300
>> +        if (!m_readableStreamSource->enqueue(body().consumer().takeAsArrayBuffer())) {
> 
> Should we assert m_readableStreamSource at the beginning of this function, or just if body.consumer.hasData?

Right, I'll add an assert at the beginning of this function.
Comment 4 youenn fablet 2016-10-06 02:12:27 PDT
Created attachment 290798 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2016-10-06 02:46:22 PDT
Comment on attachment 290798 [details]
Patch for landing

Clearing flags on attachment: 290798

Committed r206857: <http://trac.webkit.org/changeset/206857>
Comment 6 WebKit Commit Bot 2016-10-06 02:46:25 PDT
All reviewed patches have been landed.  Closing bug.