Add GetProcAddress to OpenGL control interface

Add GetProcAddress to OpenGL control interface

R=jamesr@chromium.org
BUG=

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

[Petr Hosek, 2015-09-20 20:33:15]

Patchset #1 (id:1) has been deleted

[Petr Hosek, 2015-09-21 19:22:31]

ptal

[jamesr, 2015-09-21 19:38:50]

https://codereview.chromium.org/1354353002/diff/20001/mojo/gles2/control_thun...
File mojo/gles2/control_thunks_impl.cc (right):

https://codereview.chromium.org/1354353002/diff/20001/mojo/gles2/control_thun...
mojo/gles2/control_thunks_impl.cc:57: return gles2::GetGLFunctionPointer(name);
this will return things that the underlying library exposes but that we've
chosen not to expose through our thunks.  I don't think we want to do that -
it's too broad and it makes it hard to know what is exposed and what's not.
instead, we should only expose the functions that exist in the autogen headers
used when building the system.  In practice this means going through the autogen
tables to map names to exposed functions and return those. this could just be a
big chain of strncmp()s driven by the autogen header's macros for now and we can
make it a more efficient lookup later if we want (or not).

https://codereview.chromium.org/1354353002/diff/20001/mojo/public/c/gpu/MGL/m...
File mojo/public/c/gpu/MGL/mgl.h (right):

https://codereview.chromium.org/1354353002/diff/20001/mojo/public/c/gpu/MGL/m...
mojo/public/c/gpu/MGL/mgl.h:58: typedef void (*MGLFunctionPointerType)(void);
this should go in mgl_types.h and have a bit of guidance about how to handle the
value (i.e. cast to appropriate function type).  do we know what calling
convention we want people to use?

https://codereview.chromium.org/1354353002/diff/20001/mojo/public/c/gpu/MGL/m...
mojo/public/c/gpu/MGL/mgl.h:60: MGLFunctionPointerType MGLGetProcAddress(const
char* procname);
this should be documented to say what it does, what the preconditions are and
what the restrictions on the returned value are. i think we should say that this
returns null for functions that the implementation does not support, is
independent of the currently current context, returns functions usable in any
context that advertise the corresponding extension in their GL_EXTENSIONS string
and that this only returns GL functions (i.e. it doesn't advertise MGL
functions).

[Petr Hosek, 2015-09-22 20:06:54]

https://chromiumcodereview.appspot.com/1354353002/diff/20001/mojo/gles2/contr...
File mojo/gles2/control_thunks_impl.cc (right):

https://chromiumcodereview.appspot.com/1354353002/diff/20001/mojo/gles2/contr...
mojo/gles2/control_thunks_impl.cc:57: return gles2::GetGLFunctionPointer(name);
On 2015/09/21 19:38:50, jamesr wrote:
> this will return things that the underlying library exposes but that we've
> chosen not to expose through our thunks.  I don't think we want to do that -
> it's too broad and it makes it hard to know what is exposed and what's not.
> instead, we should only expose the functions that exist in the autogen headers
> used when building the system.  In practice this means going through the
autogen
> tables to map names to exposed functions and return those. this could just be
a
> big chain of strncmp()s driven by the autogen header's macros for now and we
can
> make it a more efficient lookup later if we want (or not).

Done.

https://chromiumcodereview.appspot.com/1354353002/diff/20001/mojo/public/c/gp...
File mojo/public/c/gpu/MGL/mgl.h (right):

https://chromiumcodereview.appspot.com/1354353002/diff/20001/mojo/public/c/gp...
mojo/public/c/gpu/MGL/mgl.h:58: typedef void (*MGLFunctionPointerType)(void);
On 2015/09/21 19:38:50, jamesr wrote:
> this should go in mgl_types.h and have a bit of guidance about how to handle
the
> value (i.e. cast to appropriate function type).  do we know what calling
> convention we want people to use?

I've decided to use `void *` instead which is more akin to other
`GetProcAddress` implementations do, but I'm fine to change it back to a custom
type if you prefer.

https://chromiumcodereview.appspot.com/1354353002/diff/20001/mojo/public/c/gp...
mojo/public/c/gpu/MGL/mgl.h:60: MGLFunctionPointerType MGLGetProcAddress(const
char* procname);
On 2015/09/21 19:38:50, jamesr wrote:
> this should be documented to say what it does, what the preconditions are and
> what the restrictions on the returned value are. i think we should say that
this
> returns null for functions that the implementation does not support, is
> independent of the currently current context, returns functions usable in any
> context that advertise the corresponding extension in their GL_EXTENSIONS
string
> and that this only returns GL functions (i.e. it doesn't advertise MGL
> functions).

Done.

[jamesr, 2015-09-22 20:28:35]

lgtm

It'd be nice to have a test that verifies that calling
MGLGetProcAddress("glActiveTexture") returns non-null and MGLGetProcAddress("")
/ MGLGetProcAddress("gibberish") do not.

https://codereview.chromium.org/1354353002/diff/40001/mojo/public/c/gpu/MGL/m...
File mojo/public/c/gpu/MGL/mgl.h (right):

