[gin] Introduce Wrappable::GetObjectTemplate

[gin] Introduce Wrappable::GetObjectTemplate

Instead of explicitly registering object templates for all wrapper
infos, add a method on wrappable that returns the template when
needed.

BUG=none
R=aa@chromium.org,abarth@chromium.org

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

[Aaron Boodman, 2013-12-13 18:43:23]

This still isn't sitting right with me.

There's a minor thing that a method called 'builder' doesn't return anything,
but the I think the bigger thing is that these classes expose GetTemplate()
publicly in the first place. It seems like something that should be an
implementation detail: the first time a Wrappble needs to be wrapped, it should
create its object template, register it with the isolate, and return the wrapped
object. We shouldn't need every such class to have some registration done at
startup. As our object graphs get bigger, the up front registration is going to
become a pain.

What about something like this:

class PerIsolateData {
 public:
+ typedef v8::Local<v8::ObjectTemplate> (*TemplateMaker) ();

+ // Returns any registered object template for WrapperInfo* or creates one via
TemplateMaker* and returns that.
+ v8::Local<v8::ObjectTemplate> GetObjectTemplate(WrapperInfo*, TemplateMaker*);
};

Then, GetTemplate() on all these classes goes away and the builder logic somehow
(handwaving) gets called via Wrappable::GetWrapper().

[jochen (gone - plz use gerrit), 2013-12-13 19:06:22]

On 2013/12/13 18:43:23, Aaron Boodman wrote:
> This still isn't sitting right with me.
> 
> There's a minor thing that a method called 'builder' doesn't return anything,
> but the I think the bigger thing is that these classes expose GetTemplate()
> publicly in the first place. It seems like something that should be an
> implementation detail: the first time a Wrappble needs to be wrapped, it
should
> create its object template, register it with the isolate, and return the
wrapped
> object. We shouldn't need every such class to have some registration done at
> startup. As our object graphs get bigger, the up front registration is going
to
> become a pain.
> 
> What about something like this:
> 
> class PerIsolateData {
>  public:
> + typedef v8::Local<v8::ObjectTemplate> (*TemplateMaker) ();
> 
> + // Returns any registered object template for WrapperInfo* or creates one
via
> TemplateMaker* and returns that.
> + v8::Local<v8::ObjectTemplate> GetObjectTemplate(WrapperInfo*,
TemplateMaker*);
> };
> 
> Then, GetTemplate() on all these classes goes away and the builder logic
somehow
> (handwaving) gets called via Wrappable::GetWrapper().

Maybe Wrappable should just have a virtual v8::Handle<v8::ObjectTemplate>
GetTemplate().

GetWrapper then checks whether it finds one on the PerIsolateData and if not
installs the one returned by GetTemplate()

[Aaron Boodman, 2013-12-13 19:26:48]

On 2013/12/13 19:06:22, jochen wrote:
> On 2013/12/13 18:43:23, Aaron Boodman wrote:
> > This still isn't sitting right with me.
> > 
> > There's a minor thing that a method called 'builder' doesn't return
anything,
> > but the I think the bigger thing is that these classes expose GetTemplate()
> > publicly in the first place. It seems like something that should be an
> > implementation detail: the first time a Wrappble needs to be wrapped, it
> should
> > create its object template, register it with the isolate, and return the
> wrapped
> > object. We shouldn't need every such class to have some registration done at
> > startup. As our object graphs get bigger, the up front registration is going
> to
> > become a pain.
> > 
> > What about something like this:
> > 
> > class PerIsolateData {
> >  public:
> > + typedef v8::Local<v8::ObjectTemplate> (*TemplateMaker) ();
> > 
> > + // Returns any registered object template for WrapperInfo* or creates one
> via
> > TemplateMaker* and returns that.
> > + v8::Local<v8::ObjectTemplate> GetObjectTemplate(WrapperInfo*,
> TemplateMaker*);
> > };
> > 
> > Then, GetTemplate() on all these classes goes away and the builder logic
> somehow
> > (handwaving) gets called via Wrappable::GetWrapper().
> 
> Maybe Wrappable should just have a virtual v8::Handle<v8::ObjectTemplate>
> GetTemplate().
> 
> GetWrapper then checks whether it finds one on the PerIsolateData and if not
> installs the one returned by GetTemplate()

