Add Streams API support to XMLHttpRequest

Add Streams API support to XMLHttpRequest

Hidden behind the --enable-experimental-webkit-features flag.

Original Streams API spec is:
https://dvcs.w3.org/hg/streams-api/raw-file/tip/Overview.htm

How to read data from Stream in JavaScript is still under
discussion at this thread
http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0706.html
but integration with XHR is not controversial.

So, to allow APIs depending on Stream but not on JavaScript
Stream read/write API to start development (e.g. Media Source
Extensions API
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html),
I'd like to land XHR integration part.

BUG=169957
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157609

[tyoshino (SeeGerritForStatus), 2013-07-09 07:07:01]

This CL depends on
https://codereview.chromium.org/15817013/

Could you please do early-stage review?

[abarth-chromium, 2013-07-09 17:54:32]

Please add a link to the spec in the description.

[abarth-chromium, 2013-07-09 17:58:21]

Looks like a good start.  Obviously we'll need a bunch of tests, but the
implementation looks to be on the right track.

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:373: ASSERT_NOT_REACHED();
Can this ASSERT be reached if RuntimeEnabledFeatures::streamEnabled() is false? 
Maybe we need to set m_responseTypeCode = ResponseTypeDefault in the default
case.

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:392: return "";
Can you wrap all these strings in ASCIILiteral(..) ?

[tyoshino (SeeGerritForStatus), 2013-07-09 19:48:22]

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:373: ASSERT_NOT_REACHED();
On 2013/07/09 17:58:21, abarth wrote:
> Can this ASSERT be reached if RuntimeEnabledFeatures::streamEnabled() is
false? 
> Maybe we need to set m_responseTypeCode = ResponseTypeDefault in the default
> case.

Oh, right. As I added "stream" to the enum in the IDL file, this case can be
reached.

It's still good to ASSERT_NOT_REACHED() for the unexpected input. I moved the
flag to
L371 and made it just return when it's false as well as the generated binding
code.
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/obj/gen/...

https://codereview.chromium.org/18883002/diff/1005/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:392: return "";
On 2013/07/09 17:58:21, abarth wrote:
> Can you wrap all these strings in ASCIILiteral(..) ?

Done.

[tyoshino (SeeGerritForStatus), 2013-07-09 19:50:51]

On 2013/07/09 17:58:21, abarth wrote:
> Looks like a good start.  Obviously we'll need a bunch of tests, but the
> implementation looks to be on the right track.

Thanks. Yes, I have a pending CL adding Stream support to FileReader.
https://codereview.chromium.org/18635004/ (not ready yet)
To test Stream we need to introduce some reading method. It could be
FileReader, StreamReader, or something else accesses it via blob URL.
It's TBD.

[michaeln, 2013-07-24 03:24:37]

We have a latent desire to avoid sending bulky response data to renderers, then
back to the main process, then to renderers again when it comes to how
.responseBlob is implemented. This preliminary .responseStream impl has that
back-and-forth characteristic too. Do we want that for this initial cut at this?

[michaeln, 2013-07-24 03:57:21]

https://codereview.chromium.org/18883002/diff/8001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/8001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:1194: m_responseStream->addData(data, len);
Also, it it a desired characteristic of this implementation that the network be
gated such that it run too far ahead of the stream consumer? And if so, is that
flow control an important characteristic? If that is the behavior we want, I
think this approach (stuffing data into the stream at this callsite) makes that
more challenging to achieve then some alternatives (that would stuff data into
the stream in the browser process).

[michaeln, 2013-07-24 03:57:45]

On 2013/07/24 03:57:21, michaeln wrote:
>
https://codereview.chromium.org/18883002/diff/8001/Source/core/xml/XMLHttpReq...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/18883002/diff/8001/Source/core/xml/XMLHttpReq...
> Source/core/xml/XMLHttpRequest.cpp:1194: m_responseStream->addData(data, len);
> Also, it it a desired characteristic of this implementation that the network
be
> gated such that it run too far ahead of the stream consumer? And if so, is
that
> flow control an important characteristic? If that is the behavior we want, I
> think this approach (stuffing data into the stream at this callsite) makes
that
> more challenging to achieve then some alternatives (that would stuff data into
> the stream in the browser process).

s/b "doesn't run too far ahead"

[tyoshino (SeeGerritForStatus), 2013-08-06 19:15:02]

On 2013/07/24 03:24:37, michaeln wrote:
> We have a latent desire to avoid sending bulky response data to renderers,
then
> back to the main process, then to renderers again when it comes to how
> .responseBlob is implemented. This preliminary .responseStream impl has that
> back-and-forth characteristic too. Do we want that for this initial cut at
this?

Filed a bug.
crbug.com/269055

It makes sense but I'd like to deliver Stream asap for customers.

[tyoshino (SeeGerritForStatus), 2013-08-06 19:18:41]

Rebased. Ready basically.

I'm planning to implement Stream reading feature on FileReader temporarily. Wdyt
Adam?
https://codereview.chromium.org/18635004/

