Make ReadableStreamOperations use ScriptValue

Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments (but not return values). To fit more nicely with the rest of Blink, we convert it to use ScriptValue everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org

Committed: https://crrev.com/f2d60be35e473d557ca6cd9f7d472804d1b788f7
Cr-Commit-Position: refs/heads/master@{#370627}

[haraken, 2016-01-16 03:56:02]

LGTM

[yhirano, 2016-01-19 05:56:41]

Do we need to pass ScriptState? ScriptValue contains it.

[domenic, 2016-01-19 20:42:02]

On 2016/01/19 at 05:56:41, yhirano wrote:
> Do we need to pass ScriptState? ScriptValue contains it.

I think we do, but I am not sure. I think it is important for cross-frame cases
where the stream comes from one frame but the caller comes from another. Those
cases are hard to reason about though.

[haraken, 2016-01-19 21:11:01]

On 2016/01/19 20:42:02, domenic wrote:
> On 2016/01/19 at 05:56:41, yhirano wrote:
> > Do we need to pass ScriptState? ScriptValue contains it.
> 
> I think we do, but I am not sure. I think it is important for cross-frame
cases
> where the stream comes from one frame but the caller comes from another. Those
> cases are hard to reason about though.

If the passed-in ScriptState is different from the ScriptState of the
ScriptValue, you're doing something (seriously) wrong :)

[domenic, 2016-01-19 21:27:13]

On 2016/01/19 at 21:11:01, haraken wrote:
> On 2016/01/19 20:42:02, domenic wrote:
> > On 2016/01/19 at 05:56:41, yhirano wrote:
> > > Do we need to pass ScriptState? ScriptValue contains it.
> > 
> > I think we do, but I am not sure. I think it is important for cross-frame
cases
> > where the stream comes from one frame but the caller comes from another.
Those
> > cases are hard to reason about though.
> 
> If the passed-in ScriptState is different from the ScriptState of the
ScriptValue, you're doing something (seriously) wrong :)

Fair. And I guess it should not matter much anyway since the thing we use the
scriptState for is just calling V8 extras functions which should be the same
across all contexts.

[haraken, 2016-01-19 21:41:00]

On 2016/01/19 21:27:13, domenic wrote:
> On 2016/01/19 at 21:11:01, haraken wrote:
> > On 2016/01/19 20:42:02, domenic wrote:
> > > On 2016/01/19 at 05:56:41, yhirano wrote:
> > > > Do we need to pass ScriptState? ScriptValue contains it.
> > > 
> > > I think we do, but I am not sure. I think it is important for cross-frame
> cases
> > > where the stream comes from one frame but the caller comes from another.
> Those
> > > cases are hard to reason about though.
> > 
> > If the passed-in ScriptState is different from the ScriptState of the
> ScriptValue, you're doing something (seriously) wrong :)
> 
> Fair. And I guess it should not matter much anyway since the thing we use the
> scriptState for is just calling V8 extras functions which should be the same
> across all contexts.

It matters though. ScriptState : v8::Context = 1 : 1. When you call V8 extra's
function, you need to specify a correct ScriptState.

Either way, if the passed-in ScriptState is different from the ScriptState of
the ScriptValue, it means that you're leaking objects among contexts, which is
(already) a bug.

[domenic, 2016-01-19 22:02:14]

On 2016/01/19 at 21:41:00, haraken wrote:

> It matters though. ScriptState : v8::Context = 1 : 1. When you call V8 extra's
function, you need to specify a correct ScriptState.
> 
> Either way, if the passed-in ScriptState is different from the ScriptState of
the ScriptValue, it means that you're leaking objects among contexts, which is
(already) a bug.

I don't really understand why this would be a bug. It happens all the time in
web code that uses multiple iframes.

[haraken, 2016-01-19 22:09:30]

On 2016/01/19 22:02:14, domenic wrote:
> On 2016/01/19 at 21:41:00, haraken wrote:
> 
> > It matters though. ScriptState : v8::Context = 1 : 1. When you call V8
extra's
> function, you need to specify a correct ScriptState.
> > 
> > Either way, if the passed-in ScriptState is different from the ScriptState
of
> the ScriptValue, it means that you're leaking objects among contexts, which is
> (already) a bug.
> 
> I don't really understand why this would be a bug. It happens all the time in
> web code that uses multiple iframes.

Even if JS uses multiple iframes, each wrapper must be created on a correct
context, even listener's callback must be called with a correct context etc. For
example, if you run iframe.contentDocument in the main frame, the document
wrapper must be created on the main frame's context, not on the iframe's
context. In V8 bindings, you need to make sure that you're always on a correct
context.

[domenic, 2016-01-19 22:41:11]

On 2016/01/19 at 22:09:30, haraken wrote:
> On 2016/01/19 22:02:14, domenic wrote:
> > On 2016/01/19 at 21:41:00, haraken wrote:
> > 
> > > It matters though. ScriptState : v8::Context = 1 : 1. When you call V8
extra's
> > function, you need to specify a correct ScriptState.
> > > 
> > > Either way, if the passed-in ScriptState is different from the ScriptState
of
> > the ScriptValue, it means that you're leaking objects among contexts, which
is
> > (already) a bug.
> > 
> > I don't really understand why this would be a bug. It happens all the time
in
> > web code that uses multiple iframes.
> 
> Even if JS uses multiple iframes, each wrapper must be created on a correct
context, even listener's callback must be called with a correct context etc. For
example, if you run iframe.contentDocument in the main frame, the document
wrapper must be created on the main frame's context, not on the iframe's
context. In V8 bindings, you need to make sure that you're always on a correct
context.

OK, I think I understand. I am not sure what the conclusion is though. Should we
keep the ScriptState* parameter and do an ASSERT that it is the same as
value.scriptState()? Or should we just use value.scriptState().

[domenic, 2016-01-19 22:45:39]

Description was changed from

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments and return values. To
fit more nicely with the rest of Blink, we convert it to use ScriptValue
everywhere.

BUG=503491
R=yhirano@chromium.org
==========

to

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments and return values. To
fit more nicely with the rest of Blink, we convert it to use ScriptValue
everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org
==========

[domenic, 2016-01-19 22:45:39]

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

[haraken, 2016-01-19 23:01:16]

On 2016/01/19 22:41:11, domenic wrote:
> On 2016/01/19 at 22:09:30, haraken wrote:
> > On 2016/01/19 22:02:14, domenic wrote:
> > > On 2016/01/19 at 21:41:00, haraken wrote:
> > > 
> > > > It matters though. ScriptState : v8::Context = 1 : 1. When you call V8
> extra's
> > > function, you need to specify a correct ScriptState.
> > > > 
> > > > Either way, if the passed-in ScriptState is different from the
ScriptState
> of
> > > the ScriptValue, it means that you're leaking objects among contexts,
which
> is
> > > (already) a bug.
> > > 
> > > I don't really understand why this would be a bug. It happens all the time
> in
> > > web code that uses multiple iframes.
> > 
> > Even if JS uses multiple iframes, each wrapper must be created on a correct
> context, even listener's callback must be called with a correct context etc.
For
> example, if you run iframe.contentDocument in the main frame, the document
> wrapper must be created on the main frame's context, not on the iframe's
> context. In V8 bindings, you need to make sure that you're always on a correct
> context.
> 
> OK, I think I understand. I am not sure what the conclusion is though. Should
we
> keep the ScriptState* parameter and do an ASSERT that it is the same as
> value.scriptState()? Or should we just use value.scriptState().

Looking at the CL more carefully, I'm getting confused.

What is the advantage of replacing the Local<Value> with ScriptValue? You're
converting Local<Value> to ScriptValue but then immediately converting the
ScriptValue back to Local<Value>.

Also passing ScriptState to the extra's methods wouldn't make sense because the
ScriptState should always be ScriptState::current(). So you can just call
ScriptState::current() in the extra's methods. (If you use a ScriptState
different from ScriptState::current(), it's a bug because you're using the
Local<Value> (which comes from the ScriptState::current) at the same time.)

