Change BodyStreamBuffer to be FetchDataConsumerHandle-based and enable backpressure in Fetch API

Change BodyStreamBuffer to be FetchDataConsumerHandle-based and enable backpressure in Fetch API

This CL replaces BodyStreamBuffer with a new FetchDataConsumerHandle-based
implementation and thus enables backpressure in Fetch API.

Major changes:
1. BlobDataHandle and old BodyStreamBuffer-based interfaces to body data are
   replaced with the new FetchDataConsumerHandle-based BodyStreamBuffer.
   The old BodyStreamBuffer, BlobDataHandle-based interfaces and related
   methods/classes/tests are removed.
2. Now Body::ReadableStreamSource goes into Streaming state on construction and
   takes the reader of FetchDataConsumerHandle.
   Therefore, all accesses to body data now should take createDrainingStream().
   Also, DrainingBodyStreamBuffer is introduced to notify
   Body::ReadableStreamSource after createDrainingStream() is done.
3. populateWebServiceWorkerResponse() no longer sets BlobDataHandle.
   Callers of populateWebServiceWorkerResponse()
   (RespondWithObserver::responseWasFulfilled() and Cache::putImpl()) are
   modified accordingly.

Body:
- blobDataHandle() and buffer() are merged into createDrainingStream() [1][2].
- Body::isBodyConsumed() is removed, because it is now always true.
  This was used to check whether we have to take createDrainingStream(),
  but now we always have to take createDrainingStream() [2].
- Body::m_fetchDataLoader is moved to BodyStreamBuffer::m_fetchDataLoader [2].
- FetchDataLoader::Client methods in Body no longer call
  m_streamSource->close()/error(). Now they are done in
  Body::ReadableStreamSource::didFetchDataLoadFinishedFromDrainingStream() [2].
- Removed unused code: Body::didFinishLoadingViaStream().
- readAllAndCreateBlobHandle(), BlobHandleCreator, BlobHandleReceiver and
  associated methods are removed and replaced with FetchDataLoader.

Body::ReadableStreamSource:
- Loading via FileReaderLoader is removed.
- We no longer put data in |m_stream| that are not immediately consumed and
  thus we no longer call |m_stream->readInternal()| in createDrainingStream().

Request:
- refreshBody() is introduced and called after m_request->buffer() is set [1].

FetchRequestData:
- |m_blobDataHandle| is replaced with |m_buffer| [1].
- Some methods now require ExecutionContext* as an argument to set |m_buffer|.

Response:
- createInternalDrainingStream() is introduced and replaces internalBuffer().

FetchResponseData:
- Some methods now require ExecutionContext* as an argument to make clones.
- startTee() is replaced with DataConsumerHandleTee.

cache-put.js:
- Add a layout test for getReader() after cache.put() is completed.
  This test confirms notification by DrainingBodyStreamBuffer works correctly.

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

[hiroshige, 2015-06-23 07:51:42]

hiroshige@chromium.org changed reviewers:
+ yhirano@chromium.org

[hiroshige, 2015-06-23 07:59:08]

yhirano@, could you take a look at this?

See diff from "BASE" Patch Set.
This is the cumulative CL that includes all changes I made, except for the CLs
already uploaded (="BASE").