[tyoshino (SeeGerritForStatus), 2013-08-06 19:19:04]

On 2013/08/06 19:18:41, tyoshino wrote:
> Rebased. Ready basically.
> 
> I'm planning to implement Stream reading feature on FileReader temporarily.
Wdyt
> Adam?
> https://codereview.chromium.org/18635004/

And, Kinuko.

[abarth-chromium, 2013-08-06 19:35:55]

I'm not sure it's worth implementing streams in this temporary way.  Why not
implement them using the better approach of keeping the data on the browser side
until needed?

https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:396: return ASCIILiteral("");
Don't bother adding ASCIILiteral.  It's a no-op and we're removing it from the
codebase.

https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1195: m_responseStream->addData(data, len);
Do we have to route the data to the render process?  I thought the point of
streams was to keep the data in the browser process until it's needed.

[tyoshino (SeeGerritForStatus), 2013-08-06 22:14:42]

On 2013/08/06 19:35:55, abarth wrote:
> I'm not sure it's worth implementing streams in this temporary way.  Why not
> implement them using the better approach of keeping the data on the browser
side
> until needed?
> 

Media Stream Extension people are asking us to provide Stream. The points of
Stream+XHR are
- allow starting reading binary data in LOADING state
- discard partially read data to reduce memory usage and enable streaming very
long data
- and, as you said, it's better that Stream data is kept inside browser process
until it's consumed

first two points would be addressed with this impl

>
https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
> Source/core/xml/XMLHttpRequest.cpp:396: return ASCIILiteral("");
> Don't bother adding ASCIILiteral.  It's a no-op and we're removing it from the
> codebase.
> 

oh, i'll remove it again.

>
https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
> Source/core/xml/XMLHttpRequest.cpp:1195: m_responseStream->addData(data, len);
> Do we have to route the data to the render process?  I thought the point of
> streams was to keep the data in the browser process until it's needed.

it wound be not small work. i'd like to work on it after providing initial
implementation. this is a bug for that.
crbug.com/269055

[abarth-chromium, 2013-08-07 00:11:13]

Ok, if we're going to go with the approach of having the data flow through the
render process, then this implementation looks very reasonable.  We'll need some
tests, of course. :)

[michaeln, 2013-08-07 00:21:24]

On 2013/08/07 00:11:13, abarth wrote:
> Ok, if we're going to go with the approach of having the data flow through the
> render process, then this implementation looks very reasonable.  We'll need
some
> tests, of course. :)

And probably some logic to error out in the case where the consumer of a large
stream isn't reading fast enough.

[kinuko, 2013-08-07 03:46:57]

Same comment, the way how responseBlob was added was unfortunate and it's going
in the same way, but it'd work as a intermediary solution and might be slightly
better in terms of memory consumption (in average case).

On 2013/08/07 00:21:24, michaeln wrote:
> On 2013/08/07 00:11:13, abarth wrote:
> > Ok, if we're going to go with the approach of having the data flow through
the
> > render process, then this implementation looks very reasonable.  We'll need
> some
> > tests, of course. :)
> 
> And probably some logic to error out in the case where the consumer of a large
> stream isn't reading fast enough.

Probably streams side should have a mechanism to handle this, e.g. to spill out
to a file? But until then we'll need some guard to prevent excessive memory
usage.

[michaeln, 2013-08-07 18:12:01]

On 2013/08/07 03:46:57, kinuko wrote:
> Same comment, the way how responseBlob was added was unfortunate and it's
going
> in the same way, but it'd work as a intermediary solution and might be
slightly
> better in terms of memory consumption (in average case).
> 
> On 2013/08/07 00:21:24, michaeln wrote:
> > On 2013/08/07 00:11:13, abarth wrote:
> > > Ok, if we're going to go with the approach of having the data flow through
> the
> > > render process, then this implementation looks very reasonable.  We'll
need
> > some
> > > tests, of course. :)
> > 
> > And probably some logic to error out in the case where the consumer of a
large
> > stream isn't reading fast enough.
> 
> Probably streams side should have a mechanism to handle this, e.g. to spill
out
> to a file? But until then we'll need some guard to prevent excessive memory
> usage.

Streams shouldn't need to spill to disk ever if i understand the intent. The
producer should be throttled by a consumers laziness at consuming. At least
that's how i understand the intent. The stream impl should maintain a smallish
buffer of what the produced data which the consumers takes from. As the consumer
drains the buffer, the stream impl fills the buffer with more data from the
producer.

If the "producer" of the stream data is a network source, when the consumer
stops reading, the system should stop reading from the network once the
"smallish" buffer is full. If the producer of the stream data is a script, the
script should stop adding data if the buffer is full, and resume adding data
once the consumer has drained it some. Flow control is the feature.

There is no mechanism in this current approach to throttle the network read when
the smallish buffer is full.

[tyoshino (SeeGerritForStatus), 2013-08-22 09:39:46]