[domenic, 2016-01-19 23:11:01]

On 2016/01/19 at 23:01:16, haraken wrote:
> On 2016/01/19 22:41:11, domenic wrote:
> > On 2016/01/19 at 22:09:30, haraken wrote:
> > > On 2016/01/19 22:02:14, domenic wrote:
> > > > On 2016/01/19 at 21:41:00, haraken wrote:
> > > > 
> > > > > It matters though. ScriptState : v8::Context = 1 : 1. When you call V8
> > extra's
> > > > function, you need to specify a correct ScriptState.
> > > > > 
> > > > > Either way, if the passed-in ScriptState is different from the
ScriptState
> > of
> > > > the ScriptValue, it means that you're leaking objects among contexts,
which
> > is
> > > > (already) a bug.
> > > > 
> > > > I don't really understand why this would be a bug. It happens all the
time
> > in
> > > > web code that uses multiple iframes.
> > > 
> > > Even if JS uses multiple iframes, each wrapper must be created on a
correct
> > context, even listener's callback must be called with a correct context etc.
For
> > example, if you run iframe.contentDocument in the main frame, the document
> > wrapper must be created on the main frame's context, not on the iframe's
> > context. In V8 bindings, you need to make sure that you're always on a
correct
> > context.
> > 
> > OK, I think I understand. I am not sure what the conclusion is though.
Should we
> > keep the ScriptState* parameter and do an ASSERT that it is the same as
> > value.scriptState()? Or should we just use value.scriptState().
> 
> Looking at the CL more carefully, I'm getting confused.
> 
> What is the advantage of replacing the Local<Value> with ScriptValue? You're
converting Local<Value> to ScriptValue but then immediately converting the
ScriptValue back to Local<Value>.

