Expose MGL interface through NaCl IRT

Expose MGL interface through NaCl IRT

To enable the use of OpenGL from Mojo NaCl applications, we need to
expose the MGL interface through Mojo NaCl IRT.

BUG= https://github.com/domokit/mojo/issues/396
TEST= spinning_cube example
R=mseaborn@chromium.org, smklein@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/ebf1b839542da3524c7b9c6a8b28809f36641fad

[Mark Seaborn, 2015-10-17 01:08:08]

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
File mojo/public/platform/nacl/libmojo.cc (right):

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:228: bool g_irt_mgl_valid = false;
I realise you are copying the existing pattern for various parts here, but...

These variables and get_irt_mgl() should go in an anon namespace.

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:231: struct nacl_irt_mgl* get_irt_mgl() {
Chromium naming style would be GetIrtMgl()

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:237: else
Nit: You could drop the 'else' since this is after a return in the 'if' block

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
mojo/public/platform/nacl/mojo_irt.h:93: MGLContext
(*MGLCreateContext)(MGLOpenGLAPIVersion version,
Can you add comments to document what these functions do, or point to
documentation elsewhere?  How do these functions handle errors?

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
mojo/public/platform/nacl/mojo_irt.h:108: void (*MGLResizeSurface)(uint32_t
width, uint32_t height);
Same comment about docs...

What surface does this operate on?  What does MGLSwapBuffers() operate on?  It
looks like these operate on global state.

[Petr Hosek, 2015-10-17 01:51:14]

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
File mojo/public/platform/nacl/libmojo.cc (right):

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:228: bool g_irt_mgl_valid = false;
On 2015/10/17 01:08:08, Mark Seaborn wrote:
> I realise you are copying the existing pattern for various parts here, but...
> 
> These variables and get_irt_mgl() should go in an anon namespace.

Done.

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:231: struct nacl_irt_mgl* get_irt_mgl() {
On 2015/10/17 01:08:08, Mark Seaborn wrote:
> Chromium naming style would be GetIrtMgl()

Done.

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/l...
mojo/public/platform/nacl/libmojo.cc:237: else
On 2015/10/17 01:08:08, Mark Seaborn wrote:
> Nit: You could drop the 'else' since this is after a return in the 'if' block

Done.

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
mojo/public/platform/nacl/mojo_irt.h:93: MGLContext
(*MGLCreateContext)(MGLOpenGLAPIVersion version,
On 2015/10/17 01:08:08, Mark Seaborn wrote:
> Can you add comments to document what these functions do, or point to
> documentation elsewhere?  How do these functions handle errors?

Done.

https://codereview.chromium.org/1413683003/diff/1/mojo/public/platform/nacl/m...
mojo/public/platform/nacl/mojo_irt.h:108: void (*MGLResizeSurface)(uint32_t
width, uint32_t height);
On 2015/10/17 01:08:08, Mark Seaborn wrote:
> Same comment about docs...
> 
> What surface does this operate on?  What does MGLSwapBuffers() operate on?  It
> looks like these operate on global state.

Done.

[Sean Klein, 2015-10-17 02:03:14]

https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
File mojo/public/platform/nacl/libmojo.cc (right):

https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
mojo/public/platform/nacl/libmojo.cc:232: 
Is there a reason why you made a bunch of anonymous namespaces, rather than just
one?

[Petr Hosek, 2015-10-17 02:04:48]

On 2015/10/17 02:03:14, smklein1 wrote:
>
https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
> File mojo/public/platform/nacl/libmojo.cc (right):
> 
>
https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
> mojo/public/platform/nacl/libmojo.cc:232: 
> Is there a reason why you made a bunch of anonymous namespaces, rather than
just
> one?

Not really, just to keep those individual interface implementations separated,
but I'm fine to move them to a single namespace if you prefer.

[Sean Klein, 2015-10-17 02:12:35]

On 2015/10/17 02:04:48, Petr Hosek wrote:
> On 2015/10/17 02:03:14, smklein1 wrote:
> >
>
https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
> > File mojo/public/platform/nacl/libmojo.cc (right):
> > 
> >
>
https://codereview.chromium.org/1413683003/diff/40001/mojo/public/platform/na...
> > mojo/public/platform/nacl/libmojo.cc:232: 
> > Is there a reason why you made a bunch of anonymous namespaces, rather than
> just
> > one?
> 
> Not really, just to keep those individual interface implementations separated,
> but I'm fine to move them to a single namespace if you prefer.

Well, if it's all in the same file anyway, it's not like it would protect
against link time naming conflicts. The google C++ style guide doesn't mention
anything about multiple anonymous namespaces, but (personally) I think it would
look cleaner with just one. I'll defer to your judgment.

[Petr Hosek, 2015-10-20 00:11:32]

https://chromiumcodereview.appspot.com/1413683003/diff/40001/mojo/public/plat...
File mojo/public/platform/nacl/libmojo.cc (right):

https://chromiumcodereview.appspot.com/1413683003/diff/40001/mojo/public/plat...
mojo/public/platform/nacl/libmojo.cc:232: 
On 2015/10/17 02:03:14, smklein1 wrote:
> Is there a reason why you made a bunch of anonymous namespaces, rather than
just
> one?

Done.

[Mark Seaborn, 2015-10-20 01:24:32]

LGTM.  Can you add a TEST= field to say how you manually tested this, please? 
Can you add some automated test coverage in a follow-up change?

https://chromiumcodereview.appspot.com/1413683003/diff/1/mojo/public/platform...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://chromiumcodereview.appspot.com/1413683003/diff/1/mojo/public/platform...
mojo/public/platform/nacl/mojo_irt.h:93: MGLContext
(*MGLCreateContext)(MGLOpenGLAPIVersion version,
On 2015/10/17 01:51:13, Petr Hosek wrote:
> On 2015/10/17 01:08:08, Mark Seaborn wrote:
> > Can you add comments to document what these functions do, or point to
> > documentation elsewhere?  How do these functions handle errors?
> 
> Done.

I suppose I should have put the emphasis on "point to documentation elsewhere".
:-)  The new version duplicates comments, which isn't great, because they will
inevitably get out of sync.  Can you point to the existing comments instead?

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
mojo/public/platform/nacl/mojo_irt.h:122: // Returns GL function usable in any
context that advertise the corresponding
"advertises"

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
mojo/public/platform/nacl/mojo_irt.h:133: // Resizes the default framebuffer for
the currently bound onscreen
Aside: For the record, having a "currently bound" thing for this is really icky,
so I hope this gets revisited before it gets deployed.

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
mojo/public/platform/nacl/mojo_irt.h:146: // invokes |callback| when the service
side acknowleges that the sync point
"acknowledges"

[Petr Hosek, 2015-10-20 04:44:09]

Description was changed from

==========
Expose MGL interface through NaCl IRT

To enable the use of OpenGL from Mojo NaCl applications, we need to
expose the MGL interface through Mojo NaCl IRT.

BUG= https://github.com/domokit/mojo/issues/396
R= smklein@chromium.org, mseaborn@chromium.org
==========

to

==========
Expose MGL interface through NaCl IRT

To enable the use of OpenGL from Mojo NaCl applications, we need to
expose the MGL interface through Mojo NaCl IRT.

BUG= https://github.com/domokit/mojo/issues/396
TEST= spinning_cube example
R= smklein@chromium.org, mseaborn@chromium.org
==========

[Mark Seaborn, 2015-10-20 16:26:07]

https://codereview.chromium.org/1413683003/diff/60001/mojo/public/platform/na...
File mojo/public/platform/nacl/libmojo.cc (right):

https://codereview.chromium.org/1413683003/diff/60001/mojo/public/platform/na...
mojo/public/platform/nacl/libmojo.cc:5: // WARNING this file was generated by
generate_nacl_bindings.py
Oh, I just noticed this comment: "Do not edit by hand."

OK, don't commit this as-is. :-)  Can you put this GL stuff into a file that's
separate from this libmojo.cc, please?

[Mark Seaborn, 2015-10-20 17:24:29]

https://codereview.chromium.org/1413683003/diff/60001/mojo/public/platform/na...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://codereview.chromium.org/1413683003/diff/60001/mojo/public/platform/na...
mojo/public/platform/nacl/mojo_irt.h:5: // WARNING this file was generated by
generate_nacl_bindings.py
Same with this file -- it's also generated.  Let's put the IRT interfaces for GL
in a separate file.

[Petr Hosek, 2015-10-20 21:38:31]

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
File mojo/public/platform/nacl/libmojo.cc (right):

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
mojo/public/platform/nacl/libmojo.cc:5: // WARNING this file was generated by
generate_nacl_bindings.py
On 2015/10/20 16:26:07, Mark Seaborn wrote:
> Oh, I just noticed this comment: "Do not edit by hand."
> 
> OK, don't commit this as-is. :-)  Can you put this GL stuff into a file that's
> separate from this libmojo.cc, please?

Done.

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
File mojo/public/platform/nacl/mojo_irt.h (right):

https://chromiumcodereview.appspot.com/1413683003/diff/60001/mojo/public/plat...
mojo/public/platform/nacl/mojo_irt.h:5: // WARNING this file was generated by
generate_nacl_bindings.py
On 2015/10/20 17:24:29, Mark Seaborn wrote:
> Same with this file -- it's also generated.  Let's put the IRT interfaces for
GL
> in a separate file.

Done.

[Mark Seaborn, 2015-10-21 04:55:50]

LGTM

https://chromiumcodereview.appspot.com/1413683003/diff/80001/mojo/public/plat...
File mojo/public/platform/nacl/mgl_irt.cc (right):

https://chromiumcodereview.appspot.com/1413683003/diff/80001/mojo/public/plat...
mojo/public/platform/nacl/mgl_irt.cc:18: return NULL;
This can become 'nullptr' in Chromium-style code

https://chromiumcodereview.appspot.com/1413683003/diff/80001/mojo/public/plat...
mojo/public/platform/nacl/mgl_irt.cc:63: if (irt_mgl == NULL)
In Chromium-style code you can just do "if (!irt_mgl)"

And similarly, "if (irt_mgl)" below.

https://chromiumcodereview.appspot.com/1413683003/diff/80001/mojo/public/plat...
File mojo/public/platform/nacl/mgl_irt.h (right):

https://chromiumcodereview.appspot.com/1413683003/diff/80001/mojo/public/plat...
mojo/public/platform/nacl/mgl_irt.h:12: 
Nit: Chromium style is only 1 empty line between top-level things.  Same below.

[Petr Hosek, 2015-10-21 16:33:21]

https://codereview.chromium.org/1413683003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/nacl/mgl_irt.cc (right):

https://codereview.chromium.org/1413683003/diff/80001/mojo/public/platform/na...
mojo/public/platform/nacl/mgl_irt.cc:18: return NULL;
On 2015/10/21 04:55:49, Mark Seaborn wrote:
> This can become 'nullptr' in Chromium-style code

Done.

https://codereview.chromium.org/1413683003/diff/80001/mojo/public/platform/na...
mojo/public/platform/nacl/mgl_irt.cc:63: if (irt_mgl == NULL)
On 2015/10/21 04:55:49, Mark Seaborn wrote:
> In Chromium-style code you can just do "if (!irt_mgl)"
> 
> And similarly, "if (irt_mgl)" below.

Done.

https://codereview.chromium.org/1413683003/diff/80001/mojo/public/platform/na...
File mojo/public/platform/nacl/mgl_irt.h (right):

https://codereview.chromium.org/1413683003/diff/80001/mojo/public/platform/na...
mojo/public/platform/nacl/mgl_irt.h:12: 
On 2015/10/21 04:55:49, Mark Seaborn wrote:
> Nit: Chromium style is only 1 empty line between top-level things.  Same
below.

Done.

[Petr Hosek, 2015-10-21 18:42:11]

Committed patchset #6 (id:100001) manually as
ebf1b839542da3524c7b9c6a8b28809f36641fad (presubmit successful).