Proactive SkPixelRef invalidation

We want to give SkPixelRef a way to signal over to GrResourceCache that it's become pointless to keep around textures based on that SkPixelRef when its pixels change, so that it can be a good citizen and free those textures.

This adds an invalidation listener mechanism to SkPixelRef to let it send this message while still staying ignorant of who's listening.

These messages are tricky to deliver.  The SkPixelRefs they originates from and the GrResourceCaches they ultimately end up at may be on different threads; neither class is threadsafe; their object lifetimes are totally independent; it's a many-senders-to-many-receivers relation; and neither codebase should really know about the other.

So I've added a per-message-type global message bus to broadcast messages to threadsafe inboxes.  Anyone can post() a message, which will show up in all the inboxes of that type, read whenever the inbox's owner calls poll().  The implementation is _dumb_; it can be improved in several dimensions (inbox size limits, lock-free message delivery) if we find the need.

I took some care to make sure not to send the invalidation message for any SkPixelRef that's sharing a generation ID with another SkPixelRef.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11949

[mtklein, 2013-10-17 19:52:41]

I've got the gist of this working again.  Can you take a look at this approach
and see if it matches what you were thinking?

Please ignore (if you can) the mess of LAZY and TODO I left myself.

[bsalomon, 2013-10-17 20:24:34]

On 2013/10/17 19:52:41, mtklein wrote:
> I've got the gist of this working again.  Can you take a look at this approach
> and see if it matches what you were thinking?
> 

Yup, this looks like it is headed in the right direction to me.

> Please ignore (if you can) the mess of LAZY and TODO I left myself.

[mtklein, 2013-10-17 20:26:41]

On 2013/10/17 20:24:34, bsalomon wrote:
> On 2013/10/17 19:52:41, mtklein wrote:
> > I've got the gist of this working again.  Can you take a look at this
approach
> > and see if it matches what you were thinking?
> > 
> 
> Yup, this looks like it is headed in the right direction to me.
> 
> > Please ignore (if you can) the mess of LAZY and TODO I left myself.

Thanks.  Will ping the thread when I've got it in shape.

