First cut at gin::Bind()

First cut at gin::Bind().

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236958

[Aaron Boodman, 2013-11-22 07:56:39]

Well, I got the first example working using base::Callback. I haven't
implemented the overloads for different arities yet, or implemented return
values or optional parameters.

I'm going back and forth a bit about whether it is worth it to rely on Callback.
There are a bit differing accounts on
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/EYbjUkDwpeY...
as to whether there is a performance hit with callback.Run() when bound to a
simple function pointer, over just invoking through a function pointer.

Opinions on what I have so far?

Also, Albert, I noticed your dire warning about not wrapping base::Callback, so
cc'ing you as well.

[abarth-chromium, 2013-11-22 08:02:55]

On 2013/11/22 07:56:39, Aaron Boodman wrote:
> I'm going back and forth a bit about whether it is worth it to rely on
Callback.

Given that we're not binding any of the parameters and CallbackHolder is already
templated struct, it seems like we could just hold the function pointer directly
in CallbackHolder...

[Aaron Boodman, 2013-11-22 08:07:15]

On Fri, Nov 22, 2013 at 12:02 AM, <abarth@chromium.org> wrote:

> On 2013/11/22 07:56:39, Aaron Boodman wrote:
>
>> I'm going back and forth a bit about whether it is worth it to rely on
>>
> Callback.
>
> Given that we're not binding any of the parameters and CallbackHolder is
> already
> templated struct, it seems like we could just hold the function pointer
> directly
> in CallbackHolder...
>

Yes. The thought is just that it might be useful to be able to call a
method on an object as well as a static function. Or that it might be
useful to pre-bind params to pass through.

The Callback systems wraps all this up very neatly, and it's very hard to
get right otherwise.



>
> https://codereview.chromium.org/76923003/
>

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

[abarth-chromium, 2013-11-22 08:09:34]

On 2013/11/22 08:07:15, Aaron Boodman wrote:
> Yes. The thought is just that it might be useful to be able to call a
> method on an object as well as a static function. Or that it might be
> useful to pre-bind params to pass through.
> 
> The Callback systems wraps all this up very neatly, and it's very hard to
> get right otherwise.

Oh, I see.  We want to bind |this|.  Yeah, that makes sense.

We wouldn't be able to use this system for all the bindings in Blink, but I
think that's ok.  For the performance critical APIs, we can dig down and
customize things.  For most APIs, ideally we'd optimize for binary size.

[jochen (gone - plz use gerrit), 2013-11-22 08:12:09]

I hacked something together where you'd call

class Foo : public gin::Wrappable {
  int bar(int blub);
};

Foo *foo = new Foo;
gin::Bind(&Foo::bar, foo, "bar");

which puts a property "bar" on the object template used for Foo. I'm not sure
whether the first cut of the gin Bind api should support binding stuff for
non-wrappable objects, wdyt?

[abarth-chromium, 2013-11-22 08:17:42]

On 2013/11/22 08:12:09, jochen wrote:
> I hacked something together where you'd call
> 
> class Foo : public gin::Wrappable {
>   int bar(int blub);
> };
> 
> Foo *foo = new Foo;
> gin::Bind(&Foo::bar, foo, "bar");
> 
> which puts a property "bar" on the object template used for Foo. I'm not sure
> whether the first cut of the gin Bind api should support binding stuff for
> non-wrappable objects, wdyt?

It's odd that you'd need an instance of Foo to configure it's ObjectTemplate...

[jochen (gone - plz use gerrit), 2013-11-22 08:19:18]

On 2013/11/22 08:17:42, abarth wrote:
> On 2013/11/22 08:12:09, jochen wrote:
> > I hacked something together where you'd call
> > 
> > class Foo : public gin::Wrappable {
> >   int bar(int blub);
> > };
> > 
> > Foo *foo = new Foo;
> > gin::Bind(&Foo::bar, foo, "bar");
> > 
> > which puts a property "bar" on the object template used for Foo. I'm not
sure
> > whether the first cut of the gin Bind api should support binding stuff for
> > non-wrappable objects, wdyt?
> 
> It's odd that you'd need an instance of Foo to configure it's
ObjectTemplate...

in my hack this parameter is mainly for the template matching to deduce the
class type.

[Aaron Boodman, 2013-11-22 08:19:53]

On Fri, Nov 22, 2013 at 12:12 AM, <jochen@chromium.org> wrote:

> I hacked something together where you'd call
>
> class Foo : public gin::Wrappable {
>   int bar(int blub);
> };
>
> Foo *foo = new Foo;
> gin::Bind(&Foo::bar, foo, "bar");
>
> which puts a property "bar" on the object template used for Foo.


I don't think that |foo| (the instance) should be a part of bind(). The
function template is associated with the object template of the class Foo,
not the instance foo. Registration of function templates should just be
done once, when the object template is registered with PerIsolateData.

I'm not sure
> whether the first cut of the gin Bind api should support binding stuff for
> non-wrappable objects, wdyt?
>

I hadn't quite worked out what the interaction with Wrappable should be. I
agree there should be an easy path for common things people want to do.
Sometimes it's nice to have the lower-level prims available too.


>
> https://codereview.chromium.org/76923003/
>

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

[jochen (gone - plz use gerrit), 2013-11-22 08:24:55]

