Begin implementing V8 bindings for Mojo

Begin implementing V8 bindings for Mojo

This CL contains the beginnings of JavaScript bindings for the core Mojo
system. The approach in this CL is to bind as close to the "metal" as possible
so as to self-host as much as possiblem in the VM. I've tried to avoid
retaining any state on the C++ side of the bindings, but I didn't quite succeed
because V8 requires embedders to retain state in order to access the memory
that backs ArrayBuffers.

In this CL, I've added some basic bindings for the symbols exported by core.h.
Specifically, I've created bindings for CreateMessagePipe, Close, Wait,
WaitMany, WriteMessage, and ReadMessage.

R=aa@chromium.org, darin@chromium.org
BUG=317398
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234347

[Aaron Boodman, 2013-11-08 21:09:40]

I'm reviewing this now, but a meta question:

I wonder if it makes sense for the bindings system to be a separate top-level
module in src/ that Mojo relies upon.

[darin (slow to review), 2013-11-08 21:18:20]

We could do that, but then we need to come up with a name for it ;-)

-Darin


On Fri, Nov 8, 2013 at 1:09 PM, <aa@chromium.org> wrote:

> I'm reviewing this now, but a meta question:
>
> I wonder if it makes sense for the bindings system to be a separate
> top-level
> module in src/ that Mojo relies upon.
>
> https://codereview.chromium.org/59153005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[abarth-chromium, 2013-11-08 21:18:55]

On 2013/11/08 21:09:40, Aaron Boodman wrote:
> I'm reviewing this now, but a meta question:
> 
> I wonder if it makes sense for the bindings system to be a separate top-level
> module in src/ that Mojo relies upon.

There's definitely a bunch of this code that doesn't need to depend on Mojo. 
However, this bindings system isn't really compatible with the system we use in
Blink.  For example, they both want to set the embedder data field in the
v8::Isolate.  They could potentially exist in the same process if they used
different isolates, but they can't share an isolate.  In that sense, the design
is Mojo-specific.

[abarth-chromium, 2013-11-08 23:28:16]

Now with unittests!  The next step is to improve the testing infrastructure so
that it's easy to write tests.

If you like, I can split out the non-Mojo specific pieces into a separate
top-level target.  I can either do that in this CL or in a future CL.  I would
like to start landing some of this code, however, because this CL is already
>1000 LOC.  It's going to get difficult to review soon.

[Aaron Boodman, 2013-11-09 04:02:54]

I wrote something very similar to this for an internal project, and it was
named 'gin'. You can use the name if you like.

As for conflicting with Blink, I think that's fine. The Chromium project is
bigger than Blink. I think there's room for a lightweight bindings system
like this.


On Fri, Nov 8, 2013 at 1:09 PM, <aa@chromium.org> wrote:

> I'm reviewing this now, but a meta question:
>
> I wonder if it makes sense for the bindings system to be a separate
> top-level
> module in src/ that Mojo relies upon.
>
> https://codereview.chromium.org/59153005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2013-11-09 04:03:39]

(the gin name is a reference to the defunct v8-juice project, which had an
approach somewhat similar to this)


On Fri, Nov 8, 2013 at 8:02 PM, Aaron Boodman <aa@chromium.org> wrote:

> I wrote something very similar to this for an internal project, and it was
> named 'gin'. You can use the name if you like.
>
> As for conflicting with Blink, I think that's fine. The Chromium project
> is bigger than Blink. I think there's room for a lightweight bindings
> system like this.
>
>
> On Fri, Nov 8, 2013 at 1:09 PM, <aa@chromium.org> wrote:
>
>> I'm reviewing this now, but a meta question:
>>
>> I wonder if it makes sense for the bindings system to be a separate
>> top-level
>> module in src/ that Mojo relies upon.
>>
>> https://codereview.chromium.org/59153005/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2013-11-09 08:26:02]

After looking at this more, I think it makes sense to just move forward a bit
more on this code, then separate out into gin/ once it's more developed.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_conversions.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_conversions.h:5: #ifndef
MOJO_PUBLIC_BINDINGS_JS_V8_CONVERSIONS_H_
I don't understand why this isn't just another Wrapper type. Did you see the way
V8Converter<std::vector<T> > works in my old code?