https://codereview.chromium.org/1354353002/diff/40001/mojo/public/c/gpu/MGL/m...
mojo/public/c/gpu/MGL/mgl.h:62: // and calling convention before use.
EGL has a type for the returned type that tries to produce more useful compile
error messages if developers forget to cast the return type before using:

/* This is a generic function pointer type, whose name indicates it must
 * be cast to the proper type *and calling convention* before use.
 */
typedef void (*__eglMustCastToProperFunctionPointerType)(void);

/* Now, define eglGetProcAddress using the generic function ptr. type */
EGLAPI __eglMustCastToProperFunctionPointerType EGLAPIENTRY
       eglGetProcAddress(const char *procname);


I don't know how useful this is in practice

https://codereview.chromium.org/1354353002/diff/40001/mojo/public/platform/na...
File mojo/public/platform/native/gles2/call_visitor.h (right):

https://codereview.chromium.org/1354353002/diff/40001/mojo/public/platform/na...
mojo/public/platform/native/gles2/call_visitor.h:1: #include
"mojo/public/platform/native/gles2/call_visitor_autogen.h"
dumb as it is this should probably retain a copyright header. this should also
have a comment indicating that it intentionally does not have a header guard as
some enterprising engineer may be tempted to add one and break things in very
hard-to-understand ways.

[Petr Hosek, 2015-09-22 23:31:26]

I added a unit test and fixed a bug in the implementation it revealed.

https://chromiumcodereview.appspot.com/1354353002/diff/40001/mojo/public/c/gp...
File mojo/public/c/gpu/MGL/mgl.h (right):

https://chromiumcodereview.appspot.com/1354353002/diff/40001/mojo/public/c/gp...
mojo/public/c/gpu/MGL/mgl.h:62: // and calling convention before use.
On 2015/09/22 20:28:35, jamesr wrote:
> EGL has a type for the returned type that tries to produce more useful compile
> error messages if developers forget to cast the return type before using:
> 
> /* This is a generic function pointer type, whose name indicates it must
>  * be cast to the proper type *and calling convention* before use.
>  */
> typedef void (*__eglMustCastToProperFunctionPointerType)(void);
> 
> /* Now, define eglGetProcAddress using the generic function ptr. type */
> EGLAPI __eglMustCastToProperFunctionPointerType EGLAPIENTRY
>        eglGetProcAddress(const char *procname);
> 
> 
> I don't know how useful this is in practice

I guess it might be useful in some cases, I changed the return type back to a
typedef.

https://chromiumcodereview.appspot.com/1354353002/diff/40001/mojo/public/plat...
File mojo/public/platform/native/gles2/call_visitor.h (right):

https://chromiumcodereview.appspot.com/1354353002/diff/40001/mojo/public/plat...
mojo/public/platform/native/gles2/call_visitor.h:1: #include
"mojo/public/platform/native/gles2/call_visitor_autogen.h"
On 2015/09/22 20:28:35, jamesr wrote:
> dumb as it is this should probably retain a copyright header. this should also
> have a comment indicating that it intentionally does not have a header guard
as
> some enterprising engineer may be tempted to add one and break things in very
> hard-to-understand ways.

Done.

[jamesr, 2015-09-22 23:38:50]

Pesky tests, finding bugs and all.  +vtl for question about the test harness

https://codereview.chromium.org/1354353002/diff/60001/mojo/gles2/BUILD.gn
File mojo/gles2/BUILD.gn (right):

https://codereview.chromium.org/1354353002/diff/60001/mojo/gles2/BUILD.gn#new...
mojo/gles2/BUILD.gn:98: "../edk/test/run_all_unittests.cc",
sourcing in ../xxx files is dodgy. if this file is supposed to be reusable then
//mojo/edk/test/.. should expose a source_set that contains it and has all of
the deps for the file itself. i'm not sure offhand how reusable this file is
supposed to be - maybe check with trung?

[Petr Hosek, 2015-09-22 23:44:56]

phosek@chromium.org changed reviewers:
+ vtl@google.com

[Petr Hosek, 2015-09-22 23:45:37]

phosek@chromium.org changed reviewers:
+ viettrungluu@chromium.org
- vtl@google.com

[Petr Hosek, 2015-09-22 23:48:54]

phosek@chromium.org changed reviewers:
- viettrungluu@chromium.org

[Petr Hosek, 2015-09-23 00:04:31]

https://chromiumcodereview.appspot.com/1354353002/diff/60001/mojo/gles2/BUILD.gn
File mojo/gles2/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1354353002/diff/60001/mojo/gles2/BUILD...
mojo/gles2/BUILD.gn:98: "../edk/test/run_all_unittests.cc",
On 2015/09/22 23:38:50, jamesr wrote:
> sourcing in ../xxx files is dodgy. if this file is supposed to be reusable
then
> //mojo/edk/test/.. should expose a source_set that contains it and has all of
> the deps for the file itself. i'm not sure offhand how reusable this file is
> supposed to be - maybe check with trung?

Turned out there is a target for that file, so this isn't needed and I can just
use dependency.

[jamesr, 2015-09-23 00:09:36]

lgtm

[Petr Hosek, 2015-09-23 00:45:52]

Committed patchset #4 (id:80001) manually as
c7c7906e87cd6a5429a8ad97af0206c4bb4210bf (presubmit successful).