My impression was that in general Blink tries to expose Local<Value>s to as
little Blink code as possible, instead exposing ScriptValues. Right now
ReadableStreamOperations uses a mix of both (e.g. getReader returns a
ScriptValue, but read accepts a Local<Value>). So I thought we should
standardize on ScriptValue as the type that represents a stream or reader.

> Also passing ScriptState to the extra's methods wouldn't make sense because
the ScriptState should always be ScriptState::current(). So you can just call
ScriptState::current() in the extra's methods. (If you use a ScriptState
different from ScriptState::current(), it's a bug because you're using the
Local<Value> (which comes from the ScriptState::current) at the same time.)

Maybe https://codereview.chromium.org/1594653003 should change the extras
methods to always use ScriptState::current() then?

[haraken, 2016-01-19 23:26:24]

On 2016/01/19 23:11:01, domenic wrote:
> On 2016/01/19 at 23:01:16, haraken wrote:
> > On 2016/01/19 22:41:11, domenic wrote:
> > > On 2016/01/19 at 22:09:30, haraken wrote:
> > > > On 2016/01/19 22:02:14, domenic wrote:
> > > > > On 2016/01/19 at 21:41:00, haraken wrote:
> > > > > 
> > > > > > It matters though. ScriptState : v8::Context = 1 : 1. When you call
V8
> > > extra's
> > > > > function, you need to specify a correct ScriptState.
> > > > > > 
> > > > > > Either way, if the passed-in ScriptState is different from the
> ScriptState
> > > of
> > > > > the ScriptValue, it means that you're leaking objects among contexts,
> which
> > > is
> > > > > (already) a bug.
> > > > > 
> > > > > I don't really understand why this would be a bug. It happens all the
> time
> > > in
> > > > > web code that uses multiple iframes.
> > > > 
> > > > Even if JS uses multiple iframes, each wrapper must be created on a
> correct
> > > context, even listener's callback must be called with a correct context
etc.
> For
> > > example, if you run iframe.contentDocument in the main frame, the document
> > > wrapper must be created on the main frame's context, not on the iframe's
> > > context. In V8 bindings, you need to make sure that you're always on a
> correct
> > > context.
> > > 
> > > OK, I think I understand. I am not sure what the conclusion is though.
> Should we
> > > keep the ScriptState* parameter and do an ASSERT that it is the same as
> > > value.scriptState()? Or should we just use value.scriptState().
> > 
> > Looking at the CL more carefully, I'm getting confused.
> > 
> > What is the advantage of replacing the Local<Value> with ScriptValue? You're
> converting Local<Value> to ScriptValue but then immediately converting the
> ScriptValue back to Local<Value>.
> 
> My impression was that in general Blink tries to expose Local<Value>s to as
> little Blink code as possible, instead exposing ScriptValues. Right now
> ReadableStreamOperations uses a mix of both (e.g. getReader returns a
> ScriptValue, but read accepts a Local<Value>). So I thought we should
> standardize on ScriptValue as the type that represents a stream or reader.

