[Fetch API] |(new Response(stream)).body| should return |stream|

[Fetch API] |(new Response(stream)).body| should return |stream|

This change is behind responseConstructedWithReadableStream and
responseBodyWithV8ExtraStream.

BUG=596832
Committed: https://crrev.com/0480aa344bc4fff82247d862d00ae872334200b2
Cr-Commit-Position: refs/heads/master@{#393832}

[yhirano, 2016-05-12 06:20:49]

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

[tyoshino (SeeGerritForStatus), 2016-05-12 07:23:25]

reviewed except for tests

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:113:
ScriptState::Scope scope(scriptState);
is it ok that isReadableStream() is not in the scope?

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Response.cpp (left):

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Response.cpp:142: bodyHandle =
ReadableStreamDataConsumerHandle::create(scriptState, bodyValue);
I don't remember what this line was for. Could you please teach me that?

[yhirano, 2016-05-12 08:13:12]

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:113:
ScriptState::Scope scope(scriptState);
On 2016/05/12 07:23:25, tyoshino wrote:
> is it ok that isReadableStream() is not in the scope?

Fixed.

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Response.cpp (left):

https://codereview.chromium.org/1969333002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Response.cpp:142: bodyHandle =
ReadableStreamDataConsumerHandle::create(scriptState, bodyValue);
On 2016/05/12 07:23:25, tyoshino wrote:
> I don't remember what this line was for. Could you please teach me that?

Maybe it's a bug introduced at https://codereview.chromium.org/1539803002.

[yhirano, 2016-05-12 08:14:31]

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

[yhirano, 2016-05-12 08:14:31]

+nhiroki for modules/cachestorage

[haraken, 2016-05-12 08:22:57]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-05-12 08:22:57]

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:113:
ASSERT(ReadableStreamOperations::isReadableStream(scriptState, stream));

Use DCHECK.

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:179: ScriptValue
reader = ReadableStreamOperations::getReader(m_scriptState.get(), stream(),
exceptionState);

Is it okay to ignore the exception?

[yhirano, 2016-05-12 08:28:15]

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:113:
ASSERT(ReadableStreamOperations::isReadableStream(scriptState, stream));
On 2016/05/12 08:22:57, haraken wrote:
> 
> Use DCHECK.

Done.

https://codereview.chromium.org/1969333002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:179: ScriptValue
reader = ReadableStreamOperations::getReader(m_scriptState.get(), stream(),
exceptionState);
On 2016/05/12 08:22:57, haraken wrote:
> 
> Is it okay to ignore the exception?

Yes. It throws an exception only when locked, and we have an assertion for that
at L173.

[nhiroki, 2016-05-12 08:56:23]

modules/cachestorage/CacheTest.cpp lgtm

[haraken, 2016-05-12 13:09:38]

V8 usage LGTM

[yhirano, 2016-05-13 07:00:45]

I noticed that the comment in BodyStreamBuffer::releaseHandle (in PS3, for
example) was wrong. So I updated the implementation. See the updated comment in
BodyStreamBuffer::releaseHandle.

haraken@, I made further v8-related changes in core/streams. Can you take a look
again?

Thanks,

[haraken, 2016-05-13 07:06:03]

LGTM

https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp:128:
DCHECK(!result.isEmpty());

Can you use V8ScriptRunner::callExtraOrCrash to check these?

[yhirano, 2016-05-13 07:21:16]

https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp:128:
DCHECK(!result.isEmpty());
On 2016/05/13 07:06:03, haraken wrote:
> 
> Can you use V8ScriptRunner::callExtraOrCrash to check these?

Done.
Does that mean we don't need |block|? Currently it exists only for DCHECK.

[haraken, 2016-05-13 07:24:12]

On 2016/05/13 07:21:16, yhirano wrote:
>
https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp
> (right):
> 
>
https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp:128:
> DCHECK(!result.isEmpty());
> On 2016/05/13 07:06:03, haraken wrote:
> > 
> > Can you use V8ScriptRunner::callExtraOrCrash to check these?
> 
> Done.
> Does that mean we don't need |block|? Currently it exists only for DCHECK.

Right, we won't need the TryCatch.

[yhirano, 2016-05-13 07:29:46]

On 2016/05/13 07:24:12, haraken wrote:
> On 2016/05/13 07:21:16, yhirano wrote:
> >
>
https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1969333002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/streams/ReadableStreamOperations.cpp:128:
> > DCHECK(!result.isEmpty());
> > On 2016/05/13 07:06:03, haraken wrote:
> > > 
> > > Can you use V8ScriptRunner::callExtraOrCrash to check these?
> > 
> > Done.
> > Does that mean we don't need |block|? Currently it exists only for DCHECK.
> 
> Right, we won't need the TryCatch.

Thanks, removed.

[tyoshino (SeeGerritForStatus), 2016-05-16 05:48:54]

lgtm

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:44: };
not used?

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:154:
EXPECT_FALSE(buffer->isStreamDisturbed());
Leave some TODO about this behavior?

