Fix for GrTextureStripAtlas memory leak

This should fix http://code.google.com/p/chromium/issues/detail?id=151017

[robertphillips, 2012-09-21 18:24:08]

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
src/gpu/effects/GrTextureStripAtlas.cpp:70: AtlasEntry** temp =
gAtlasEntries.push();
With the SkTDArray storing the AtlasEntry's they would move around when the
array reallocated. This caused problems in the GrTHashTable since the pointer is
had became invalid.

[bsalomon, 2012-09-24 13:26:34]

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

http://codereview.appspot.com/6549050/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:65: typedef void (*PFCleanUpFunc)(const GrContext*
context);
I think these types of callbacks usually allow the client to specify a ptr-sized
value to be passed to the function. E.g.:

typedef void (*PFCleanUpFunc)(const GrContext* context, void* info);

http://codereview.appspot.com/6549050/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:71: static void AddCleanUp(PFCleanUpFunc cleanUp) {
Why static rather than just registering with each context you care about?

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
src/gpu/effects/GrTextureStripAtlas.cpp:70: AtlasEntry** temp =
gAtlasEntries.push();
On 2012/09/21 18:24:09, robertphillips wrote:
> With the SkTDArray storing the AtlasEntry's they would move around when the
> array reallocated. This caused problems in the GrTHashTable since the pointer
is
> had became invalid.

Would it make it simpler to have the atlas ptr come back with the cleanup
function call (e.g. use an "info" param).

[robertphillips, 2012-09-24 16:29:34]

Much cleaner now. Note that 'gAtlasEntries' has entirely gone away.

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

http://codereview.appspot.com/6549050/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:65: typedef void (*PFCleanUpFunc)(const GrContext*
context);
On 2012/09/24 13:26:34, bsalomon wrote:
> I think these types of callbacks usually allow the client to specify a
ptr-sized
> value to be passed to the function. E.g.:
> 
> typedef void (*PFCleanUpFunc)(const GrContext* context, void* info);

Done.

http://codereview.appspot.com/6549050/diff/2001/include/gpu/GrContext.h#newco...
include/gpu/GrContext.h:71: static void AddCleanUp(PFCleanUpFunc cleanUp) {
On 2012/09/24 13:26:34, bsalomon wrote:
> Why static rather than just registering with each context you care about?

Done.

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

http://codereview.appspot.com/6549050/diff/2001/src/gpu/effects/GrTextureStri...
src/gpu/effects/GrTextureStripAtlas.cpp:70: AtlasEntry** temp =
gAtlasEntries.push();
Yep.

[bsalomon, 2012-09-24 16:42:55]

Cool, it seems much simpler now. LGTM except for Q about static init.

https://codereview.appspot.com/6549050/diff/1007/src/gpu/effects/GrTextureStr...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.appspot.com/6549050/diff/1007/src/gpu/effects/GrTextureStr...
src/gpu/effects/GrTextureStripAtlas.cpp:22:
GrTHashTable<GrTextureStripAtlas::AtlasEntry,
Will this violate our no-static-initializer policy in chrome? Maybe this needs
to be accessed in a static function so it can be created at first access?

[robertphillips, 2012-09-24 18:07:35]

https://codereview.appspot.com/6549050/diff/1007/src/gpu/effects/GrTextureStr...
File src/gpu/effects/GrTextureStripAtlas.cpp (right):

https://codereview.appspot.com/6549050/diff/1007/src/gpu/effects/GrTextureStr...
src/gpu/effects/GrTextureStripAtlas.cpp:22:
GrTHashTable<GrTextureStripAtlas::AtlasEntry,
I believe the new approach is Chrome-acceptable (given
http://neugierig.org/software/chromium/notes/2011/08/static-initializers.html).
The issue is that I don't want to "leak" the memory allocated by the cache even
if it is still "reachable".

[robertphillips, 2012-09-24 18:15:34]

Note that - creating the GrTHashTable in the GetCache method causes problems
since (at least for the tests app) the GrTHashTable will be freed before the
GrContext and seg faults will ensue.

[bsalomon, 2012-09-24 19:11:58]

LGTM

[robertphillips, 2012-09-24 19:34:28]

committed as r5648