It seems like that would be nice because the same code would work for arrays of
other types, like ints, or bools or whatever.

You will have to get rid of HasInstance(), but it seems like that could be
handled by making ToNative() return success/failure. Or you could rename it to
IsInstance() and define it for every Wrapper, not just "class" ones.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_core.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:17: static WrapperInfo g_handle_wrapper_info
= {};
We aren't supposed to have non-primitive static data in normal Chrome code. Do
we want to follow the same rule here?

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:22: if (!value->IsArray()) {
It seems like Wrapper<std::vector<mojo::Handle>> should do all this and this
extra function shouldn't be needed.

The only question is the error messages. I would just say to collapse both 
cases into "invalid value - expected array of mojo::Handles".

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:33: void CloseCallback(const
v8::FunctionCallbackInfo<v8::Value>& info) {
I think the term 'callback' here is confusing because we frequently use terms
like 'FooCallback' to mean 'the code that should be called after the async
process "foo" is done'.

Maybe just Close? or CloseImpl?

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:44: if (info.Length() < 2)
In my experience, this kind of code quickly gets inconsistent and unruly. There
should at least be some kind of simple V8ArgumentParser object I think:

ArgumentParser parser(info);
Foo foo = parser.GetRequired<Foo>(0);
Bar bar = parser.GetOptional<Bar>(1);
if (parser.HasError()) {
  ThrowError(parser.GetErrorMessage());
  return;
}

...

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:65: BufferView buffer(isolate,
v8::Handle<v8::ArrayBufferView>::Cast(info[0]));
It would be cool if BufferView could again have a WrapperImpl for converting to
and from JS. This would make it work with the ArgumentParser mentioned above.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_core.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.h:5: #ifndef MOJO_PUBLIC_BINDINGS_JS_V8_CORE_H_
The naming of this file confused me at first, I wasn't sure what it was core to.
I thought maybe to the bindings system, but realized it's the bindings file for
the Mojo core APIs.

This might be automatically cleared up if the bindings system itself is moved
out of this directory, and the directory contains only bindings files.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_exceptions.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_exceptions.cc:14: void ThrowTypeError(v8::Isolate*
isolate, const char* message) {
Nit: you are using 'type' error in cases where it's not really a type error,
like when an incorrect number of arguments are passed to a function.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_wrapper.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_wrapper.cc:25: return value->Int32Value();
You probably want some way for this method to indicate conversion failure.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_wrapper.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_wrapper.h:25: struct Wrapper {};
The naming is a bit confused here between Wrapper, ToObject, and ToNative.

Also, ToObject is weird because depending on T, the returned v8::Value might not
actually be an Object.

How about one of the following naming schemes:

Wrapper
  Wrap()
  Unwrap()

Converter
  ToV8Value()
  ToNative()

Also, I think that this could use a class comment. At first read, it makes me
think wrapper as in "thing around another thing", not as in "one who wraps",
which I believe is what's intended.

https://codereview.chromium.org/59153005/diff/220001/mojo/public/bindings/js/...
File mojo/public/bindings/js/v8_per_isolate_data.cc (right):

https://codereview.chromium.org/59153005/diff/220001/mojo/public/bindings/js/...
mojo/public/bindings/js/v8_per_isolate_data.cc:16: mojo_template_(isolate,
CreateMojoTemplate()) {
I think it is possible to reduce the duplication and coupling here a bit if you
want:

class V8PerIsolatedData {
 public:
  static v8::Local<v8::ObjectTemplate> GetTemplate(WrapperTypeID id,
v8::Isolate* isolate) {
    ...
  }

  static void SetTemplate(WrapperTypeID id, v8::Local<v8::ObjectTemplate> templ,
v8::Isolate* isolate) {
    ...
  }

 private:
  typedef std::map<WrapperTypeID, v8::Eternal<v8::ObjectTemplate> > StuffMap;
  StuffMap stuff_;
};

...

typdef WrapperInfo* WrapperTypeID;

template<typename T>
struct Wrapper {
 public:
  static WrapperTypeID type_id() {
    return &wrapper_info;
  }

 private:
  // If you decide to get rid of the static non-primitive data, this can just
  // be a bool for now.
  static WrapperInfo wrapper_info;
};


This way, Wrapper<T>::CreateTemplate() doesn't need to be public - lazily
creating and caching the ObjectTemplate per-isolate can be impl detail of
Wrapper<T>.

[abarth-chromium, 2013-11-09 08:52:37]

Thank you for the thoughtful review.  I'm going to try breaking this CL into
pieces to make it easier to review/land.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_conversions.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_conversions.h:5: #ifndef
MOJO_PUBLIC_BINDINGS_JS_V8_CONVERSIONS_H_
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> I don't understand why this isn't just another Wrapper type. Did you see the
way
> V8Converter<std::vector<T> > works in my old code?

I'll take a closer look.

> It seems like that would be nice because the same code would work for arrays
of
> other types, like ints, or bools or whatever.

Yes.

> You will have to get rid of HasInstance(), but it seems like that could be
> handled by making ToNative() return success/failure. Or you could rename it to
> IsInstance() and define it for every Wrapper, not just "class" ones.

That's a good idea.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_core.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:17: static WrapperInfo g_handle_wrapper_info
= {};
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> We aren't supposed to have non-primitive static data in normal Chrome code. Do
> we want to follow the same rule here?

WrapperInfo is POD.  It doesn't have any static initializers or anything.  If
that's against Chromium style, I can find another way to handle this situation.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:22: if (!value->IsArray()) {
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> It seems like Wrapper<std::vector<mojo::Handle>> should do all this and this
> extra function shouldn't be needed.
> 
> The only question is the error messages. I would just say to collapse both 
> cases into "invalid value - expected array of mojo::Handles".

Good point.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:33: void CloseCallback(const
v8::FunctionCallbackInfo<v8::Value>& info) {
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> I think the term 'callback' here is confusing because we frequently use terms
> like 'FooCallback' to mean 'the code that should be called after the async
> process "foo" is done'.
> 
> Maybe just Close? or CloseImpl?

Sorry, that's terminology from the Blink bindings leaking in.  I'll change this
to Close.  We'll still be able to call the Close from core.h because it's in a
different namespace.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:44: if (info.Length() < 2)
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> In my experience, this kind of code quickly gets inconsistent and unruly.
There
> should at least be some kind of simple V8ArgumentParser object I think:
> 
> ArgumentParser parser(info);
> Foo foo = parser.GetRequired<Foo>(0);
> Bar bar = parser.GetOptional<Bar>(1);
> if (parser.HasError()) {
>   ThrowError(parser.GetErrorMessage());
>   return;
> }
> 
> ...

Ok.  I'll work something up along those lines.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:65: BufferView buffer(isolate,
v8::Handle<v8::ArrayBufferView>::Cast(info[0]));
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> It would be cool if BufferView could again have a WrapperImpl for converting
to
> and from JS. This would make it work with the ArgumentParser mentioned above.

Ok.  I'll have to do some more work to make BufferView copyable, but that should
be doable.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_core.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.h:5: #ifndef MOJO_PUBLIC_BINDINGS_JS_V8_CORE_H_
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> The naming of this file confused me at first, I wasn't sure what it was core
to.
> I thought maybe to the bindings system, but realized it's the bindings file
for
> the Mojo core APIs.
> 
> This might be automatically cleared up if the bindings system itself is moved
> out of this directory, and the directory contains only bindings files.

Yeah.  I agree that we should separate the mojo-specific bindings from the more
generic pieces.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_exceptions.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_exceptions.cc:14: void ThrowTypeError(v8::Isolate*
isolate, const char* message) {
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> Nit: you are using 'type' error in cases where it's not really a type error,
> like when an incorrect number of arguments are passed to a function.

TypeError is what the we throw for insufficient arguments in Blink.  I think
that's in the WebIDL spec.  I'm happy to throw a different exception if you'd
prefer.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_wrapper.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_wrapper.cc:25: return value->Int32Value();
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> You probably want some way for this method to indicate conversion failure.

As written, the conversion can't really fail.  If I'm understanding the API
correctly, this is the moral equivalent of writing

(value)|0

in JavaScript.  Specifically, I believe it calls valueOf and then forces the
result to a number.  For example ({})|0 is 0.

Another option is to check IsNumber first and have a conversion that can fall. 
That might be more predictable for callers because it's not going to call
valueOf at surprising times.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_wrapper.h (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_wrapper.h:25: struct Wrapper {};
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> The naming is a bit confused here between Wrapper, ToObject, and ToNative.
> 
> Also, ToObject is weird because depending on T, the returned v8::Value might
not
> actually be an Object.
> 
> How about one of the following naming schemes:
> 
> Wrapper
>   Wrap()
>   Unwrap()
> 
> Converter
>   ToV8Value()
>   ToNative()
> 
> Also, I think that this could use a class comment. At first read, it makes me
> think wrapper as in "thing around another thing", not as in "one who wraps",
> which I believe is what's intended.

Yeah, this code evolved but the names didn't.  I like the Converter names. 
Let's do that.

https://codereview.chromium.org/59153005/diff/220001/mojo/public/bindings/js/...
File mojo/public/bindings/js/v8_per_isolate_data.cc (right):

https://codereview.chromium.org/59153005/diff/220001/mojo/public/bindings/js/...
mojo/public/bindings/js/v8_per_isolate_data.cc:16: mojo_template_(isolate,
CreateMojoTemplate()) {
On 2013/11/09 08:26:02, Aaron Boodman wrote:
> I think it is possible to reduce the duplication and coupling here a bit
> if you want:

Will do.  That will make it easier to separate the Mojo-independent pieces.

[abarth-chromium, 2013-11-09 11:52:20]

I've extracted the non-Mojo specific parts of this CL into
https://codereview.chromium.org/67763002/.  I'll rebase this CL on top of that
CL once it lands.  Thanks for all the feedback!

[Aaron Boodman, 2013-11-09 21:12:11]

I'll review the other CL too, but I wanted to respond to these points in
context.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_core.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_core.cc:17: static WrapperInfo g_handle_wrapper_info
= {};
On 2013/11/09 08:52:37, abarth wrote:
> On 2013/11/09 08:26:02, Aaron Boodman wrote:
> > We aren't supposed to have non-primitive static data in normal Chrome code.
Do
> > we want to follow the same rule here?
> 
> WrapperInfo is POD.  It doesn't have any static initializers or anything.  If
> that's against Chromium style, I can find another way to handle this
situation.

It seems like that should be OK logically, but I vaguely remember mpcomplete
arguing against any non-primitive types. Ask darin or trung? I'll go with
whatever one of them says.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_exceptions.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_exceptions.cc:14: void ThrowTypeError(v8::Isolate*
isolate, const char* message) {
On 2013/11/09 08:52:37, abarth wrote:
> On 2013/11/09 08:26:02, Aaron Boodman wrote:
> > Nit: you are using 'type' error in cases where it's not really a type error,
> > like when an incorrect number of arguments are passed to a function.
> 
> TypeError is what the we throw for insufficient arguments in Blink.  I think
> that's in the WebIDL spec.  I'm happy to throw a different exception if you'd
> prefer.

Oh, if that's what's in the IDL spec, that's a good justification.

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
File mojo/public/bindings/js/v8_wrapper.cc (right):

https://codereview.chromium.org/59153005/diff/30001/mojo/public/bindings/js/v...
mojo/public/bindings/js/v8_wrapper.cc:25: return value->Int32Value();
On 2013/11/09 08:52:37, abarth wrote:
> On 2013/11/09 08:26:02, Aaron Boodman wrote:
> > You probably want some way for this method to indicate conversion failure.
> 
> As written, the conversion can't really fail.  If I'm understanding the API
> correctly, this is the moral equivalent of writing
> 
> (value)|0
> 
> in JavaScript.  Specifically, I believe it calls valueOf and then forces the
> result to a number.  For example ({})|0 is 0.
> 
> Another option is to check IsNumber first and have a conversion that can fall.

> That might be more predictable for callers because it's not going to call
> valueOf at surprising times.

For number it can't fail, but for, e.g., mojo::Handle, it can. Unless you want
it to just return null/undef for failure in those cases. That could be OK.

I think it is my personal preference as a user of APIs that they are strict,
throw errors, and don't coerce everything every which way automatically. It's
more common for me to get the argument list wrong than it is for me to actually
want to pass {} and have it be treated as a number.

[abarth-chromium, 2013-11-11 00:00:31]

Ok, I've reworked this CL to layer on top of Gini (still called Gin in the
code).  I've got bindings for all the core Mojo APIs.  The next step is to add a
simple testing harness.

You (Aaron or Darin) are welcome to review this CL as it is currently, or I'm
happy to include the testing harness in this CL if you want to wait until this
code can actually be executed before reviewing it.

[abarth-chromium, 2013-11-11 10:13:18]

Ok, this CL has some testing now.  It's ready for review.  Thanks!

[Aaron Boodman, 2013-11-11 19:01:08]

https://codereview.chromium.org/59153005/diff/620001/gin/arguments.h
File gin/arguments.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/arguments.h#newcode24
gin/arguments.h:24: bool Next(T* out) {
I think we'll eventually need Get(index) too, but that can be added when needed.
For simple cases, Next() is a bit cleaner-looking than having all the magic
numbers for argument positions at the call site.

https://codereview.chromium.org/59153005/diff/620001/gin/arguments.h#newcode24
gin/arguments.h:24: bool Next(T* out) {
I feel like it is Chromium (or Google) style to use have method names be verb
phrases, so GetNext() instead of Next().

https://codereview.chromium.org/59153005/diff/620001/gin/converter.cc
File gin/converter.cc (right):

https://codereview.chromium.org/59153005/diff/620001/gin/converter.cc#newcode59
gin/converter.cc:59: *out = val->IntegerValue();
I was surprised to see that IntegerValue() returns int64_t. You might want to
add a comment to that effect.

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.cc
File gin/per_isolate_data.cc (right):

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.cc#...
gin/per_isolate_data.cc:33: bool PerIsolateData::GetObjectTemplate(WrapperInfo*
info,
You can avoid the two return values by having this function return
Local<ObjectTemplate> and having it be empty on failure.

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.h
File gin/per_isolate_data.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.h#n...
gin/per_isolate_data.h:23: void RegisterObjectTemplate(WrapperInfo* info,
Change this to Set, just for parallelism with Get?

https://codereview.chromium.org/59153005/diff/620001/gin/runner.h
File gin/runner.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/runner.h#newcode9
gin/runner.h:9: #include "base/compiler_specific.h"
Unnecessary include?

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/core.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:20: void Close(const
v8::FunctionCallbackInfo<v8::Value>& info) {
The C++ template stuff I had in the other project would make many of these
functions almost disappear.

You'd just do:

templ->Set("setMonkey", gin::Bind(SetMonkey));
templ->Set("getMonkey", gin::Bind(GetMonkey));
... etc ...

I feel like it will also be useful for people like Jochen who want to start
using it to replacing CppBoundClass. Thoughts?

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:23: mojo::Handle handle;
The uninitialized locals that Argument's interface encourages bug me. The
alternative would be to have Next() just return the value directly, and have it
be responsible for returning an empty value on error:

mojo::Handle handle = args.Next<mojo::Handle>();
Thing thing = args.Next<Thing>();
if (args.HasError()) {
  args.ThrowError();
}

This is more typing but eliminates the possibility of uninitialized mistakes.
WDYT?

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:101: return args.ThrowTypeError("Expected
MojoWaitFlags.");
I think you should make Arguments responsible for accumulating errors and
creating an error message:

if (!args.Next(&handle) ||
    !args.Next(&buffer) ||
    !args.Next(&handles) ... {
  return args.ThrowError();
}

I also think you'll want the error messages to indicates the index of the
incorrect argument, which Arguments is in a better position to do than the
client code.

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/mojo.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/mojo.cc:26: templ->Set(gin::StringToV8(isolate, "core"),
CoreTemplate(isolate));
Do you want to use symbols for these static strings?

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/mojo.h (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/mojo.h:13: v8::Local<v8::ObjectTemplate>
MojoTemplate(v8::Isolate* isolate);
GetMojoTemplate()

[abarth-chromium, 2013-11-11 20:17:41]

Thanks for the review!

https://codereview.chromium.org/59153005/diff/620001/gin/arguments.h
File gin/arguments.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/arguments.h#newcode24
gin/arguments.h:24: bool Next(T* out) {
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> I feel like it is Chromium (or Google) style to use have method names be verb
> phrases, so GetNext() instead of Next().

Will do.

https://codereview.chromium.org/59153005/diff/620001/gin/converter.cc
File gin/converter.cc (right):

https://codereview.chromium.org/59153005/diff/620001/gin/converter.cc#newcode59
gin/converter.cc:59: *out = val->IntegerValue();
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> I was surprised to see that IntegerValue() returns int64_t. You might want to
> add a comment to that effect.

Will do.

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.cc
File gin/per_isolate_data.cc (right):

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.cc#...
gin/per_isolate_data.cc:33: bool PerIsolateData::GetObjectTemplate(WrapperInfo*
info,
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> You can avoid the two return values by having this function return
> Local<ObjectTemplate> and having it be empty on failure.

Ok.

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.h
File gin/per_isolate_data.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/per_isolate_data.h#n...
gin/per_isolate_data.h:23: void RegisterObjectTemplate(WrapperInfo* info,
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> Change this to Set, just for parallelism with Get?

Will do.

https://codereview.chromium.org/59153005/diff/620001/gin/runner.h
File gin/runner.h (right):

https://codereview.chromium.org/59153005/diff/620001/gin/runner.h#newcode9
gin/runner.h:9: #include "base/compiler_specific.h"
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> Unnecessary include?

No, I'm cheating a bit.  The files that include this header need to see OVERRIDE
when subclassing RunnerDelegate, but I didn't want to add base to their DEPS
file.  Maybe the solution is to whitelist basictypes.h and compiler_specific.h
in mojo/public/bindings/js/DEPS

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/core.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:20: void Close(const
v8::FunctionCallbackInfo<v8::Value>& info) {
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> The C++ template stuff I had in the other project would make many of these
> functions almost disappear.
> 
> You'd just do:
> 
> templ->Set("setMonkey", gin::Bind(SetMonkey));
> templ->Set("getMonkey", gin::Bind(GetMonkey));
> ... etc ...
> 
> I feel like it will also be useful for people like Jochen who want to start
> using it to replacing CppBoundClass. Thoughts?

Yes, I think gin::Bind is super cool.  Would you like me to add that in this CL
or in the next CL.

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:23: mojo::Handle handle;
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> The uninitialized locals that Argument's interface encourages bug me. The
> alternative would be to have Next() just return the value directly, and have
it
> be responsible for returning an empty value on error:
> 
> mojo::Handle handle = args.Next<mojo::Handle>();
> Thing thing = args.Next<Thing>();
> if (args.HasError()) {
>   args.ThrowError();
> }
> 
> This is more typing but eliminates the possibility of uninitialized mistakes.
> WDYT?

Sure, we can do that.

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:101: return args.ThrowTypeError("Expected
MojoWaitFlags.");
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> I think you should make Arguments responsible for accumulating errors and
> creating an error message:
> 
> if (!args.Next(&handle) ||
>     !args.Next(&buffer) ||
>     !args.Next(&handles) ... {
>   return args.ThrowError();
> }
> 
> I also think you'll want the error messages to indicates the index of the
> incorrect argument, which Arguments is in a better position to do than the
> client code.

Makes sense.  It won't know how to stringify the types, but reporting the
position is probably more valuable.

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/mojo.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/mojo.cc:26: templ->Set(gin::StringToV8(isolate, "core"),
CoreTemplate(isolate));
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> Do you want to use symbols for these static strings?

Yes.  I'll add a gin::StringToSymbol function.

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/mojo.h (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/mojo.h:13: v8::Local<v8::ObjectTemplate>
MojoTemplate(v8::Isolate* isolate);
On 2013/11/11 19:01:08, Aaron Boodman wrote:
> GetMojoTemplate()

Will do.

[Aaron Boodman, 2013-11-11 20:33:22]

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/core.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:20: void Close(const
v8::FunctionCallbackInfo<v8::Value>& info) {
On 2013/11/11 20:17:42, abarth wrote:
> On 2013/11/11 19:01:08, Aaron Boodman wrote:
> > The C++ template stuff I had in the other project would make many of these
> > functions almost disappear.
> > 
> > You'd just do:
> > 
> > templ->Set("setMonkey", gin::Bind(SetMonkey));
> > templ->Set("getMonkey", gin::Bind(GetMonkey));
> > ... etc ...
> > 
> > I feel like it will also be useful for people like Jochen who want to start
> > using it to replacing CppBoundClass. Thoughts?
> 
> Yes, I think gin::Bind is super cool.  Would you like me to add that in this
CL
> or in the next CL.

If you think it is useful, I will do it this week.

[abarth-chromium, 2013-11-11 21:05:48]

Done.  The only comment I didn't address was having GetNext return a T instead
of taking a T* argument.  PTAL.  Thanks!

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/core.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:20: void Close(const
v8::FunctionCallbackInfo<v8::Value>& info) {
On 2013/11/11 20:33:23, Aaron Boodman wrote:
> If you think it is useful, I will do it this week.

Thanks!

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:23: mojo::Handle handle;
On 2013/11/11 20:17:42, abarth wrote:
> On 2013/11/11 19:01:08, Aaron Boodman wrote:
> > This is more typing but eliminates the possibility of uninitialized
mistakes.
> > WDYT?
> 
> Sure, we can do that.

Oh, I remember why I did it this way.  The problem is that ArrayBufferView needs
the v8::Isolate in it's contructor, which means GetNext won't be smart enough to
make one.

We could add some traits for initializing these types, but we'd have to know the
superset of what all objects need in their constructions...

I've changed this to initialize all the variables in this file.

[Aaron Boodman, 2013-11-11 21:12:48]

lgtm

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
File mojo/public/bindings/js/core.cc (right):

https://codereview.chromium.org/59153005/diff/620001/mojo/public/bindings/js/...
mojo/public/bindings/js/core.cc:101: return args.ThrowTypeError("Expected
MojoWaitFlags.");
On 2013/11/11 20:17:42, abarth wrote:
> On 2013/11/11 19:01:08, Aaron Boodman wrote:
> > I think you should make Arguments responsible for accumulating errors and
> > creating an error message:
> > 
> > if (!args.Next(&handle) ||
> >     !args.Next(&buffer) ||
> >     !args.Next(&handles) ... {
> >   return args.ThrowError();
> > }
> > 
> > I also think you'll want the error messages to indicates the index of the
> > incorrect argument, which Arguments is in a better position to do than the
> > client code.
> 
> Makes sense.  It won't know how to stringify the types, but reporting the
> position is probably more valuable.

Note: you didn't address this one. I'm fine with it in the next CL though.

[abarth-chromium, 2013-11-11 21:32:12]

On 2013/11/11 21:12:48, Aaron Boodman wrote:
> Note: you didn't address this one. I'm fine with it in the next CL though.

Oops!  Fixed.

[commit-bot: I haz the power, 2013-11-11 21:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/59153005/680003

[Aaron Boodman, 2013-11-11 21:37:31]

https://codereview.chromium.org/59153005/diff/680003/gin/arguments.cc
File gin/arguments.cc (right):

https://codereview.chromium.org/59153005/diff/680003/gin/arguments.cc#newcode27
gin/arguments.cc:27: stream << "Error processing argument " << next_ - 1 << ".";
This will do the wrong thing when the client calls GetNext() after there was an
error. You need to save whether or not you are in an error state.

[Aaron Boodman, 2013-11-11 21:38:58]

https://codereview.chromium.org/59153005/diff/680003/gin/arguments.cc
File gin/arguments.cc (right):

https://codereview.chromium.org/59153005/diff/680003/gin/arguments.cc#newcode27
gin/arguments.cc:27: stream << "Error processing argument " << next_ - 1 << ".";
On 2013/11/11 21:37:32, Aaron Boodman wrote:
> This will do the wrong thing when the client calls GetNext() after there was
an
> error. You need to save whether or not you are in an error state.

Nevermind. LGTM.

[abarth-chromium, 2013-11-11 21:42:24]

It does the wrong thing if you call ThrowError before calling GetNext.  I
imagine we'll want something fancier in the future.

[abarth-chromium, 2013-11-12 00:41:31]

Committed patchset #10 manually as r234347.