[yhirano, 2016-05-16 06:35:37]

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp (right):

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:44: };
On 2016/05/16 05:48:53, tyoshino wrote:
> not used?

You're right. Thanks.

https://codereview.chromium.org/1969333002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBufferTest.cpp:154:
EXPECT_FALSE(buffer->isStreamDisturbed());
On 2016/05/16 05:48:53, tyoshino wrote:
> Leave some TODO about this behavior?

Done.

[yhirano, 2016-05-16 06:35:44]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-05-16 06:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, haraken@chromium.org, tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/1969333002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-05-16 06:36:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1969333002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1969333002/140001

[commit-bot: I haz the power, 2016-05-16 08:05:06]

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

[commit-bot: I haz the power, 2016-05-16 08:05:07]

Failed to apply patch for
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:144
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp' with conflicts.
  U third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp

Patch:       third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
Index: third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
diff --git a/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
b/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
index
9eec54a24245ef5dd7d7b8f15eb97104eff97f15..63b39114f4bbf0e2e0ef13c054f65d5133602930
100644
--- a/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
+++ b/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp
@@ -13,6 +13,8 @@
 #include "core/streams/ReadableStreamOperations.h"
 #include "modules/fetch/Body.h"
 #include "modules/fetch/DataConsumerHandleUtil.h"
+#include "modules/fetch/DataConsumerTee.h"
+#include "modules/fetch/ReadableStreamDataConsumerHandle.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/blob/BlobData.h"
 #include "platform/network/EncodedFormData.h"
@@ -84,6 +86,7 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState* scriptState,
PassOwnPtr<FetchDat
     , m_scriptState(scriptState)
     , m_handle(std::move(handle))
     , m_reader(m_handle->obtainReader(this))