The rule is:

- bindings/ can use V8 APIs (i.e., v8::) directly. They are error-prone and must
be reviewed by bindings/'s OWNERs.

- core/ and modules/ are not allowed to use V8 APIs. If they need to interact
with V8, they must use abstraction types such as ScriptValue, ScriptState,
DOMWrapperWorld, ExceptionState etc defined in bindings/.

In case of V8 extras, there are two options:

a) Put the code in core/ or modules/. Don't use V8 APIs.

b) Put the code in bindings/. Use V8 APIs.

Given that V8 extras need to interact with V8 heavily, b) would make more sense.

> 
> > Also passing ScriptState to the extra's methods wouldn't make sense because
> the ScriptState should always be ScriptState::current(). So you can just call
> ScriptState::current() in the extra's methods. (If you use a ScriptState
> different from ScriptState::current(), it's a bug because you're using the
> Local<Value> (which comes from the ScriptState::current) at the same time.)
> 
> Maybe https://codereview.chromium.org/1594653003 should change the extras
> methods to always use ScriptState::current() then?

Let's make the changes one by one. Land
https://codereview.chromium.org/1594653003 first, and then switch the extra's
methods to use ScriptState::current().

[domenic, 2016-01-19 23:46:47]

On 2016/01/19 at 23:26:24, haraken wrote:
> On 2016/01/19 23:11:01, domenic wrote:
> > On 2016/01/19 at 23:01:16, haraken wrote:
> > > Looking at the CL more carefully, I'm getting confused.
> > > 
> > > What is the advantage of replacing the Local<Value> with ScriptValue?
You're
> > converting Local<Value> to ScriptValue but then immediately converting the
> > ScriptValue back to Local<Value>.
> > 
> > My impression was that in general Blink tries to expose Local<Value>s to as
> > little Blink code as possible, instead exposing ScriptValues. Right now
> > ReadableStreamOperations uses a mix of both (e.g. getReader returns a
> > ScriptValue, but read accepts a Local<Value>). So I thought we should
> > standardize on ScriptValue as the type that represents a stream or reader.
> 
> The rule is:
> 
> - bindings/ can use V8 APIs (i.e., v8::) directly. They are error-prone and
must be reviewed by bindings/'s OWNERs.
> 
> - core/ and modules/ are not allowed to use V8 APIs. If they need to interact
with V8, they must use abstraction types such as ScriptValue, ScriptState,
DOMWrapperWorld, ExceptionState etc defined in bindings/.
> 
> In case of V8 extras, there are two options:
> 
> a) Put the code in core/ or modules/. Don't use V8 APIs.
> 
> b) Put the code in bindings/. Use V8 APIs.
> 
> Given that V8 extras need to interact with V8 heavily, b) would make more
sense.

But ReadableStreamOperations is meant to be used by code outside bindings/, like
modules/fetch/Response.cpp and
modules/fetch/ReadableStreamDataConsumerHandle.cpp. So its parameters and return
values should be ScriptValues, I think.

[haraken, 2016-01-19 23:57:08]

On 2016/01/19 23:46:47, domenic wrote:
> On 2016/01/19 at 23:26:24, haraken wrote:
> > On 2016/01/19 23:11:01, domenic wrote:
> > > On 2016/01/19 at 23:01:16, haraken wrote:
> > > > Looking at the CL more carefully, I'm getting confused.
> > > > 
> > > > What is the advantage of replacing the Local<Value> with ScriptValue?
> You're
> > > converting Local<Value> to ScriptValue but then immediately converting the
> > > ScriptValue back to Local<Value>.
> > > 
> > > My impression was that in general Blink tries to expose Local<Value>s to
as
> > > little Blink code as possible, instead exposing ScriptValues. Right now
> > > ReadableStreamOperations uses a mix of both (e.g. getReader returns a
> > > ScriptValue, but read accepts a Local<Value>). So I thought we should
> > > standardize on ScriptValue as the type that represents a stream or reader.
> > 
> > The rule is:
> > 
> > - bindings/ can use V8 APIs (i.e., v8::) directly. They are error-prone and
> must be reviewed by bindings/'s OWNERs.
> > 
> > - core/ and modules/ are not allowed to use V8 APIs. If they need to
interact
> with V8, they must use abstraction types such as ScriptValue, ScriptState,
> DOMWrapperWorld, ExceptionState etc defined in bindings/.
> > 
> > In case of V8 extras, there are two options:
> > 
> > a) Put the code in core/ or modules/. Don't use V8 APIs.
> > 
> > b) Put the code in bindings/. Use V8 APIs.
> > 
> > Given that V8 extras need to interact with V8 heavily, b) would make more
> sense.
> 
> But ReadableStreamOperations is meant to be used by code outside bindings/,
like
> modules/fetch/Response.cpp and
> modules/fetch/ReadableStreamDataConsumerHandle.cpp. So its parameters and
return
> values should be ScriptValues, I think.

