Beginning of JS Mojo API to GL

Beginning of JS Mojo API to GL.

R=abarth@chromium.org

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

[Aaron Boodman, 2013-12-11 00:21:50]

There are a few questions for you in the comments as well, in context.cc.

https://codereview.chromium.org/111083005/diff/1/gin/function_template.h
File gin/function_template.h (right):

https://codereview.chromium.org/111083005/diff/1/gin/function_template.h#newc...
gin/function_template.h:63: static WrapperInfo kWrapperInfo;
I got rid of the macro because it did not work well when called inside a
namespace. So you have to define this one static field manually again.

[abarth-chromium, 2013-12-11 06:24:21]

This looks like a good start.  There's some tension between your wanting to put
compileShader and such on an instance object and having glCompileShader be a
static function that finds the context in TLS.

If I were doing this, I'd just make a module named "gl" and have it contain a
bunch of static functions.  That makes most directly to the C API and will work
the same was a WebGL as long as there's only one context.  We can cross the
multiple contexts bridge when we come to it.

https://codereview.chromium.org/111083005/diff/20001/gin/handle.h
File gin/handle.h (left):

https://codereview.chromium.org/111083005/diff/20001/gin/handle.h#oldcode52
gin/handle.h:52: return gin::Handle<T>(val, object);
wat

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/context.cc:73: // TODO(aa): Does this need to be called
at the start of every public method?
Probably not.  We only support on GL context right now, so you just need to make
it current once.

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/context.h:22: namespace gl {
Do we want to have this deep a nesting of namespaces?

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/module.cc:21: const char* kModuleName =
"mojo/apps/js/bindings/gljs";
mojo/apps/js/bindings/gljs  ->  mojo/apps/js/bindings/gl ?

Normally the module name corresponds to the file path.  We'll need to figure out
what stable names we want for all these modules eventually.

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/opaque.cc:30: }
This boilerplate does seem ripe for factoring out.

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/main.js
File mojo/apps/js/main.js (right):

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/main.js#new...
mojo/apps/js/main.js:54: var context = new gljs.Context(encoded, width, height);
Typically folks use a variable named |gl| for the context.

[Aaron Boodman, 2013-12-11 07:17:06]

I'd rather do things right from the start if there's not a major cost to it. Is
there any downside to having the API support multiple contexts? I'll just make
the methods be non-static and call MakeCurrent() on each entry.

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/context.cc:73: // TODO(aa): Does this need to be called
at the start of every public method?
On 2013/12/11 06:24:22, abarth wrote:
> Probably not.  We only support on GL context right now, so you just need to
make
> it current once.

But if we had multiple contexts, we would need to right? Is there any
disadvantage to calling it on every entry now?

[abarth-chromium, 2013-12-11 16:39:14]

On 2013/12/11 07:17:06, Aaron Boodman wrote:
> I'd rather do things right from the start if there's not a major cost to it.
Is
> there any downside to having the API support multiple contexts? I'll just make
> the methods be non-static and call MakeCurrent() on each entry.
> 
>
https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
> File mojo/apps/js/bindings/gl/context.cc (right):
> 
>
https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
> mojo/apps/js/bindings/gl/context.cc:73: // TODO(aa): Does this need to be
called
> at the start of every public method?
> On 2013/12/11 06:24:22, abarth wrote:
> > Probably not.  We only support on GL context right now, so you just need to
> make
> > it current once.
> 
> But if we had multiple contexts, we would need to right? Is there any
> disadvantage to calling it on every entry now?

Just that it's slow.

Maybe we should go with the approach you've got in this CL of structuring the
API to support multiple context and just calling make current on construction.

[abarth-chromium, 2013-12-11 23:52:23]

LGTM modulo the nitpicks above.

[Aaron Boodman, 2013-12-12 19:30:29]

I considered making the functions all free, c-style, but I think there will be
need for some member functions eventually (at least gl.makeCurrent()), and
organizing it as a class is nice for consistency.

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

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/bindings/gl...
mojo/apps/js/bindings/gl/module.cc:21: const char* kModuleName =
"mojo/apps/js/bindings/gljs";
On 2013/12/11 06:24:22, abarth wrote:
> mojo/apps/js/bindings/gljs  ->  mojo/apps/js/bindings/gl ?
> 
> Normally the module name corresponds to the file path.  We'll need to figure
out
> what stable names we want for all these modules eventually.

Whoops, I initially had named all the code 'gljs', but changed my mind and
forgot this bit.

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/main.js
File mojo/apps/js/main.js (right):

https://codereview.chromium.org/111083005/diff/20001/mojo/apps/js/main.js#new...
mojo/apps/js/main.js:54: var context = new gljs.Context(encoded, width, height);
On 2013/12/11 06:24:22, abarth wrote:
> Typically folks use a variable named |gl| for the context.

okie doke

[commit-bot: I haz the power, 2013-12-12 20:28:51]

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

[commit-bot: I haz the power, 2013-12-12 20:49:08]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-13 01:38:25]

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

[commit-bot: I haz the power, 2013-12-13 01:54:28]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-13 19:38:56]

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

[Aaron Boodman, 2013-12-13 23:56:17]

Committed patchset #8 manually as r240807 (presubmit successful).