Add GrGLContextInfo

Comitted at r3162

[bsalomon, 2012-02-10 18:50:15]

Today we pass around some subset of GrGLInterface, GLSLGeneration, GrGLVersion,
GrGLBinding. We've also lost the notion that a single GrGLInterface could
support multiple bindings.

The idea of this change is to encapsulate the various aspects of a GL context in
a single object that can be passed around. This change doesn't push the new type
out to all potential users yet (e.g. bowels of the shader generator).

It centralizes where the binding type (desktop vs es2) of the GL context is
determined and does so using the OpenGL version string rather than assuming only
one bit of GrGLInterface::fBindingsExported is set. (There are some lingering
instances of using fBindingsExported in the shader generator that will be
addressed in a subsequent change).

[TomH, 2012-02-10 19:25:12]

Mostly LGTM.

http://codereview.appspot.com/5653060/diff/2001/include/gpu/GrGLInterface.h
File include/gpu/GrGLInterface.h (right):

http://codereview.appspot.com/5653060/diff/2001/include/gpu/GrGLInterface.h#n...
include/gpu/GrGLInterface.h:25: * A GrGLInterface (defined below) may support
multiple bindings.
Why would we want to?

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.cpp
File src/gpu/GrGLContextInfo.cpp (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.cpp#n...
src/gpu/GrGLContextInfo.cpp:41: // function pointer
I assume you have a principled reason for not wanting to do early out?

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h
File src/gpu/GrGLContextInfo.h (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:20: * Creates a GrGLContext info from a GrGLInterface
and the currently
Typo: GrGLContextInfo

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:39: * bound OpenGL context accessible by the
GrGLInterface.
I assume this is necessary iff you've called the default ctor?

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:54: return GrGLHasExtensionFromString(ext,
fExtensionString.c_str());
Not safe if fExtensionString is NULL, which looks like it can happen between
construction and initialization?

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp
File src/gpu/GrGLInterface.cpp (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp#new...
src/gpu/GrGLInterface.cpp:142: fBindingsExported = kNone_GrGLBinding;
Yay

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp#new...
src/gpu/GrGLInterface.cpp:269: // We should never succeed if binding is kNone
Nit: Comment doesn't quite describe what's being done in the following two
lines?

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGpuGL.h
File src/gpu/GrGpuGL.h (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGpuGL.h#newcode36
src/gpu/GrGpuGL.h:36: 
If we're returning all the fields of GrGLContextInfo individually (which is
allegedly good from a reducing-coupling point of view), must we also return the
GrGLContextInfo itself?
(Guess: so we can pass it to other functions/objects that need it?)

[bsalomon, 2012-02-10 19:48:53]

http://codereview.appspot.com/5653060/diff/2001/include/gpu/GrGLInterface.h
File include/gpu/GrGLInterface.h (right):

http://codereview.appspot.com/5653060/diff/2001/include/gpu/GrGLInterface.h#n...
include/gpu/GrGLInterface.h:25: * A GrGLInterface (defined below) may support
multiple bindings.
On 2012/02/10 19:25:13, TomH wrote:
> Why would we want to?
Some desktop systems allow you to create an ES2 context. So you could
potentially have a single GrGLInterface that can be used with both desktop and
ES2 contexts.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.cpp
File src/gpu/GrGLContextInfo.cpp (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.cpp#n...
src/gpu/GrGLContextInfo.cpp:41: // function pointer
On 2012/02/10 19:25:13, TomH wrote:
> I assume you have a principled reason for not wanting to do early out?

no :)

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h
File src/gpu/GrGLContextInfo.h (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:20: * Creates a GrGLContext info from a GrGLInterface
and the currently
On 2012/02/10 19:25:13, TomH wrote:
> Typo: GrGLContextInfo

Done.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:39: * bound OpenGL context accessible by the
GrGLInterface.
On 2012/02/10 19:25:13, TomH wrote:
> I assume this is necessary iff you've called the default ctor?

In theory you could call this on an already initialized GrGLContextInfo to
initialize it to another context, though no code path does this today. The code
correctly unrefs the previous fInterface value.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLContextInfo.h#new...
src/gpu/GrGLContextInfo.h:54: return GrGLHasExtensionFromString(ext,
fExtensionString.c_str());
On 2012/02/10 19:25:13, TomH wrote:
> Not safe if fExtensionString is NULL, which looks like it can happen between
> construction and initialization?

true, nice catch.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp
File src/gpu/GrGLInterface.cpp (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp#new...
src/gpu/GrGLInterface.cpp:49: }
Yikes I missed a return at the end here. Added in updated patch.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGLInterface.cpp#new...
src/gpu/GrGLInterface.cpp:269: // We should never succeed if binding is kNone
On 2012/02/10 19:25:13, TomH wrote:
> Nit: Comment doesn't quite describe what's being done in the following two
> lines?

Done.

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGpuGL.h
File src/gpu/GrGpuGL.h (right):

http://codereview.appspot.com/5653060/diff/2001/src/gpu/GrGpuGL.h#newcode36
src/gpu/GrGpuGL.h:36: 
On 2012/02/10 19:25:13, TomH wrote:
> If we're returning all the fields of GrGLContextInfo individually (which is
> allegedly good from a reducing-coupling point of view), must we also return
the
> GrGLContextInfo itself?
> (Guess: so we can pass it to other functions/objects that need it?)

I suppose we don't need the getContextInfo(). Originally I kept the individual
getters around in order to keep the size of this change down. But for decoupling
and line-length reasons it probably makes some sense to keep the getters. I
deleted glContextInfo() in the updated patch.

[TomH, 2012-02-10 19:55:06]

Well, then, entirely LGTM. :)