[scroggo, 2013-10-22 21:04:01]

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:214: // This can be used to invalidate caches keyed by
generation ID.
It's not obvious from this comment that an InvalidationListener is only
meaningful/called if someone else knows this pixel ref's generation ID. Could
you add that to the comment?

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:224: bool addingAnInvalidationListenerMakesSense()
const { return fUniqueGenerationID; }
It was initially odd to me that this function depends on whether
getGenerationID() has been called (since creation/the last invalidation), but I
think adding a comment to InvalidationListener (as I suggested above) would make
that more clear.

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:276: void needsNewGenID();
Style nit: fields should go before methods (these are interspersed).

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:280: void invalidate();
This name seems to be lacking some context. It makes sense to the only caller to
have an InvalidationListener, but this private function really means "invalidate
anyone who used my genID as a cache." We also have a pattern of using a function
called "validate()" as a test to make sure that the object is valid, which makes
this name seem out of place.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1059: dst->pixelRef()->cloneGenID(*fPixelRef);
So this patch does not address the bug we discussed earlier that the ID gets
cloned even if we're looking at a subset? Can you add a FIXME that we know
there's a bug (or fix it)?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h
File src/core/SkMessageBus.h (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:25: ~Inbox();
Should this be virtual?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:27: // Make messages contain all the messages we've
received since the last call.  Threadsafe.
I find this comment hard to read - it would help to differentiate messages the
object from messages the concept. It also doesn't quite describe the function.
messages (the object) will only contain the messages received since the last
call, and fMessages is cleared.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:62: // This is a cheaper
fInboxes.remove(fInboxes.find(this)) when order doesn't matter.
Clever. Should this be an option on SkTDArray?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:80: messages->reset();
Should we check for NULL or assert non NULL?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:120: // This is subtle.  We must call
that.getGenerationID() to make sure it's genID isn't 0.
its*

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:220: // Listeners get at most one shot, so whether these
triggered or not, blow them away.
Should these comments go in the header file so a future user knows that the
invalidation listeners may not get called?

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp
File tests/PixelRefTest.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp#n...
tests/PixelRefTest.cpp:45: // Quick check that NULL is safe.
Is the goal here to test that we don't crash? Can you put this inside SK_DEBUG
since it won't really test anything in Release mode?

[bsalomon, 2013-10-23 13:52:57]

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:224: bool addingAnInvalidationListenerMakesSense()
const { return fUniqueGenerationID; }
MakesSense seems ambiguous. invalidationListenerMayBeCalled() ?

Or maybe it's just simpler to allow addInvalidationListener() to fail with a
false return if the listener will never be called?

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:280: void invalidate();
On 2013/10/22 21:04:02, scroggo wrote:
> This name seems to be lacking some context. It makes sense to the only caller
to
> have an InvalidationListener, but this private function really means
"invalidate
> anyone who used my genID as a cache." We also have a pattern of using a
function
> called "validate()" as a test to make sure that the object is valid, which
makes
> this name seem out of place.

sendInvalidationEvents(), notifyInvalidationListeners(), ...

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1059: dst->pixelRef()->cloneGenID(*fPixelRef);
On 2013/10/22 21:04:02, scroggo wrote:
> So this patch does not address the bug we discussed earlier that the ID gets
> cloned even if we're looking at a subset? Can you add a FIXME that we know
> there's a bug (or fix it)?

I'd prefer keeping a fix for that as a separate issue.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1099: if (pixelRef != NULL && fPixelRef != NULL) {
ubernbit: NULL != pixelRef

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h
File src/core/SkMessageBus.h (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:27: // Make messages contain all the messages we've
received since the last call.  Threadsafe.
On 2013/10/22 21:04:02, scroggo wrote:
> I find this comment hard to read - it would help to differentiate messages the
> object from messages the concept. It also doesn't quite describe the function.
> messages (the object) will only contain the messages received since the last
> call, and fMessages is cleared.

"The array messages"?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:62: // This is a cheaper
fInboxes.remove(fInboxes.find(this)) when order doesn't matter.
On 2013/10/22 21:04:02, scroggo wrote:
> Clever. Should this be an option on SkTDArray?

It already is: removeShuffle

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:109: /*static*/ void SkMessageBus<Message>::post(const
Message& m) {
Is the Get()/post() pattern useful? Could we just have static Post() that
internally calls Get(). Or maybe you're trying to avoid hitting that mutex a lot
for callers that have multiple messages to post in succession?

https://codereview.chromium.org/26734003/diff/466001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:315: GrResourceEntry* entry =
fCache.find(invalidated[i].key, GrTFindUnreffedFunctor());
Don't we need the unique check somewhere in here?

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp
File tests/PixelRefTest.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp#n...
tests/PixelRefTest.cpp:45: // Quick check that NULL is safe.
On 2013/10/22 21:04:02, scroggo wrote:
> Is the goal here to test that we don't crash? Can you put this inside SK_DEBUG
> since it won't really test anything in Release mode?

¿If there was a bug it could crash in release, no?

[mtklein, 2013-10-23 15:28:09]

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:214: // This can be used to invalidate caches keyed by
generation ID.
On 2013/10/22 21:04:02, scroggo wrote:
> It's not obvious from this comment that an InvalidationListener is only
> meaningful/called if someone else knows this pixel ref's generation ID. Could
> you add that to the comment?

Yeah, I was hoping you guys would get tripped up by this API and help me find a
clear way to explain the conditions under which we'll actually call the
listeners.  It's pretty weird.  Does this make more sense?

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:224: bool addingAnInvalidationListenerMakesSense()
const { return fUniqueGenerationID; }
On 2013/10/23 13:52:57, bsalomon wrote:
> MakesSense seems ambiguous. invalidationListenerMayBeCalled() ?
> 
> Or maybe it's just simpler to allow addInvalidationListener() to fail with a
> false return if the listener will never be called?

The idea was that we could guard against an unnecessary "l = new Listener;
delete l", which we can't really do in addInvalidationListener().  I think it
may be simpler to just remove this method.  No correctness difference with or
without it.

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:276: void needsNewGenID();
On 2013/10/22 21:04:02, scroggo wrote:
> Style nit: fields should go before methods (these are interspersed).

Done.  Are you sure this is a good thing to be in our style guide?  I find it
more readable to see the fields and their related methods together when they're
closely coupled like this.

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:280: void invalidate();
On 2013/10/23 13:52:57, bsalomon wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > This name seems to be lacking some context. It makes sense to the only
caller
> to
> > have an InvalidationListener, but this private function really means
> "invalidate
> > anyone who used my genID as a cache." We also have a pattern of using a
> function
> > called "validate()" as a test to make sure that the object is valid, which
> makes
> > this name seem out of place.
> 
> sendInvalidationEvents(), notifyInvalidationListeners(), ...

How's that?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1059: dst->pixelRef()->cloneGenID(*fPixelRef);
On 2013/10/23 13:52:57, bsalomon wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > So this patch does not address the bug we discussed earlier that the ID gets
> > cloned even if we're looking at a subset? Can you add a FIXME that we know
> > there's a bug (or fix it)?
> 
> I'd prefer keeping a fix for that as a separate issue.

Added skia:1742.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1099: if (pixelRef != NULL && fPixelRef != NULL) {
On 2013/10/23 13:52:57, bsalomon wrote:
> ubernbit: NULL != pixelRef

Done.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h
File src/core/SkMessageBus.h (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:25: ~Inbox();
On 2013/10/22 21:04:02, scroggo wrote:
> Should this be virtual?

Nope.  No virtual methods on Inbox (or SkMessageBus itself).

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:27: // Make messages contain all the messages we've
received since the last call.  Threadsafe.
On 2013/10/23 13:52:57, bsalomon wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > I find this comment hard to read - it would help to differentiate messages
the
> > object from messages the concept. It also doesn't quite describe the
function.
> > messages (the object) will only contain the messages received since the last
> > call, and fMessages is cleared.
> 
> "The array messages"?

Better?

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:62: // This is a cheaper
fInboxes.remove(fInboxes.find(this)) when order doesn't matter.
On 2013/10/23 13:52:57, bsalomon wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > Clever. Should this be an option on SkTDArray?
> 
> It already is: removeShuffle

Ah!  Missed that.  Updated.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:80: messages->reset();
On 2013/10/22 21:04:02, scroggo wrote:
> Should we check for NULL or assert non NULL?

Yes.  Added an assert.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:109: /*static*/ void SkMessageBus<Message>::post(const
Message& m) {
On 2013/10/23 13:52:57, bsalomon wrote:
> Is the Get()/post() pattern useful? Could we just have static Post() that
> internally calls Get(). Or maybe you're trying to avoid hitting that mutex a
lot
> for callers that have multiple messages to post in succession?

Ah, this may just be an issue with my tenuous grasp of Skia style.  This is
itself that very same static Post that internally calls Get.  Renamed.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:120: // This is subtle.  We must call
that.getGenerationID() to make sure it's genID isn't 0.
On 2013/10/22 21:04:02, scroggo wrote:
> its*

Done.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:220: // Listeners get at most one shot, so whether these
triggered or not, blow them away.
On 2013/10/22 21:04:02, scroggo wrote:
> Should these comments go in the header file so a future user knows that the
> invalidation listeners may not get called?

See if the header makes more sense now?  Leaving these here as a reminder.

https://codereview.chromium.org/26734003/diff/466001/src/gpu/GrResourceCache.cpp
File src/gpu/GrResourceCache.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/gpu/GrResourceCache....
src/gpu/GrResourceCache.cpp:315: GrResourceEntry* entry =
fCache.find(invalidated[i].key, GrTFindUnreffedFunctor());
On 2013/10/23 13:52:57, bsalomon wrote:
> Don't we need the unique check somewhere in here?

Yes.  That's what GrTFindUnreffedFunctor() does. :)

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp
File tests/PixelRefTest.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/tests/PixelRefTest.cpp#n...
tests/PixelRefTest.cpp:45: // Quick check that NULL is safe.
On 2013/10/23 13:52:57, bsalomon wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > Is the goal here to test that we don't crash? Can you put this inside
SK_DEBUG
> > since it won't really test anything in Release mode?
> 
> ¿If there was a bug it could crash in release, no?
Right.  This is relevant in both Release and Debug.  I think this should stay
as-is.

[bsalomon, 2013-10-23 15:32:27]

lgtm

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/466001/include/core/SkPixelRef....
include/core/SkPixelRef.h:280: void invalidate();
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/23 13:52:57, bsalomon wrote:
> > On 2013/10/22 21:04:02, scroggo wrote:
> > > This name seems to be lacking some context. It makes sense to the only
> caller
> > to
> > > have an InvalidationListener, but this private function really means
> > "invalidate
> > > anyone who used my genID as a cache." We also have a pattern of using a
> > function
> > > called "validate()" as a test to make sure that the object is valid, which
> > makes
> > > this name seem out of place.
> > 
> > sendInvalidationEvents(), notifyInvalidationListeners(), ...
> 
> How's that?
good

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h
File src/core/SkMessageBus.h (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:27: // Make messages contain all the messages we've
received since the last call.  Threadsafe.
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/23 13:52:57, bsalomon wrote:
> > On 2013/10/22 21:04:02, scroggo wrote:
> > > I find this comment hard to read - it would help to differentiate messages
> the
> > > object from messages the concept. It also doesn't quite describe the
> function.
> > > messages (the object) will only contain the messages received since the
last
> > > call, and fMessages is cleared.
> > 
> > "The array messages"?
> 
> Better?
ok by me

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:109: /*static*/ void SkMessageBus<Message>::post(const
Message& m) {
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/23 13:52:57, bsalomon wrote:
> > Is the Get()/post() pattern useful? Could we just have static Post() that
> > internally calls Get(). Or maybe you're trying to avoid hitting that mutex a
> lot
> > for callers that have multiple messages to post in succession?
> 
> Ah, this may just be an issue with my tenuous grasp of Skia style.  This is
> itself that very same static Post that internally calls Get.  Renamed.

Ha... I should have realized that from the impl.

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef....
include/core/SkPixelRef.h:219: // This can be used to invalidate caches keyed by
SkPixelRef generation ID.
clear to me

[scroggo, 2013-10-23 15:39:19]

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1059: dst->pixelRef()->cloneGenID(*fPixelRef);
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/23 13:52:57, bsalomon wrote:
> > On 2013/10/22 21:04:02, scroggo wrote:
> > > So this patch does not address the bug we discussed earlier that the ID
gets
> > > cloned even if we're looking at a subset? Can you add a FIXME that we know
> > > there's a bug (or fix it)?
> > 
> > I'd prefer keeping a fix for that as a separate issue.
> 
> Added skia:1742.

I still think it would be nice to document this in the code. Thanks for adding
skia:1742; I think it should be referenced in a comment. The bug references line
1058 may be less meaningful in the future if the code moves.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h
File src/core/SkMessageBus.h (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkMessageBus.h#...
src/core/SkMessageBus.h:27: // Make messages contain all the messages we've
received since the last call.  Threadsafe.
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/23 13:52:57, bsalomon wrote:
> > On 2013/10/22 21:04:02, scroggo wrote:
> > > I find this comment hard to read - it would help to differentiate messages
> the
> > > object from messages the concept. It also doesn't quite describe the
> function.
> > > messages (the object) will only contain the messages received since the
last
> > > call, and fMessages is cleared.
> > 
> > "The array messages"?
> 
> Better?

Yes.

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkPixelRef.cpp#...
src/core/SkPixelRef.cpp:220: // Listeners get at most one shot, so whether these
triggered or not, blow them away.
On 2013/10/23 15:28:10, mtklein wrote:
> On 2013/10/22 21:04:02, scroggo wrote:
> > Should these comments go in the header file so a future user knows that the
> > invalidation listeners may not get called?
> 
> See if the header makes more sense now?  Leaving these here as a reminder.

Yes, the header makes more sense now.

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef....
include/core/SkPixelRef.h:219: // This can be used to invalidate caches keyed by
SkPixelRef generation ID.
Thanks for the new comments, which are much clearer.

Maybe a better name might be GenIDChangeListener?

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef....
include/core/SkPixelRef.h:291: void callInvalidationListeners();
+1 to the new name (unless you change the name of InvalidationListener)

https://codereview.chromium.org/26734003/diff/556001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/26734003/diff/556001/src/gpu/SkGr.cpp#newcode111
src/gpu/SkGr.cpp:111: if (pixelRef) {
Do you want this check even though we've just asserted?

[mtklein, 2013-10-23 16:07:09]

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.chromium.org/26734003/diff/466001/src/core/SkBitmap.cpp#ne...
src/core/SkBitmap.cpp:1059: dst->pixelRef()->cloneGenID(*fPixelRef);
On 2013/10/23 15:39:19, scroggo wrote:
> On 2013/10/23 15:28:10, mtklein wrote:
> > On 2013/10/23 13:52:57, bsalomon wrote:
> > > On 2013/10/22 21:04:02, scroggo wrote:
> > > > So this patch does not address the bug we discussed earlier that the ID
> gets
> > > > cloned even if we're looking at a subset? Can you add a FIXME that we
know
> > > > there's a bug (or fix it)?
> > > 
> > > I'd prefer keeping a fix for that as a separate issue.
> > 
> > Added skia:1742.
> 
> I still think it would be nice to document this in the code. Thanks for adding
> skia:1742; I think it should be referenced in a comment. The bug references
line
> 1058 may be less meaningful in the future if the code moves.

Done.

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/26734003/diff/556001/include/core/SkPixelRef....
include/core/SkPixelRef.h:219: // This can be used to invalidate caches keyed by
SkPixelRef generation ID.
On 2013/10/23 15:39:19, scroggo wrote:
> Thanks for the new comments, which are much clearer.
> 
> Maybe a better name might be GenIDChangeListener?

Changed all the names to variants of GenIDChangeListener.

https://codereview.chromium.org/26734003/diff/556001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/26734003/diff/556001/src/gpu/SkGr.cpp#newcode111
src/gpu/SkGr.cpp:111: if (pixelRef) {
On 2013/10/23 15:39:19, scroggo wrote:
> Do you want this check even though we've just asserted?

Dropped the check.

[scroggo, 2013-10-23 16:10:59]

lgtm

[commit-bot: I haz the power, 2013-10-24 16:14:48]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/26734003/706001

[commit-bot: I haz the power, 2013-10-24 17:44:39]

Change committed as 11949