in https://codereview.chromium.org/69953006 I wrote a similar "holder to
callback" thing that used a weak ptr to free itself up

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_util.h
File gin/function_template_util.h (right):

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_uti...
gin/function_template_util.h:66: return args.ThrowError();
the function is void

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_uti...
gin/function_template_util.h:78: holder->callback = callback;
where is holder deleted again?

[Aaron Boodman, 2013-11-22 08:27:59]

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_util.h
File gin/function_template_util.h (right):

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_uti...
gin/function_template_util.h:78: holder->callback = callback;
On 2013/11/22 08:24:55, jochen wrote:
> where is holder deleted again?

The Wrappable base class deletes it during GC. (Wrappable sets up join ownership
between GC and RefCounted ... but we only have this one reference here, and then
no more. So after this point, it's only owned by GC).

[jochen (gone - plz use gerrit), 2013-11-22 08:29:08]

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_util.h
File gin/function_template_util.h (right):

https://codereview.chromium.org/76923003/diff/30001/gin/function_template_uti...
gin/function_template_util.h:78: holder->callback = callback;
On 2013/11/22 08:28:00, Aaron Boodman wrote:
> On 2013/11/22 08:24:55, jochen wrote:
> > where is holder deleted again?
> 
> The Wrappable base class deletes it during GC. (Wrappable sets up join
ownership
> between GC and RefCounted ... but we only have this one reference here, and
then
> no more. So after this point, it's only owned by GC).

ah, makes sense

[jochen (gone - plz use gerrit), 2013-11-22 08:33:27]

i don't know how you feel about this, but there aren't many users yet, so I
think it's fine to start iterating with this

lgtm

[abarth-chromium, 2013-11-22 09:17:13]

The approach LGTM.  The comments below are mostly naming nits.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
File gin/function_template_util.cc (right):

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.cc:5: #include "gin/function_template_util.h"
function_template_util  <-- Can we give this a snappier name?  Maybe I'm
prejudiced against names with "util"...  How about function_template.h ?

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.cc:12: void CallbackHolderBase::Register(v8::Isolate*
isolate) {
EnsureRegistered?  Who's job is it to call this function?  Maybe we should call
it in gin.cc since almost every embedder is going to need it.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
File gin/function_template_util.h (right):

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:12: #include "gin/wrappable.h"
You've got a bunch more headers than you need here.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:41: class CallbackHolder : public
CallbackHolderBase {
Should we move CallbackHolder and CallbackHolderBase into an "internal"
namespace?

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:52: static void DispatchV8Call(const
v8::FunctionCallbackInfo<v8::Value>& info) {
DispatchV8Call -> DispatchToCallback ?

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:63: P2Storage a2;
You can just write:

typename RemoveConstRef<P1>::Type a2;

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:79: holder->callback = callback;
It might be slightly prettier to pass the callback via the HolderT constructor.

https://codereview.chromium.org/76923003/diff/190001/gin/test/gtest.cc
File gin/test/gtest.cc (right):

https://codereview.chromium.org/76923003/diff/190001/gin/test/gtest.cc#newcode55
gin/test/gtest.cc:55: CreateFunctionTempate(isolate, base::Bind(ExpectFalse)));
This looks really nice.

[Aaron Boodman, 2013-11-22 18:01:00]

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
File gin/function_template_util.cc (right):

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.cc:5: #include "gin/function_template_util.h"
On 2013/11/22 09:17:13, abarth wrote:
> function_template_util  <-- Can we give this a snappier name?  Maybe I'm
> prejudiced against names with "util"...  How about function_template.h ?

Done.

Yeah I always struggle with naming with C++ templates, because they tend to not
be object-oriented. Naming suggestions throughout welcome.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
File gin/function_template_util.h (right):

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:12: #include "gin/wrappable.h"
On 2013/11/22 09:17:13, abarth wrote:
> You've got a bunch more headers than you need here.

It's Google (and Chrome) style:
https://code.google.com/p/include-what-you-use/wiki/WhyIWYU

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:41: class CallbackHolder : public
CallbackHolderBase {
On 2013/11/22 09:17:13, abarth wrote:
> Should we move CallbackHolder and CallbackHolderBase into an "internal"
> namespace?

C++ template esoterica #23: template specializations must be defined in same
namespace as template:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3730.html

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:52: static void DispatchV8Call(const
v8::FunctionCallbackInfo<v8::Value>& info) {
On 2013/11/22 09:17:13, abarth wrote:
> DispatchV8Call -> DispatchToCallback ?

Done.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:63: P2Storage a2;
On 2013/11/22 09:17:13, abarth wrote:
> You can just write:
> 
> typename RemoveConstRef<P1>::Type a2;

Fair enough. I think I was using these types in more places before.

https://codereview.chromium.org/76923003/diff/190001/gin/function_template_ut...
gin/function_template_util.h:79: holder->callback = callback;
On 2013/11/22 09:17:13, abarth wrote:
> It might be slightly prettier to pass the callback via the HolderT
constructor.

Done.

[abarth-chromium, 2013-11-23 00:39:04]

LGTM

https://codereview.chromium.org/76923003/diff/330001/gin/function_template.h
File gin/function_template.h (right):

https://codereview.chromium.org/76923003/diff/330001/gin/function_template.h#...
gin/function_template.h:12: 
extra blank line here.

[commit-bot: I haz the power, 2013-11-23 03:26:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aa@chromium.org/76923003/330001

[commit-bot: I haz the power, 2013-11-23 20:26:53]

Change committed as 236958