Gin: Add support for binding JS methods to C++ instance methods.

Gin: Add support for binding JS methods to C++ instance methods.

Also:
- Added support for computed properties to ObjectTemplateBuilder
- Realized it was possible for CreateFunctionTemplate to be just one template

BUG=
R=abarth@chromium.org, jyasskin@chromium.org

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

[Aaron Boodman, 2013-12-04 07:05:28]

I know that I'll need to rebase this, but let's review it at this old revision
for now.

https://codereview.chromium.org/103703002/diff/20001/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/103703002/diff/20001/gin/try_catch.cc#newcode29
gin/try_catch.cc:29: v8::Handle<v8::Message> message = try_catch_.Message();
FYI, this technically doesn't adhere to the styleguide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...

But it's pretty convenient to write this way, so ima let it slide.

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.cc
File gin/wrappable.cc (right):

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.cc#newcode37
gin/wrappable.cc:37: CHECK_EQ(kNumberOfInternalFields,
templ->InternalFieldCount());
So that the mismatch is printed in the logs if this checks.

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.h
File gin/wrappable.h (left):

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.h#oldcode85
gin/wrappable.h:85: Wrappable* wrappable = 0;
We use NULL, not zero in Chrome code

[Aaron Boodman, 2013-12-04 07:06:55]