Thanks, now I'm getting to understand.

Then a right fix would be:

- keep Response and ReadableStreamDataConsumerHandle in modules/fetch/
- pass ScriptValue to ReadableStreamDataConsumerHandle
- remove ScriptState from ReadableStreamDataConsumerHandle and use
ScriptState::current instead

?

[domenic, 2016-01-20 00:25:17]

On 2016/01/19 at 23:57:08, haraken wrote:
> Thanks, now I'm getting to understand.
> 
> Then a right fix would be:
> 
> - keep Response and ReadableStreamDataConsumerHandle in modules/fetch/
> - pass ScriptValue to ReadableStreamDataConsumerHandle
> - remove ScriptState from ReadableStreamDataConsumerHandle and use
ScriptState::current instead
> 
> ?

I can do that. So ReadableStreamOperations methods take both ScriptState and
ScriptValue (and do an assert that they are the same?), but we update the call
sites to use ScriptState::current?

Preferably I can leave the call site updating for yhirano@. I mostly just need
to work on ReadableStreamOperations and am not too familiar with
ReadableStreamDataConsumerHandle.

[haraken, 2016-01-20 00:31:55]

On 2016/01/20 00:25:17, domenic wrote:
> On 2016/01/19 at 23:57:08, haraken wrote:
> > Thanks, now I'm getting to understand.
> > 
> > Then a right fix would be:
> > 
> > - keep Response and ReadableStreamDataConsumerHandle in modules/fetch/
> > - pass ScriptValue to ReadableStreamDataConsumerHandle
> > - remove ScriptState from ReadableStreamDataConsumerHandle and use
> ScriptState::current instead
> > 
> > ?
> 
> I can do that. So ReadableStreamOperations methods take both ScriptState and
> ScriptValue (and do an assert that they are the same?), but we update the call
> sites to use ScriptState::current?

It wouldn't be too helpful to pass ScriptState just for validating the
ScriptValue. ScriptValue.v8Value() already has an assert about the world where
the ScriptValue is used.

So I think you can just pass in only ScriptValue. And you can use
ScriptState::current.

> 
> Preferably I can leave the call site updating for yhirano@. I mostly just need
> to work on ReadableStreamOperations and am not too familiar with
> ReadableStreamDataConsumerHandle.

[yhirano, 2016-01-20 04:02:02]

> remove ScriptState from ReadableStreamDataConsumerHandle and use
ScriptState::current instead
I don't understand why this is needed: We need to create a ScriptState::Scope in
ReadableStreamDataConsumerHandle. At least in that case we cannot use
ScriptState::current.

[haraken, 2016-01-20 04:08:10]

On 2016/01/20 04:02:02, yhirano wrote:
> > remove ScriptState from ReadableStreamDataConsumerHandle and use
> ScriptState::current instead
> I don't understand why this is needed: We need to create a ScriptState::Scope
in
> ReadableStreamDataConsumerHandle. At least in that case we cannot use
> ScriptState::current.

Just to clarify: I'm not saying that we should always use
ScriptState::current(). (We should always use a *correct* ScriptState.)

What I'm saying in the above is just that we should replace this pattern:

  void foo(ScriptState* state, ScriptValue value) {
    value.v8Value();
    bar(state, ...);
  }

with:

  void foo(ScriptValue value) {
    value.v8Value();
    bar(ScriptState::current(), ...);
  }

