ref counted GrGLInterface

[bsalomon, 2011-08-18 14:11:54]

Hi Jeff, We will soon have different GrContexts using different GL
implementations in the same process so we need to move away from the global
interface ptr. I know this review is about as much fun as long division :(

[twiz, 2011-08-18 15:58:12]

On 2011/08/18 14:11:54, bsalomon wrote:
> Hi Jeff, We will soon have different GrContexts using different GL
> implementations in the same process so we need to move away from the global
> interface ptr. I know this review is about as much fun as long division :(

Can you update me on why we will need different GL back-ends in the same
process?  

Having a look . . .

[twiz, 2011-08-18 18:48:19]

LGTM.

Just a few NITS.  

I think that the current GL_CALL macro definition is fine.  Declaring a macro in
a header file (GrResource.h) doesn't feel as clean as the current system of
separate declarations.

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

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrGLInterface.h#n...
gpu/include/GrGLInterface.h:233: struct GrGLInterface : GrRefCnt {
Nit:  Most of the code in skia uses the public keyword to specify inheritance.

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrTypes.h
File gpu/include/GrTypes.h (left):

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrTypes.h#oldcode176
gpu/include/GrTypes.h:176: kDirect3D9_GrEngine
Just cleaning up some unused values?

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp
File gpu/src/GrGLStencilBuffer.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp...
gpu/src/GrGLStencilBuffer.cpp:8: 
Nit:  Style among other skia files is 2 lines (or 3 in some cases).

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp...
gpu/src/GrGLStencilBuffer.cpp:21: return (size_t)(size / 8);
Nit:  Convert to static_cast?

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLTexture.cpp
File gpu/src/GrGLTexture.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLTexture.cpp#newco...
gpu/src/GrGLTexture.cpp:16: #define GL_CALL(X) GR_GL_CALL(GPUGL->glInterface(),
X)
It's a shame that you have to re-declare this everywhere.  Is there a way that
it could be declared in a single location?

Could this be declared in GrResource.h?

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp#newc...
gpu/src/GrGpuFactory.cpp:29: glInterface = (const GrGLInterface*) context3D;
Nit:  Use static_cast here.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:64: : fGL(gl)
The order of initializers should be the same as the order of declarations.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:258: }
Namespaces do not introduce an indentation scope.  (Or is there no standard on
this?  After checking I've seen both styles in the skia source.)

http://codereview.appspot.com/4901046/diff/8002/gpu/src/android/GrGLDefaultIn...
File gpu/src/android/GrGLDefaultInterface_android.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/android/GrGLDefaultIn...
gpu/src/android/GrGLDefaultInterface_android.cpp:18: */
Should these old includes also be stripped?

[bsalomon, 2011-08-18 19:50:45]

Thanks for the thorough review! I made changes related to all the comments
except GL_CALL.

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

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrGLInterface.h#n...
gpu/include/GrGLInterface.h:233: struct GrGLInterface : GrRefCnt {
On 2011/08/18 18:48:19, twiz wrote:
> Nit:  Most of the code in skia uses the public keyword to specify inheritance.

Done.

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrTypes.h
File gpu/include/GrTypes.h (left):

http://codereview.appspot.com/4901046/diff/8002/gpu/include/GrTypes.h#oldcode176
gpu/include/GrTypes.h:176: kDirect3D9_GrEngine
On 2011/08/18 18:48:19, twiz wrote:
> Just cleaning up some unused values?
Yes, happened to notice it when doing the below mod.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp
File gpu/src/GrGLStencilBuffer.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp...
gpu/src/GrGLStencilBuffer.cpp:8: 
On 2011/08/18 18:48:19, twiz wrote:
> Nit:  Style among other skia files is 2 lines (or 3 in some cases).

Done.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLStencilBuffer.cpp...
gpu/src/GrGLStencilBuffer.cpp:21: return (size_t)(size / 8);
On 2011/08/18 18:48:19, twiz wrote:
> Nit:  Convert to static_cast?

Done.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLTexture.cpp
File gpu/src/GrGLTexture.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGLTexture.cpp#newco...
gpu/src/GrGLTexture.cpp:16: #define GL_CALL(X) GR_GL_CALL(GPUGL->glInterface(),
X)
On 2011/08/18 18:48:19, twiz wrote:
> It's a shame that you have to re-declare this everywhere.  Is there a way that
> it could be declared in a single location?
> 
> Could this be declared in GrResource.h?
Yeah I'm not crazy about redeclaring it but GrResource isn't GL-specific. There
isn't an appropriate GL-specific resource file for these to go in, and it
doesn't feel right to create a file just for these macros.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp#newc...
gpu/src/GrGpuFactory.cpp:29: glInterface = (const GrGLInterface*) context3D;
On 2011/08/18 18:48:19, twiz wrote:
> Nit:  Use static_cast here.
I think reinterpret_cast is the right one here(?)

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp
File gpu/src/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:64: : fGL(gl)
On 2011/08/18 18:48:19, twiz wrote:
> The order of initializers should be the same as the order of declarations.  

Done.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuGLShaders.cpp#ne...
gpu/src/GrGpuGLShaders.cpp:258: }
On 2011/08/18 18:48:19, twiz wrote:
> Namespaces do not introduce an indentation scope.  (Or is there no standard on
> this?  After checking I've seen both styles in the skia source.)

Done.

http://codereview.appspot.com/4901046/diff/8002/gpu/src/android/GrGLDefaultIn...
File gpu/src/android/GrGLDefaultInterface_android.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/android/GrGLDefaultIn...
gpu/src/android/GrGLDefaultInterface_android.cpp:18: */
On 2011/08/18 18:48:19, twiz wrote:
> Should these old includes also be stripped?

Done.

[twiz, 2011-08-18 20:06:42]

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp
File gpu/src/GrGpuFactory.cpp (right):

http://codereview.appspot.com/4901046/diff/8002/gpu/src/GrGpuFactory.cpp#newc...
gpu/src/GrGpuFactory.cpp:29: glInterface = (const GrGLInterface*) context3D;
On 2011/08/18 19:50:47, bsalomon wrote:
> On 2011/08/18 18:48:19, twiz wrote:
> > Nit:  Use static_cast here.
> I think reinterpret_cast is the right one here(?)

Yes, you're right.  A reinterpret_cast is required to go from an intptr_t to a
GrGLInterface*.

[bsalomon, 2011-08-19 17:17:02]

Closed with r2140.