Oh yeah, that sounds good. One thing: I would prefer that GetTemplate() is not
virtual, so that there are no virtual calls on the common js function dispatch
path. But I think that can be handled using the same CRTP approach that
Wrappable is using now for kWrapperInfo.

[jochen (gone - plz use gerrit), 2013-12-16 10:14:31]

PTAL

[Aaron Boodman, 2013-12-16 16:44:26]

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h#newcode35
gin/wrappable.h:35: //   static v8::Local<v8::ObjectTemplate> GetObjectTemplate(
It's ok for this to be public now (I think before what was bothering me was that
the responsibility for calling it was with the user).

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h#newcode61
gin/wrappable.h:61: typedef v8::Local<v8::ObjectTemplate>
(*TemplateBuilder)(v8::Isolate*);
Confusing name since we also have ObjectTemplateBuilder. How about just
*GetObjectTemplateFunction

https://codereview.chromium.org/113893005/diff/20001/mojo/apps/js/bindings/gl...
File mojo/apps/js/bindings/gl/context.cc (right):

https://codereview.chromium.org/113893005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/context.cc:26: return
gin::ObjectTemplateBuilder(isolate)
Ah, that looks nicer

[Aaron Boodman, 2013-12-16 16:50:35]

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h#newcode35
gin/wrappable.h:35: //   static v8::Local<v8::ObjectTemplate> GetObjectTemplate(
Some classes don't want to have any methods or properties on the JS object --
they just use the JS wrapper for lifetime management.

For those cases it would be nice to have a default implementation of
GetObjectTemplate(). Can this be achieved without making GetObjectTemplate()
virtual by having an impl in WrapperBase? I forget how resolution would work in
that case.

[jochen (gone - plz use gerrit), 2013-12-17 14:32:31]

ptal

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h#newcode35
gin/wrappable.h:35: //   static v8::Local<v8::ObjectTemplate> GetObjectTemplate(
On 2013/12/16 16:50:35, Aaron Boodman wrote:
> Some classes don't want to have any methods or properties on the JS object --
> they just use the JS wrapper for lifetime management.
> 
> For those cases it would be nice to have a default implementation of
> GetObjectTemplate(). Can this be achieved without making GetObjectTemplate()
> virtual by having an impl in WrapperBase? I forget how resolution would work
in
> that case.

Done.

https://codereview.chromium.org/113893005/diff/20001/gin/wrappable.h#newcode61
gin/wrappable.h:61: typedef v8::Local<v8::ObjectTemplate>
(*TemplateBuilder)(v8::Isolate*);
On 2013/12/16 16:44:26, Aaron Boodman wrote:
> Confusing name since we also have ObjectTemplateBuilder. How about just
> *GetObjectTemplateFunction

Done.

[Aaron Boodman, 2013-12-17 17:58:20]

LGTM

Thanks!

https://codereview.chromium.org/113893005/diff/160001/gin/wrappable.h
File gin/wrappable.h (right):

https://codereview.chromium.org/113893005/diff/160001/gin/wrappable.h#newcode44
gin/wrappable.h:44: //   return ObjectTemplateBuilder(isolate).Build();
Can you add a few example calls to SetMethod and SetValue?

[jochen (gone - plz use gerrit), 2013-12-17 18:59:33]

I added a SetValue call. SetMethod is a bit confusing unless you also add
methods to the sample class which is a bit overkill.

[commit-bot: I haz the power, 2013-12-17 18:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/113893005/170001

[commit-bot: I haz the power, 2013-12-17 21:47:06]

Change committed as 241370

[abarth-chromium, 2013-12-19 07:51:49]

Very nice