The former pattern doesn't make sense because:

- The fact that you're using value.v8Value() means that the value is a V8 object
of the current context.
- Then the ScriptState should be the current ScriptState. Thus you can just use
ScriptState::current().

[yhirano, 2016-01-20 06:14:38]

On 2016/01/20 04:08:10, haraken wrote:
> On 2016/01/20 04:02:02, yhirano wrote:
> > > remove ScriptState from ReadableStreamDataConsumerHandle and use
> > ScriptState::current instead
> > I don't understand why this is needed: We need to create a
ScriptState::Scope
> in
> > ReadableStreamDataConsumerHandle. At least in that case we cannot use
> > ScriptState::current.
> 
> Just to clarify: I'm not saying that we should always use
> ScriptState::current(). (We should always use a *correct* ScriptState.)
> 
> What I'm saying in the above is just that we should replace this pattern:
> 
>   void foo(ScriptState* state, ScriptValue value) {
>     value.v8Value();
>     bar(state, ...);
>   }
> 
> with:
> 
>   void foo(ScriptValue value) {
>     value.v8Value();
>     bar(ScriptState::current(), ...);
>   }
> 
> The former pattern doesn't make sense because:
> 
> - The fact that you're using value.v8Value() means that the value is a V8
object
> of the current context.
> - Then the ScriptState should be the current ScriptState. Thus you can just
use
> ScriptState::current().

I have two questions:

1. How about the following code? Does it have pros / cons to other patterns?

  void foo(ScriptValue value) {
    ASSERT(!value.isEmpty());
    value.v8Value();
    bar(value.scriptState(), ...);
  }

2. If ScriptState::current() is preferable, should we use it instead of
[CallWith=ScriptState]?

[yhirano, 2016-01-20 06:46:37]

I didn't notice that ScriptState::current takes v8::Isolate*.

Hence the answer for
> 2. If ScriptState::current() is preferable, should we use it instead of
> [CallWith=ScriptState]?
should be no, because we don't want to use v8::Isolate::GetCurrent().

Also, 

>   void foo(ScriptValue value) {
>     value.v8Value();
>     bar(ScriptState::current(), ...);
>   }

should be

>   void foo(ScriptValue value) {
>     value.v8Value();
>     bar(ScriptState::current(isolate), ...);
>   }

. Where is the isolate from?

[yhirano, 2016-01-20 06:47:28]

https://codereview.chromium.org/1595713003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h
(right):

https://codereview.chromium.org/1595713003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:20: //
arguments must not be empty.
All ScriptValue arguments must not be empty.

[haraken, 2016-01-20 09:49:29]

On 2016/01/20 06:46:37, yhirano wrote:
> I didn't notice that ScriptState::current takes v8::Isolate*.
> 
> Hence the answer for
> > 2. If ScriptState::current() is preferable, should we use it instead of
> > [CallWith=ScriptState]?
> should be no, because we don't want to use v8::Isolate::GetCurrent().
> 
> Also, 
> 
> >   void foo(ScriptValue value) {
> >     value.v8Value();
> >     bar(ScriptState::current(), ...);
> >   }
> 
> should be
> 
> >   void foo(ScriptValue value) {
> >     value.v8Value();
> >     bar(ScriptState::current(isolate), ...);
> >   }
> 
> . Where is the isolate from?

Ah, this is a good point...

So, to sum up the discussion in this thread, I think we have two options:

a) Remove ScriptState. Don't use ScriptState::current(). Use
value.scriptState().

b) Pass in ScriptState. Just use the passed-in ScriptState.

b) is what the PS1 is doing. a) is what yhirano@ proposed before.

This boils down to an uninteresting question: where should we retrieve the
ScriptState? Given that there are many ways to retrieve the ScriptState, there
could be many answers. Both a) and b) will work.

I'm sorry for getting you back and forth in the thread :/ My conclusion is that
I'm fine with either a) or b).

[yhirano, 2016-01-20 14:50:48]

> Preferably I can leave the call site updating for yhirano@. I mostly just need
> to work on ReadableStreamOperations and am not too familiar with
> ReadableStreamDataConsumerHandle.

Either is fine. I can write a separate CL updating
ReadableStreamDataConsumerHandle and Response.