+    , m_madeFromReadableStream(false)
 {
     if (RuntimeEnabledFeatures::responseBodyWithV8ExtraStreamEnabled()) {
         ScriptState::Scope scope(scriptState);
@@ -101,6 +104,22 @@ BodyStreamBuffer::BodyStreamBuffer(ScriptState*
scriptState, PassOwnPtr<FetchDat
     }
 }
 
+BodyStreamBuffer::BodyStreamBuffer(ScriptState* scriptState, ScriptValue
stream)
+    : UnderlyingSourceBase(scriptState)
+    , m_scriptState(scriptState)
+    , m_madeFromReadableStream(true)
+{
+    ScriptState::Scope scope(scriptState);
+    DCHECK(RuntimeEnabledFeatures::responseBodyWithV8ExtraStreamEnabled());
+    DCHECK(ReadableStreamOperations::isReadableStream(scriptState, stream));
+    v8::Local<v8::Value> bodyValue = toV8(this, scriptState);
+    DCHECK(!bodyValue.IsEmpty());
+    DCHECK(bodyValue->IsObject());
+    v8::Local<v8::Object> body = bodyValue.As<v8::Object>();
+
+    V8HiddenValue::setHiddenValue(scriptState, body,
V8HiddenValue::internalBodyStream(scriptState->isolate()), stream.v8Value());
+}
+
 ScriptValue BodyStreamBuffer::stream()
 {
     ScriptState::Scope scope(m_scriptState.get());
@@ -121,6 +140,9 @@ PassRefPtr<BlobDataHandle>
BodyStreamBuffer::drainAsBlobDataHandle(FetchDataCons
     if (isStreamClosed() || isStreamErrored())
         return nullptr;
 
+    if (m_madeFromReadableStream)
+        return nullptr;
+
     RefPtr<BlobDataHandle> blobDataHandle =
m_reader->drainAsBlobDataHandle(policy);
     if (blobDataHandle) {
         closeAndLockAndDisturb();
@@ -136,6 +158,9 @@ PassRefPtr<EncodedFormData>
BodyStreamBuffer::drainAsFormData()
     if (isStreamClosed() || isStreamErrored())
         return nullptr;
 
+    if (m_madeFromReadableStream)
+        return nullptr;
+
     RefPtr<EncodedFormData> formData = m_reader->drainAsFormData();
     if (formData) {
         closeAndLockAndDisturb();
@@ -144,29 +169,6 @@ PassRefPtr<EncodedFormData>
BodyStreamBuffer::drainAsFormData()
     return nullptr;
 }
 
-PassOwnPtr<FetchDataConsumerHandle> BodyStreamBuffer::releaseHandle()
-{
-    ASSERT(!isStreamLocked());
-    ASSERT(!isStreamDisturbed());
-    // We need to call these before calling closeAndLockAndDisturb.
-    const bool isClosed = isStreamClosed();
-    const bool isErrored = isStreamErrored();
-    OwnPtr<FetchDataConsumerHandle> handle = m_handle.release();
-
-    closeAndLockAndDisturb();
-
-    if (isClosed) {
-        // Note that the stream cannot be "draining", because it doesn't have
-        // the internal buffer.
-        return
createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle());
-    }
-    if (isErrored)
-        return
createFetchDataConsumerHandleFromWebHandle(createUnexpectedErrorDataConsumerHandle());
-
-    ASSERT(handle);
-    return handle.release();
-}
-
 void BodyStreamBuffer::startLoading(FetchDataLoader* loader,
FetchDataLoader::Client* client)
 {
     ASSERT(!m_loader);
@@ -176,6 +178,28 @@ void BodyStreamBuffer::startLoading(FetchDataLoader*
loader, FetchDataLoader::Cl
     loader->start(handle.get(), new
LoaderClient(m_scriptState->getExecutionContext(), this, client));
 }
 
+void BodyStreamBuffer::tee(BodyStreamBuffer** branch1, BodyStreamBuffer**
branch2)
+{
+    ASSERT(!isStreamLocked());
+    ASSERT(!isStreamDisturbed());
+    *branch1 = nullptr;
+    *branch2 = nullptr;
+
+    if (m_madeFromReadableStream) {
+        ScriptState::Scope scope(m_scriptState.get());
+        ScriptValue stream1, stream2;
+        ReadableStreamOperations::tee(m_scriptState.get(), stream(), &stream1,
&stream2);
+        *branch1 = new BodyStreamBuffer(m_scriptState.get(), stream1);
+        *branch2 = new BodyStreamBuffer(m_scriptState.get(), stream2);
+        return;
+    }
+    OwnPtr<FetchDataConsumerHandle> handle = releaseHandle();
+    OwnPtr<FetchDataConsumerHandle> handle1, handle2;
+    DataConsumerTee::create(m_scriptState->getExecutionContext(),
std::move(handle), &handle1, &handle2);
+    *branch1 = new BodyStreamBuffer(m_scriptState.get(), std::move(handle1));
+    *branch2 = new BodyStreamBuffer(m_scriptState.get(), std::move(handle2));
+}
+
 void BodyStreamBuffer::pullSource()
 {
     ASSERT(!m_streamNeedsMore);
@@ -384,4 +408,40 @@ void BodyStreamBuffer::stopLoading()
     m_loader = nullptr;
 }
 
+PassOwnPtr<FetchDataConsumerHandle> BodyStreamBuffer::releaseHandle()
+{
+    ASSERT(!isStreamLocked());
+    ASSERT(!isStreamDisturbed());
+
+    if (m_madeFromReadableStream) {
+        ScriptState::Scope scope(m_scriptState.get());
+        // We need to have |reader| alive by some means (as written in
+        // ReadableStreamDataConsumerHandle). Based on the following facts
+        //  - This function is used only from tee and startLoading.
+        //  - This branch cannot be taken when called from tee.
+        //  - startLoading makes hasPendingActivity return true while loading.
+        // , we don't need to keep the reader explicitly.
+        NonThrowableExceptionState exceptionState;
+        ScriptValue reader =
ReadableStreamOperations::getReader(m_scriptState.get(), stream(),
exceptionState);
+        return ReadableStreamDataConsumerHandle::create(m_scriptState.get(),
reader);
+    }
+    // We need to call these before calling closeAndLockAndDisturb.
+    const bool isClosed = isStreamClosed();
+    const bool isErrored = isStreamErrored();
+    OwnPtr<FetchDataConsumerHandle> handle = m_handle.release();
+
+    closeAndLockAndDisturb();
+
+    if (isClosed) {
+        // Note that the stream cannot be "draining", because it doesn't have
+        // the internal buffer.
+        return
createFetchDataConsumerHandleFromWebHandle(createDoneDataConsumerHandle());
+    }
+    if (isErrored)
+        return
createFetchDataConsumerHandleFromWebHandle(createUnexpectedErrorDataConsumerHandle());
+
+    ASSERT(handle);
+    return handle;
+}
+
 } // namespace blink

[commit-bot: I haz the power, 2016-05-16 08:05:10]

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

[yhirano, 2016-05-16 09:30:43]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-05-16 09:30:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tyoshino@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1969333002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-05-16 09:31:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1969333002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1969333002/160001

[commit-bot: I haz the power, 2016-05-16 13:19:57]

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

[commit-bot: I haz the power, 2016-05-16 13:19:58]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yhirano, 2016-05-16 13:24:01]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-05-16 13:24:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1969333002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1969333002/160001

[commit-bot: I haz the power, 2016-05-16 15:01:02]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-16 15:02:42]

Description was changed from

==========
[Fetch API] |(new Response(stream)).body| should return |stream|

This change is behind responseConstructedWithReadableStream and
responseBodyWithV8ExtraStream.

BUG=596832
==========

to

==========
[Fetch API] |(new Response(stream)).body| should return |stream|

This change is behind responseConstructedWithReadableStream and
responseBodyWithV8ExtraStream.

BUG=596832

Committed: https://crrev.com/0480aa344bc4fff82247d862d00ae872334200b2
Cr-Commit-Position: refs/heads/master@{#393832}
==========

[commit-bot: I haz the power, 2016-05-16 15:02:43]

Patchset 9 (id:??) landed as
https://crrev.com/0480aa344bc4fff82247d862d00ae872334200b2
Cr-Commit-Position: refs/heads/master@{#393832}