On 2013/08/07 18:12:01, michaeln wrote:
> Streams shouldn't need to spill to disk ever if i understand the intent. The
> producer should be throttled by a consumers laziness at consuming. At least
...

Yes. I'd like to put back pressure to loader and TCP stack.

Stream with spill-to-disk could be used as micro optimization for
store-and-consume-later type app, but sounds not big deal.

For now, at least to prevent browser crash I've added total memory consumption
limit similar to one in blob_storage_controller to stream_registry.
https://codereview.chromium.org/22908008/

[tyoshino (SeeGerritForStatus), 2013-08-23 03:31:44]

https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/23001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:396: return ASCIILiteral("");
On 2013/08/06 19:35:55, abarth wrote:
> Don't bother adding ASCIILiteral.  It's a no-op and we're removing it from the
> codebase.

Done.

[tyoshino (SeeGerritForStatus), 2013-08-23 03:37:38]

On 2013/08/07 00:11:13, abarth wrote:
> Ok, if we're going to go with the approach of having the data flow through the
> render process, then this implementation looks very reasonable.  We'll need
some
> tests, of course. :)

Each user of Stream writes their own layout tests. So, we can move forward
without read operation tests, I think.

I just added some simple tests to check responseType and response value without
pulling data from Stream.

[tyoshino (SeeGerritForStatus), 2013-08-23 03:38:20]

Rebased and added layout tests.

Ready for review again.

[acolwell GONE FROM CHROMIUM, 2013-08-23 23:54:01]

https://codereview.chromium.org/18883002/diff/39001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/39001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:849: 
I think you need something like the following here:
if (m_responseStream && m_state < DONE)
        m_responseStream->finalize();

so that a loader reading from the stream will actually get notified that the
stream has ended if abort() gets called on this object.

[tyoshino (SeeGerritForStatus), 2013-08-27 08:51:51]

https://codereview.chromium.org/18883002/diff/39001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/18883002/diff/39001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:849: 
On 2013/08/23 23:54:01, acolwell wrote:
> I think you need something like the following here:
> if (m_responseStream && m_state < DONE)
>         m_responseStream->finalize();
> 
> so that a loader reading from the stream will actually get notified that the
> stream has ended if abort() gets called on this object.

Right, thanks. I'd like to introduce and call abort() instead of finalize
to tell the receiver it's failed.

Chromium side change needed for this.
https://codereview.chromium.org/23543002/

[tyoshino (SeeGerritForStatus), 2013-08-29 06:40:13]

On 2013/08/27 08:51:51, tyoshino wrote:
> Right, thanks. I'd like to introduce and call abort() instead of finalize
> to tell the receiver it's failed.
> 
> Chromium side change needed for this.
> https://codereview.chromium.org/23543002/

This CL has been landed

and split BlobRegistry part of this CL into
https://codereview.chromium.org/23490013/

[tyoshino (SeeGerritForStatus), 2013-09-02 07:21:34]

On 2013/08/29 06:40:13, tyoshino wrote:
> and split BlobRegistry part of this CL into
> https://codereview.chromium.org/23490013/

This CL has also been landed.

PTAL

[tyoshino (SeeGerritForStatus), 2013-09-05 09:37:57]

On 2013/09/02 07:21:34, tyoshino wrote:
> On 2013/08/29 06:40:13, tyoshino wrote:
> > and split BlobRegistry part of this CL into
> > https://codereview.chromium.org/23490013/
> 
> This CL has also been landed.
> 
> PTAL

Kinuko-san,

Could you also review this?

[kinuko, 2013-09-05 13:48:40]

I think this lgtm (as a stopgap for MSE)

https://codereview.chromium.org/18883002/diff/65002/LayoutTests/http/tests/xm...
File LayoutTests/http/tests/xmlhttprequest/response-stream-abort.html (right):

https://codereview.chromium.org/18883002/diff/65002/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/response-stream-abort.html:78: 
nit: maybe check xhr.response is not null before abort()?

[tyoshino (SeeGerritForStatus), 2013-09-06 04:18:24]

On 2013/07/09 17:54:32, abarth wrote:
> Please add a link to the spec in the description.

Sorry, I forgot to address this comment. Done.

[tyoshino (SeeGerritForStatus), 2013-09-06 04:24:54]

https://codereview.chromium.org/18883002/diff/65002/LayoutTests/http/tests/xm...
File LayoutTests/http/tests/xmlhttprequest/response-stream-abort.html (right):

https://codereview.chromium.org/18883002/diff/65002/LayoutTests/http/tests/xm...
LayoutTests/http/tests/xmlhttprequest/response-stream-abort.html:78: 
On 2013/09/05 13:48:41, kinuko wrote:
> nit: maybe check xhr.response is not null before abort()?

Done.

[abarth-chromium, 2013-09-11 07:06:37]

Sorry for the delay.  LGTM!

[commit-bot: I haz the power, 2013-09-11 16:17:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoshino@chromium.org/18883002/65003

[commit-bot: I haz the power, 2013-09-11 22:33:44]

Change committed as 157609