[haraken, 2016-01-20 18:11:26]

On 2016/01/20 14:50:48, yhirano wrote:
> > Preferably I can leave the call site updating for yhirano@. I mostly just
need
> > to work on ReadableStreamOperations and am not too familiar with
> > ReadableStreamDataConsumerHandle.
> 
> Either is fine. I can write a separate CL updating
> ReadableStreamDataConsumerHandle and Response.

So the PS1 LGTM. Sorry about the noise in this thread :/

[domenic, 2016-01-20 23:46:43]

On 2016/01/20 at 18:11:26, haraken wrote:
> On 2016/01/20 14:50:48, yhirano wrote:
> > > Preferably I can leave the call site updating for yhirano@. I mostly just
need
> > > to work on ReadableStreamOperations and am not too familiar with
> > > ReadableStreamDataConsumerHandle.
> > 
> > Either is fine. I can write a separate CL updating
> > ReadableStreamDataConsumerHandle and Response.
> 
> So the PS1 LGTM. Sorry about the noise in this thread :/

OK, no problem, I learned things and I think we all came to a better
understanding. I think I will just stick with the original idea for now as you
said.

I've rebased on it master and re-started the try jobs; it looks like the
failures last time were probably not my fault but we'll see. Still hoping for
yhirano@'s LGTM :)

[yhirano, 2016-01-20 23:56:53]

On 2016/01/20 23:46:43, domenic wrote:
> On 2016/01/20 at 18:11:26, haraken wrote:
> > On 2016/01/20 14:50:48, yhirano wrote:
> > > > Preferably I can leave the call site updating for yhirano@. I mostly
just
> need
> > > > to work on ReadableStreamOperations and am not too familiar with
> > > > ReadableStreamDataConsumerHandle.
> > > 
> > > Either is fine. I can write a separate CL updating
> > > ReadableStreamDataConsumerHandle and Response.
> > 
> > So the PS1 LGTM. Sorry about the noise in this thread :/
> 
> OK, no problem, I learned things and I think we all came to a better
> understanding. I think I will just stick with the original idea for now as you
> said.
> 
> I've rebased on it master and re-started the try jobs; it looks like the
> failures last time were probably not my fault but we'll see. Still hoping for
> yhirano@'s LGTM :)

Please see https://codereview.chromium.org/1595713003/#msg24.

[domenic, 2016-01-21 00:06:42]

Description was changed from

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments and return values. To
fit more nicely with the rest of Blink, we convert it to use ScriptValue
everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org
==========

to

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments (but not return
values). To fit more nicely with the rest of Blink, we convert it to use
ScriptValue everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org
==========

[domenic, 2016-01-21 00:15:29]

On 2016/01/20 at 23:56:53, yhirano wrote:

> Please see https://codereview.chromium.org/1595713003/#msg24.

Thanks, fixed and added a couple asserts.

[yhirano, 2016-01-21 00:17:10]

lgtm

[domenic, 2016-01-21 03:59:36]

The CQ bit was checked by domenic@chromium.org

[domenic, 2016-01-21 03:59:37]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1595713003/#ps40001
(title: "Fix comment and add a couple asserts")

[commit-bot: I haz the power, 2016-01-21 03:59:57]

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

[commit-bot: I haz the power, 2016-01-21 05:06:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 05:07:44]

Description was changed from

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments (but not return
values). To fit more nicely with the rest of Blink, we convert it to use
ScriptValue everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org
==========

to

==========
Make ReadableStreamOperations use ScriptValue

Previously it used v8::Local<v8::Value> for many arguments (but not return
values). To fit more nicely with the rest of Blink, we convert it to use
ScriptValue everywhere.

BUG=503491
R=yhirano@chromium.org,haraken@chromium.org

Committed: https://crrev.com/f2d60be35e473d557ca6cd9f7d472804d1b788f7
Cr-Commit-Position: refs/heads/master@{#370627}
==========

[commit-bot: I haz the power, 2016-01-21 05:07:45]

Patchset 3 (id:??) landed as
https://crrev.com/f2d60be35e473d557ca6cd9f7d472804d1b788f7
Cr-Commit-Position: refs/heads/master@{#370627}