GrResource

This change adds a new parent class to vbs, ibs, textures, and rts: GrResource. The GrGpu object maintains a linked list of all the GrResources it has created so that they can be freed or abandoned by the GrContext when the GrContext is destroyed or the underlying 3D context is lost.

GrResources can outlive the GrContext if the client has kept a ref on them, but they will be marked as invalid. 

Motivations:

Enables safe shutdown of a GrContext after the 3D context has already gone away.

SkBitmaps created on a SkGpuDevice are texture backed and have a pointer to GrTexture. The SkBitmap's lifetime is independent of GrContext. When the pixels of a SkBitmap are accessed we will be able to query GrResource to know whether a readback can be performed or if the underlying texture has been freed and we must fail. (Actually adding this plumbing will be a separate change.)

[reed1, 2011-03-30 18:32:11]

LGTM

[Stephen White, 2011-03-30 18:54:14]

LG, modulo nits.

Does this need a change to skia.gyp also?

http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrGeometryBuffer.h
File gpu/include/GrGeometryBuffer.h (right):

http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrGeometryBuffer...
gpu/include/GrGeometryBuffer.h:3: 
In the future, it would be nice if you could submit the whitespace-only changes
in a separate CL, to make reviewing easier.

http://codereview.appspot.com/4328044/diff/11001/gpu/src/GrContext.cpp
File gpu/src/GrContext.cpp (right):

http://codereview.appspot.com/4328044/diff/11001/gpu/src/GrContext.cpp#newcode73
gpu/src/GrContext.cpp:73: delete fDrawBufferIBAllocPool;
Nit:  these pointers are dangling for a bit.  Could set them to NULL here.  No
big deal I guess, since setupDrawBuffer() fills them in again.  Could also
delete them in setupDrawBuffer() instead, so it's closer to the place they're
reinitialized.

[twiz1, 2011-03-30 19:10:28]

Quick drive-by.  Just a nit, and a question.

Jeff

http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp
File gpu/src/GrGLIndexBuffer.cpp (right):

http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp#n...
gpu/src/GrGLIndexBuffer.cpp:60: this->bind();
I'm curious about why you've introduced 'this' into these calls.  Are there
compiler issues that require the indirection?

http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h
File gpu/include/GrResource.h (right):

http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h#new...
gpu/include/GrResource.h:26: GrResource(GrGpu* gpu);
Small nit, but single-argument constructors should be explicit.

[reed1, 2011-03-30 20:06:46]

this->method();

This is encouraged for all skia code.

On 2011/03/30 19:10:28, twiz1 wrote:
> Quick drive-by.  Just a nit, and a question.
> 
> Jeff
> 
> http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp
> File gpu/src/GrGLIndexBuffer.cpp (right):
> 
>
http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp#n...
> gpu/src/GrGLIndexBuffer.cpp:60: this->bind();
> I'm curious about why you've introduced 'this' into these calls.  Are there
> compiler issues that require the indirection?
> 
> http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h
> File gpu/include/GrResource.h (right):
> 
>
http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h#new...
> gpu/include/GrResource.h:26: GrResource(GrGpu* gpu);
> Small nit, but single-argument constructors should be explicit.

[bsalomon, 2011-03-30 20:13:01]

Thanks for the reviews!

On 2011/03/30 18:54:14, Stephen White wrote:
> 
> Does this need a change to skia.gyp also?
yup, done, thanks

>
http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrGeometryBuffer...
> gpu/include/GrGeometryBuffer.h:3: 
> In the future, it would be nice if you could submit the whitespace-only
changes
> in a separate CL, to make reviewing easier.
Sorry about that. I'll  disable the setting in the Google XCode plugin that
removes trailing whitespace automatically on save.

>
http://codereview.appspot.com/4328044/diff/11001/gpu/src/GrContext.cpp#newcode73
> gpu/src/GrContext.cpp:73: delete fDrawBufferIBAllocPool;
> Nit:  these pointers are dangling for a bit.  Could set them to NULL here.  No
> big deal I guess, since setupDrawBuffer() fills them in again.  Could also
> delete them in setupDrawBuffer() instead, so it's closer to the place they're
> reinitialized.

They're now set to null and the setup func asserts that they are null.

-------------

On 2011/03/30 19:10:28, twiz1 wrote:
> gpu/src/GrGLIndexBuffer.cpp:60: this->bind();
> I'm curious about why you've introduced 'this' into these calls.  Are there
> compiler issues that require the indirection?

Just style changes, skia by convention uses this-> for internal method calls as
documentation. In the past I haven't been very good at following this in Gr.

> 
> http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h
> File gpu/include/GrResource.h (right):
> 
>
http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h#new...
> gpu/include/GrResource.h:26: GrResource(GrGpu* gpu);
> Small nit, but single-argument constructors should be explicit.

Done. (though does the language allow automatic cons invocation for ptrs?)

[twiz1, 2011-03-30 20:21:24]

On 2011/03/30 20:06:46, reed1 wrote:
> this->method();
> 
> This is encouraged for all skia code.

I just added a mention of this style to the Skia style-guide.

> 
> On 2011/03/30 19:10:28, twiz1 wrote:
> > Quick drive-by.  Just a nit, and a question.
> > 
> > Jeff
> > 
> > http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp
> > File gpu/src/GrGLIndexBuffer.cpp (right):
> > 
> >
>
http://codereview.appspot.com/4328044/diff/3029/gpu/src/GrGLIndexBuffer.cpp#n...
> > gpu/src/GrGLIndexBuffer.cpp:60: this->bind();
> > I'm curious about why you've introduced 'this' into these calls.  Are there
> > compiler issues that require the indirection?
> > 
> > http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h
> > File gpu/include/GrResource.h (right):
> > 
> >
>
http://codereview.appspot.com/4328044/diff/11001/gpu/include/GrResource.h#new...
> > gpu/include/GrResource.h:26: GrResource(GrGpu* gpu);
> > Small nit, but single-argument constructors should be explicit.

[bsalomon, 2011-03-30 21:27:59]

checked in as r1026, closed.