[yhirano, 2015-06-25 10:45:31]

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/cachest...
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:212:
m_resolver->reject(V8ThrowException::createTypeError(state->isolate(), "FOXES
ARE CUTE")); // FIXME error message
Please use TODO.
I do like messages like "network error" as this is user-exposed.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:67: BodyStreamBuffer2* createDrainingStream()
If close / error is properly handled, I think this function effectively does

{
  m_reader.clear();
  ASSERT(!m_drained);
  m_drained = true;
  return m_bodyStreamBuffer;
}

Is that right?

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:81: if (m_state == Closed)
This block can be deleted.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:123: m_streamNeedsMore = true;
+ASSERT(!m_streamNeedsMore);

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:134: if (m_drained)
I believe m_drained will never be set here because we have Body::lockBody and
ReadableStream::state. IIUC m_drained should not be used outside ASSERT.

[yhirano, 2015-06-25 11:11:32]

https://codereview.chromium.org/1192913007/diff/260001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/260001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:117: void didGetReadable() override
Consider using zero-length read in order to detect an error.

[hiroshige, 2015-06-25 11:27:28]

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/cachest...
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:212:
m_resolver->reject(V8ThrowException::createTypeError(state->isolate(), "FOXES
ARE CUTE")); // FIXME error message
On 2015/06/25 10:45:31, yhirano wrote:
> Please use TODO.
> I do like messages like "network error" as this is user-exposed.

Done.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:67: BodyStreamBuffer2* createDrainingStream()
On 2015/06/25 10:45:31, yhirano wrote:
> If close / error is properly handled, I think this function effectively does
> 
> {
>   m_reader.clear();
>   ASSERT(!m_drained);
>   m_drained = true;
>   return m_bodyStreamBuffer;
> }
> 
> Is that right?

Yes, currently, but as we discussed offline, we have to add close/error
notification from the drained stream to |m_stream|.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:81: if (m_state == Closed)
On 2015/06/25 10:45:31, yhirano wrote:
> This block can be deleted.

Done.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:123: m_streamNeedsMore = true;
On 2015/06/25 10:45:31, yhirano wrote:
> +ASSERT(!m_streamNeedsMore);

Done.

https://codereview.chromium.org/1192913007/diff/180001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:134: if (m_drained)
On 2015/06/25 10:45:31, yhirano wrote:
> I believe m_drained will never be set here because we have Body::lockBody and
> ReadableStream::state. IIUC m_drained should not be used outside ASSERT.

Done.

https://codereview.chromium.org/1192913007/diff/260001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/260001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:117: void didGetReadable() override
On 2015/06/25 11:11:32, yhirano wrote:
> Consider using zero-length read in order to detect an error.

Done.

[hiroshige, 2015-07-01 19:44:59]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-01 19:45:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192913007/610001

[commit-bot: I haz the power, 2015-07-01 20:19:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-01 20:19:04]

Dry run: Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[hiroshige, 2015-07-02 08:22:10]

I think now this CL is ready for review again; could you take a look at again?

After Patch Set 15, I've
- Replaced Request's BlobDataHandle-based interface with
FetchDataConsumerHandle,
- Make ReadableStreamSource to obtain Reader from construction (unless drained),
- Replaced buffer()/internalBuffer() to
createDrainingStream()/createInternalDrainingStream().
- Removed isBodyConsumed(), and
- Fixed bugs.

https://codereview.chromium.org/1192913007/diff/670001/LayoutTests/http/tests...
File LayoutTests/http/tests/workers/resources/performance-timeline-worker.js
(right):

https://codereview.chromium.org/1192913007/diff/670001/LayoutTests/http/tests...
LayoutTests/http/tests/workers/resources/performance-timeline-worker.js:37:
return new Promise(function(resolve, reject) {
This is temporary fix: |performance| entries are updated almost the same time
when .text() is resolved, but not necessarily before .text() is resolved. So I
inserted a sleep, but I'll resolve this issue next week with ksakamoto@.

[yhirano, 2015-07-03 04:42:28]

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:27: // This class is an ActiveDOMObject subclass
only for holding the
We do no longer need ExecutionContext, right?

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:32: enum State {
Now we can remove this enum and rely on the stream's state.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:117: ASSERT(m_bodyStreamBuffer);
ASSERT(m_drained)?

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:375: void Body::stop()
If you do nothing you can delete this function.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:25: // #define
createDebugHandleAlways(handle) FetchDebugDataConsumerHandle::create(handle,
__FILE__, __LINE__)
Can we delete this block?

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:226: BodyStreamBuffer* leak();
|leakBuffer| or |releaseBuffer| might be a better name? |b->leak()| looks like
|b.leakPtr()| and may confuse readers.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:230: explicit
DrainingBodyStreamBuffer(BodyStreamBuffer*,
BodyStreamBuffer::DrainingStreamNotificationClient*);
-explicit

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:72: temporaryBody =
inputRequest->createDrainingStream();
The spec intends to not mutate |inputRequest|'s body until it is actually used
(e.g. the constructor throws, |body| property is specified, etc). I think
createDrainingStream has side-effect, right?
If not, please add comments.

[yhirano, 2015-07-03 07:20:14]

IIUC all required CLs are landed (except
https://codereview.chromium.org/1217943007/ which is not strictly required).
Can you rebase again?

[hiroshige, 2015-07-05 07:30:25]

Rebased in Patch Set 37, and reflected comments in Patch Set 38.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:27: // This class is an ActiveDOMObject subclass
only for holding the
On 2015/07/03 04:42:28, yhirano wrote:
> We do no longer need ExecutionContext, right?

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:32: enum State {
On 2015/07/03 04:42:28, yhirano wrote:
> Now we can remove this enum and rely on the stream's state.

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:117: ASSERT(m_bodyStreamBuffer);
On 2015/07/03 04:42:28, yhirano wrote:
> ASSERT(m_drained)?

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:375: void Body::stop()
On 2015/07/03 04:42:28, yhirano wrote:
> If you do nothing you can delete this function.

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:25: // #define
createDebugHandleAlways(handle) FetchDebugDataConsumerHandle::create(handle,
__FILE__, __LINE__)
On 2015/07/03 04:42:28, yhirano wrote:
> Can we delete this block?

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:226: BodyStreamBuffer* leak();
On 2015/07/03 04:42:28, yhirano wrote:
> |leakBuffer| or |releaseBuffer| might be a better name? |b->leak()| looks like
> |b.leakPtr()| and may confuse readers.

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:230: explicit
DrainingBodyStreamBuffer(BodyStreamBuffer*,
BodyStreamBuffer::DrainingStreamNotificationClient*);
On 2015/07/03 04:42:28, yhirano wrote:
> -explicit

Done.

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/R...
File Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1192913007/diff/690001/Source/modules/fetch/R...
Source/modules/fetch/Request.cpp:72: temporaryBody =
inputRequest->createDrainingStream();
On 2015/07/03 04:42:28, yhirano wrote:
> The spec intends to not mutate |inputRequest|'s body until it is actually used
> (e.g. the constructor throws, |body| property is specified, etc). I think
> createDrainingStream has side-effect, right?
Yes.
I modified the code to remove the side effects on |inputRequest|'s body.

[yhirano, 2015-07-06 03:25:39]

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:59: if (m_stream->stateInternal() ==
ReadableByteStream::Closed)
[optional] It would be good to move this block to close() function.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:61: if (m_stream->stateInternal() ==
ReadableByteStream::Errored)
[optional] It would be good to move this block to error() function.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:87: void obtainReader()
This function can be private.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.cpp:46: }
+empty line

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:8: #include
"core/dom/DOMArrayBuffer.h"
-includes

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:8: #include
"core/dom/DOMArrayBuffer.h"
Please add TODO if you want to re-add tests later. Otherwise, please delete this
file.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:17: class BlobDataHandle;
We don't need this declaration.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
File Source/modules/fetch/Request.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Request.h:62: void setBuffer(BodyStreamBuffer*);
This function can be private.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Request.h:65: void refreshBody();
ditto

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
File Source/modules/fetch/Response.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Response.h:69: String internalMIMEType() const;
It might be good to place this function just after mimeType().

[hiroshige, 2015-07-06 05:47:08]

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:59: if (m_stream->stateInternal() ==
ReadableByteStream::Closed)
On 2015/07/06 03:25:38, yhirano wrote:
> [optional] It would be good to move this block to close() function.
Can we assume |m_stream|'s state changes to Closed/Errored are always via
ReadableStreamSource::close()/error()?
E.g.
m_stream->closeInternal() can be called from readInternalPostAction():
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
m_stream->error() can be called when ActiveDOMObject is stopped:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.cpp:46: }
On 2015/07/06 03:25:39, yhirano wrote:
> +empty line

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/F...
File Source/modules/fetch/FetchResponseData.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/F...
Source/modules/fetch/FetchResponseData.h:17: class BlobDataHandle;
On 2015/07/06 03:25:39, yhirano wrote:
> We don't need this declaration.

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
File Source/modules/fetch/Request.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Request.h:62: void setBuffer(BodyStreamBuffer*);
On 2015/07/06 03:25:39, yhirano wrote:
> This function can be private.

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Request.h:65: void refreshBody();
On 2015/07/06 03:25:39, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
File Source/modules/fetch/Response.h (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/R...
Source/modules/fetch/Response.h:69: String internalMIMEType() const;
On 2015/07/06 03:25:39, yhirano wrote:
> It might be good to place this function just after mimeType().

Done.

[yhirano, 2015-07-06 06:14:01]

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:59: if (m_stream->stateInternal() ==
ReadableByteStream::Closed)
On 2015/07/06 05:47:07, hiroshige wrote:
> On 2015/07/06 03:25:38, yhirano wrote:
> > [optional] It would be good to move this block to close() function.
> Can we assume |m_stream|'s state changes to Closed/Errored are always via
> ReadableStreamSource::close()/error()?
> E.g.
> m_stream->closeInternal() can be called from readInternalPostAction():
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> m_stream->error() can be called when ActiveDOMObject is stopped:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You're right. Though currently all close / error can be done through the
conroller (except the ActiveDOMObject::stop case which we don't need to behave
consistently), we will not be sure when the v8 extra comes in.

https://codereview.chromium.org/1192913007/diff/750001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1192913007/diff/750001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:45: // TODO(yhirano): This method will
be destructive.
Please remove this comment.

[yhirano, 2015-07-06 09:06:24]

yhirano@chromium.org changed reviewers:
+ horo@chromium.org

[yhirano, 2015-07-06 09:06:25]

+horo@ for modules/serviceworkers, modules/cachestorage and layout tests.

[hiroshige, 2015-07-07 03:50:58]

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:59: if (m_stream->stateInternal() ==
ReadableByteStream::Closed)
On 2015/07/06 06:14:00, yhirano wrote:
> On 2015/07/06 05:47:07, hiroshige wrote:
> > On 2015/07/06 03:25:38, yhirano wrote:
> > > [optional] It would be good to move this block to close() function.
> > Can we assume |m_stream|'s state changes to Closed/Errored are always via
> > ReadableStreamSource::close()/error()?
> > E.g.
> > m_stream->closeInternal() can be called from readInternalPostAction():
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > m_stream->error() can be called when ActiveDOMObject is stopped:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> You're right. Though currently all close / error can be done through the
> conroller (except the ActiveDOMObject::stop case which we don't need to behave
> consistently), we will not be sure when the v8 extra comes in.

I moved the blocks and added asserts here.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:61: if (m_stream->stateInternal() ==
ReadableByteStream::Errored)
On 2015/07/06 03:25:38, yhirano wrote:
> [optional] It would be good to move this block to error() function.

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/Body.cpp:87: void obtainReader()
On 2015/07/06 03:25:38, yhirano wrote:
> This function can be private.

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:8: #include
"core/dom/DOMArrayBuffer.h"
On 2015/07/06 03:25:39, yhirano wrote:
> -includes

Done.

https://codereview.chromium.org/1192913007/diff/730001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBufferTest.cpp:8: #include
"core/dom/DOMArrayBuffer.h"
On 2015/07/06 03:25:39, yhirano wrote:
> Please add TODO if you want to re-add tests later. Otherwise, please delete
this
> file.

Done.

https://codereview.chromium.org/1192913007/diff/750001/Source/modules/fetch/F...
File Source/modules/fetch/FetchRequestData.cpp (right):

https://codereview.chromium.org/1192913007/diff/750001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:45: // TODO(yhirano): This method will
be destructive.
On 2015/07/06 06:14:00, yhirano wrote:
> Please remove this comment.

Done.

https://codereview.chromium.org/1192913007/diff/750001/Source/modules/fetch/F...
Source/modules/fetch/FetchRequestData.cpp:70: // TODO(yhirano): This method will
be destructive.
Also removed this.

[yhirano, 2015-07-07 04:07:51]

lgtm

[horo, 2015-07-07 06:34:47]

lgtm with nit.

https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:85: BodyStreamBuffer* leakBuffer();
nit: I prefer releaseBuffer.

[hiroshige, 2015-07-07 06:50:53]

Thanks for reviewing!

https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
File Source/modules/fetch/BodyStreamBuffer.h (right):

https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
Source/modules/fetch/BodyStreamBuffer.h:85: BodyStreamBuffer* leakBuffer();
On 2015/07/07 06:34:46, horo wrote:
> nit: I prefer releaseBuffer.
I prefer leakBuffer() because this function omits the notification to
DrainingStreamNotificationClient (not simply moving the ownership of the
BodyStreamBuffer to another), should be used with care, and is used only in
Request/Response methods that creates the draining buffer.

[hiroshige, 2015-07-07 08:25:07]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-07-07 08:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/1192913007/#ps830001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-07-07 08:25:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192913007/830001

[horo, 2015-07-07 09:40:29]

On 2015/07/07 06:50:53, hiroshige wrote:
> Thanks for reviewing!
> 
>
https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
> File Source/modules/fetch/BodyStreamBuffer.h (right):
> 
>
https://codereview.chromium.org/1192913007/diff/810001/Source/modules/fetch/B...
> Source/modules/fetch/BodyStreamBuffer.h:85: BodyStreamBuffer* leakBuffer();
> On 2015/07/07 06:34:46, horo wrote:
> > nit: I prefer releaseBuffer.
> I prefer leakBuffer() because this function omits the notification to
> DrainingStreamNotificationClient (not simply moving the ownership of the
> BodyStreamBuffer to another), should be used with care, and is used only in
> Request/Response methods that creates the draining buffer.

I see.
LGTM.

[commit-bot: I haz the power, 2015-07-07 11:16:19]

Committed patchset #44 (id:830001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198391