+jyasskin for template_util* (I'll get an OWNER to rubberstamp after)

[abarth-chromium, 2013-12-04 16:27:54]

LGTM

https://codereview.chromium.org/103703002/diff/20001/gin/function_template.h
File gin/function_template.h (right):

https://codereview.chromium.org/103703002/diff/20001/gin/function_template.h#...
gin/function_template.h:43: struct RemoveConstRef<const T*> {
This seems like a slight misnomer.  Maybe we should pick a new name for this
struct?

https://codereview.chromium.org/103703002/diff/20001/gin/function_template.h#...
gin/function_template.h:203: inline bool GetNextArgument(Arguments* args, int
flags, bool is_first,
|flags| is slightly inconsistent with the name |create_flags| above.

https://codereview.chromium.org/103703002/diff/20001/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/103703002/diff/20001/gin/try_catch.cc#newcode29
gin/try_catch.cc:29: v8::Handle<v8::Message> message = try_catch_.Message();
On 2013/12/04 07:05:29, Aaron Boodman wrote:
> FYI, this technically doesn't adhere to the styleguide:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...
> 
> But it's pretty convenient to write this way, so ima let it slide.

Oh!  Sorry.  :)

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.cc
File gin/wrappable.cc (right):

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.cc#newcode37
gin/wrappable.cc:37: CHECK_EQ(kNumberOfInternalFields,
templ->InternalFieldCount());
On 2013/12/04 07:05:29, Aaron Boodman wrote:
> So that the mismatch is printed in the logs if this checks.

Thanks for fixing these.  My Chromium idioms are a bit rusty.

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.h
File gin/wrappable.h (left):

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable.h#oldcode85
gin/wrappable.h:85: Wrappable* wrappable = 0;
On 2013/12/04 07:05:29, Aaron Boodman wrote:
> We use NULL, not zero in Chrome code

Yep!

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable_unittest.cc
File gin/wrappable_unittest.cc (right):

https://codereview.chromium.org/103703002/diff/20001/gin/wrappable_unittest.c...
gin/wrappable_unittest.cc:58: .Build();
Very pretty

[Aaron Boodman, 2013-12-04 18:47:24]

jyasskin: can you also check the usage of is_member_function_pointer in
arguments.h?

[Jeffrey Yasskin, 2013-12-04 20:00:11]

Mostly lgtm

https://codereview.chromium.org/103703002/diff/20001/base/template_util.h
File base/template_util.h (right):

https://codereview.chromium.org/103703002/diff/20001/base/template_util.h#new...
base/template_util.h:31: // Member function pointer detection up to four params.
Add more as needed
There's a more complicated technique to detect arbitrary numbers of parameters
(http://stackoverflow.com/a/9008947/943619), but this is probably fine.

bind_internal also marks member-functions: 
https://code.google.com/p/chromium/codesearch/#chromium/src/base/bind_interna...
and
https://code.google.com/p/chromium/codesearch/#chromium/src/base/bind_helpers....
Let's ping awong to see if this should be exposed from Callback instead of
reinvented here.

https://codereview.chromium.org/103703002/diff/20001/gin/object_template_buil...
File gin/object_template_builder.h (right):

https://codereview.chromium.org/103703002/diff/20001/gin/object_template_buil...
gin/object_template_builder.h:28: // This partial specialization is used for
member function pointers.
You should say what the member-function-pointer specialization does differently
and why, rather than just pointing out the enable_if clause.

[Aaron Boodman, 2013-12-04 21:52:55]

On Wed, Dec 4, 2013 at 12:00 PM, <jyasskin@chromium.org> wrote:

> Mostly lgtm
>
>
> https://codereview.chromium.org/103703002/diff/20001/base/template_util.h
> File base/template_util.h (right):
>
> https://codereview.chromium.org/103703002/diff/20001/base/
> template_util.h#newcode31
> base/template_util.h:31: // Member function pointer detection up to four
> params. Add more as needed
> There's a more complicated technique to detect arbitrary numbers of
> parameters (http://stackoverflow.com/a/9008947/943619), but this is
> probably fine.
>

That is crazy, but it kind of worries me to rely on so many corner cases of
the spec ... especially things that could conceivably change in future
versions of the language.  I kind of prefer the explicitness of the simple
solution, even thought it means we'll have to keep adding overloads for
more params.

bind_internal also marks member-functions:
> https://code.google.com/p/chromium/codesearch/#chromium/
> src/base/bind_internal.h&l=34
> and
> https://code.google.com/p/chromium/codesearch/#chromium/
> src/base/bind_helpers.h&l=278.
> Let's ping awong to see if this should be exposed from Callback instead
> of reinvented here.
>

I thought about that, but since there still must be one specialization for
each variant of RunnableAdapter, it seemed to not really share anything to
have RunnableAdapter delegate to some lower-level detection.

https://codereview.chromium.org/103703002/diff/20001/gin/
> object_template_builder.h
> File gin/object_template_builder.h (right):
>
> https://codereview.chromium.org/103703002/diff/20001/gin/
> object_template_builder.h#newcode28
> gin/object_template_builder.h:28: // This partial specialization is used
> for member function pointers.
> You should say what the member-function-pointer specialization does
> differently and why, rather than just pointing out the enable_if clause.
>

Done. I also realized that I needed a specialization for base::Callback and
added that.

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

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

[Aaron Boodman, 2013-12-04 21:53:00]

On Wed, Dec 4, 2013 at 12:00 PM, <jyasskin@chromium.org> wrote:
Mostly lgtm


https://codereview.chromium.org/103703002/diff/20001/base/template_util.h
File base/template_util.h (right):

https://codereview.chromium.org/103703002/diff/20001/base/template_util.h#new...
base/template_util.h:31: // Member function pointer detection up to four
params. Add more as needed
There's a more complicated technique to detect arbitrary numbers of
parameters (http://stackoverflow.com/a/9008947/943619), but this is
probably fine.

That is crazy, but it kind of worries me to rely on so many corner cases of the
spec ... especially things that could conceivably change in future versions of
the language.  I kind of prefer the explicitness of the simple solution, even
thought it means we'll have to keep adding overloads for more params.

bind_internal also marks member-functions:
https://code.google.com/p/chromium/codesearch/#chromium/src/base/bind_interna...
and
https://code.google.com/p/chromium/codesearch/#chromium/src/base/bind_helpers....
Let's ping awong to see if this should be exposed from Callback instead
of reinvented here.

I thought about that, but since there still must be one specialization for each
variant of RunnableAdapter, it seemed to not really share anything to have
RunnableAdapter delegate to some lower-level detection.

https://codereview.chromium.org/103703002/diff/20001/gin/object_template_buil...
File gin/object_template_builder.h (right):

https://codereview.chromium.org/103703002/diff/20001/gin/object_template_buil...
gin/object_template_builder.h:28: // This partial specialization is used
for member function pointers.
You should say what the member-function-pointer specialization does
differently and why, rather than just pointing out the enable_if clause.

Done. I also realized that I needed a specialization for base::Callback and
added that.

https://codereview.chromium.org/103703002/

[Aaron Boodman, 2013-12-05 21:40:43]

+darin for rubberstamp -- Albert, if you have any changes you want, I'll be
happy to make them asynchronously.

[Aaron Boodman, 2013-12-06 06:55:29]

Committed patchset #5 manually as r239126.

[awong, 2013-12-10 02:37:23]

Sorry for the late review.

Here's a few comments. Feel free to address in another CL.

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
File gin/object_template_builder.h (right):

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
gin/object_template_builder.h:18: namespace {
erp...I'm concerned about putting an anonymous namespace in a header.

I think that if you have a.cc and b.cc both of which cause an instantiation of
CreateTemplate<<MyType>, the final binary will end up with 2 blobs of code for
what is effectively the same function.

If I'm right, then the workaround is to use a nested named namespace like
internal.

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
gin/object_template_builder.h:46: base::is_member_function_pointer<T>::value
>::type> {
Why do we need this enable_if?

Also, can this be simplified to just be an enable_if around T on the first
template?

template<typename T>
struct CallbackTraits<
  typename base::enable_if<
     base::is_member_function_pointer<T>::value, T>::type> {
};

Then you don't need the "typename Enable = void" template parameter in line 23.

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
gin/object_template_builder.h:82: // In the following methods, T and U can be
function pointer, member function
nit: can be -> can be a

[Jeffrey Yasskin, 2013-12-10 02:42:45]

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
File gin/object_template_builder.h (right):

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
gin/object_template_builder.h:18: namespace {
On 2013/12/10 02:37:24, awong wrote:
> erp...I'm concerned about putting an anonymous namespace in a header.
> 
> I think that if you have a.cc and b.cc both of which cause an instantiation of
> CreateTemplate<<MyType>, the final binary will end up with 2 blobs of code for
> what is effectively the same function.

Very good catch. It actually causes undefined behavior because
ObjectTemplateBuilder winds up with different definitions in different
translation units, although most of the time the compiler won't hurt you with
it.

> If I'm right, then the workaround is to use a nested named namespace like
> internal.

+1.

https://codereview.chromium.org/103703002/diff/80001/gin/object_template_buil...
gin/object_template_builder.h:46: base::is_member_function_pointer<T>::value
>::type> {
On 2013/12/10 02:37:24, awong wrote:
> Also, can this be simplified to just be an enable_if around T on the first
> template?
> 
> template<typename T>
> struct CallbackTraits<
>   typename base::enable_if<
>      base::is_member_function_pointer<T>::value, T>::type> {
> };

Probably not, because he's using the specialization to define different
definitions for different template arguments, and that sort of structure
matching won't deduce 'T' through a nested typedef.