Add a utility class to call stream methods implemented with v8 extras.

Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491
Committed: https://crrev.com/e1cf4a50c55b6ebac10c9c6d9d508e4d794eb4f5
Cr-Commit-Position: refs/heads/master@{#363478}

[yhirano, 2015-12-02 06:31:44]

Description was changed from

==========
Add a utility class to call stream methods implemented on v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented on V8 extras.

BUG=503491
==========

to

==========
Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491
==========

[yhirano, 2015-12-02 07:16:59]

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

[yhirano, 2015-12-02 07:16:59]

This CL depends on [1].
I borrowed the ScriptState change from Domenic's CL[2].

I'm planning to use this class from modules/fetch to implement Response
construction with a stream.

1: https://codereview.chromium.org/1491083002/
2: https://codereview.chromium.org/1167343002/

[haraken, 2015-12-02 07:37:52]

On 2015/12/02 07:16:59, yhirano wrote:
> This CL depends on [1].
> I borrowed the ScriptState change from Domenic's CL[2].
> 
> I'm planning to use this class from modules/fetch to implement Response
> construction with a stream.
> 
> 1: https://codereview.chromium.org/1491083002/
> 2: https://codereview.chromium.org/1167343002/

The change to ScriptState looks good.

[domenic, 2015-12-02 17:53:03]

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:25:
template <size_t N>
Do you think this might be useful enough that it should go in ScriptState?

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:20: class
CORE_EXPORT ReadableStreamOperations {
Why a class full of public methods instead of a namespace full of functions?

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:25: //
Returns an empty value and throws an error (in the V8 context) when
Should this be handled using an ExceptionState& instead?

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:274:
ReadableStreamOperations::read(scriptState(), reader).then(
This is workable but kind of takes a lot of supporting code. I am OK with it if
you are but would it be possible to create the wrapper so that you could write

ReadableStreamOperations::read<String>(scriptState(), reader,
  [](option<String> value) {
    EXPECT_EQ("hello", *value);
  },
  [](auto err) {
    EXPECT_TRUE(false) << "'Unreachable' code was reached";
  });

?

Feel free to ignore though, this might just be my JavaScript brain saying things
that make less sense in C++ :)

[yhirano, 2015-12-03 11:18:29]

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:25:
template <size_t N>
On 2015/12/02 17:53:03, domenic wrote:
> Do you think this might be useful enough that it should go in ScriptState?

For now, I think having this utility function here is reasonable. We can move
this to a shared place when other people also want it.

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:20: class
CORE_EXPORT ReadableStreamOperations {
On 2015/12/02 17:53:03, domenic wrote:
> Why a class full of public methods instead of a namespace full of functions?

I'm hearing opinions on blink-dev.

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.h:25: //
Returns an empty value and throws an error (in the V8 context) when
On 2015/12/02 17:53:03, domenic wrote:
> Should this be handled using an ExceptionState& instead?

Done.

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
(right):

https://codereview.chromium.org/1492763002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:274:
ReadableStreamOperations::read(scriptState(), reader).then(
On 2015/12/02 17:53:03, domenic wrote:
> This is workable but kind of takes a lot of supporting code. I am OK with it
if
> you are but would it be possible to create the wrapper so that you could write
> 
> ReadableStreamOperations::read<String>(scriptState(), reader,
>   [](option<String> value) {
>     EXPECT_EQ("hello", *value);
>   },
>   [](auto err) {
>     EXPECT_TRUE(false) << "'Unreachable' code was reached";
>   });
> 
> ?
> 
> Feel free to ignore though, this might just be my JavaScript brain saying
things
> that make less sense in C++ :)

Unfortunately, we cannot use lambda for the purpose. See "Lambda Expressions" in
https://chromium-cpp.appspot.com/. I added a utility function to unpack an
iterator to make it less painful.

[domenic, 2015-12-03 17:51:37]

LGTM. Since blink-dev doesn't seem to have consensus on namespace vs.
STATIC_ONLY class I don't care either way.

As for how usable read() ends up being, I guess we will find out when you start
using it in real code. Maybe there will be better patterns then, but for now
this is fine.

[haraken, 2015-12-04 01:41:05]

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:53: bool
ok = call(scriptState, "IsReadableStream", args).ToLocal(&resultValue);

Can we use v8CallOrBoolean? The same comment for other ToLocals. Basically we
prefer using v8CallXXX macros than ToLocal.

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8IteratorResultValue.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8IteratorResultValue.cpp:21:
v8::MaybeLocal<v8::Value> v8IteratorUnpack(ScriptState* scriptState,
v8::Local<v8::Value> iterator, bool* done)

I don't fully understand why this method is needed. If this is only for testing,
can we move this into the test?

[yhirano, 2015-12-04 05:50:58]

Patchset #10 (id:180001) has been deleted

[yhirano, 2015-12-04 05:51:02]

Patchset #9 (id:160001) has been deleted

[yhirano, 2015-12-04 05:51:09]

Patchset #9 (id:200001) has been deleted

[yhirano, 2015-12-04 05:55:46]

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

[yhirano, 2015-12-04 05:55:47]

+bashi@chromium.org

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:53: bool
ok = call(scriptState, "IsReadableStream", args).ToLocal(&resultValue);
On 2015/12/04 01:41:05, haraken wrote:
> 
> Can we use v8CallOrBoolean? The same comment for other ToLocals. Basically we
> prefer using v8CallXXX macros than ToLocal.
> 

Fixed.

I talked with bashi@ and added v8Call(MaybeLocal<T>, Local<T>&) for just calling
ToLocal, because passing around a TryCatch block just for assertion is tiresome.

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8IteratorResultValue.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8IteratorResultValue.cpp:21:
v8::MaybeLocal<v8::Value> v8IteratorUnpack(ScriptState* scriptState,
v8::Local<v8::Value> iterator, bool* done)
On 2015/12/04 01:41:05, haraken wrote:
> 
> I don't fully understand why this method is needed. If this is only for
testing,
> can we move this into the test?
> 

I plan to use it in the future, but moving it back to the test file is OK.
Fixed.

[haraken, 2015-12-04 05:58:51]

LGTM

[bashi, 2015-12-04 06:48:54]

LGTM for maybe API use.

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperations.cpp:53: bool
ok = call(scriptState, "IsReadableStream", args).ToLocal(&resultValue);
On 2015/12/04 05:55:47, yhirano wrote:
> On 2015/12/04 01:41:05, haraken wrote:
> > 
> > Can we use v8CallOrBoolean? The same comment for other ToLocals. Basically
we
> > prefer using v8CallXXX macros than ToLocal.
> > 
> 
> Fixed.
> 
> I talked with bashi@ and added v8Call(MaybeLocal<T>, Local<T>&) for just
calling
> ToLocal, because passing around a TryCatch block just for assertion is
tiresome.

In this specific case, we may just want to do:

resultValue = v8CallOrCrash(call(...));

because we just ASSERT() here and provide no additional information.

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:128:
v8::MaybeLocal<v8::String> source = v8::String::NewFromUtf8(isolate(), s,
v8::String::kNormalString);
nit: It would be nice to combine empty check and value extraction:

v8::Local<v8::String> source;
if (!v8::String::NewFromUtf8(...).ToLocal(&source)) {
  ADD_FAILURE();
  return v8::MaybeLocal<v8::Value>();
}
v8::Local<v8::Script> script;
if (!v8::Script::Compile(scriptState()->context(), source).ToLocal(&script)) {
  ADD_FAILURE();
  return v8::MaybeLocal<v8::Value>();
}
...

[yhirano, 2015-12-04 06:51:21]

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
(right):

https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:128:
v8::MaybeLocal<v8::String> source = v8::String::NewFromUtf8(isolate(), s,
v8::String::kNormalString);
On 2015/12/04 06:48:53, bashi1 wrote:
> nit: It would be nice to combine empty check and value extraction:
> 
> v8::Local<v8::String> source;
> if (!v8::String::NewFromUtf8(...).ToLocal(&source)) {
>   ADD_FAILURE();
>   return v8::MaybeLocal<v8::Value>();
> }
> v8::Local<v8::Script> script;
> if (!v8::Script::Compile(scriptState()->context(), source).ToLocal(&script)) {
>   ADD_FAILURE();
>   return v8::MaybeLocal<v8::Value>();
> }
> ...

I did essentially the same in PS9. Is it OK?

[bashi, 2015-12-04 07:05:37]

On 2015/12/04 06:51:21, yhirano wrote:
>
https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp
> (right):
> 
>
https://codereview.chromium.org/1492763002/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/core/v8/ReadableStreamOperationsTest.cpp:128:
> v8::MaybeLocal<v8::String> source = v8::String::NewFromUtf8(isolate(), s,
> v8::String::kNormalString);
> On 2015/12/04 06:48:53, bashi1 wrote:
> > nit: It would be nice to combine empty check and value extraction:
> > 
> > v8::Local<v8::String> source;
> > if (!v8::String::NewFromUtf8(...).ToLocal(&source)) {
> >   ADD_FAILURE();
> >   return v8::MaybeLocal<v8::Value>();
> > }
> > v8::Local<v8::Script> script;
> > if (!v8::Script::Compile(scriptState()->context(), source).ToLocal(&script))
{
> >   ADD_FAILURE();
> >   return v8::MaybeLocal<v8::Value>();
> > }
> > ...
> 
> I did essentially the same in PS9. Is it OK?

Still LGTM

[yhirano, 2015-12-07 13:38:31]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-12-07 13:38:31]

The patchset sent to the CQ was uploaded after l-g-t-m from domenic@chromium.org
Link to the patchset: https://codereview.chromium.org/1492763002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-12-07 13:38:53]

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

[commit-bot: I haz the power, 2015-12-07 15:09:26]

Description was changed from

==========
Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491
==========

to

==========
Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491
==========

[commit-bot: I haz the power, 2015-12-07 15:09:27]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2015-12-07 15:10:10]

Description was changed from

==========
Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491
==========

to

==========
Add a utility class to call stream methods implemented with v8 extras.

This CL adds a class "ReadableStreamOperations" providing a easy way to
call methods of ReadableStream implemented with V8 extras.

BUG=503491

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

[commit-bot: I haz the power, 2015-12-07 15:10:11]

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