Bug 162892

Summary: [Fetch API] Use ReadableStream pull to transfer binary data to stream when application needs it
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

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.