add optional SkAlphaType parameter to notifyPixelsChanged

add optional SkAlphaType parameter to notifyPixelsChanged

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

[scroggo, 2014-01-24 20:50:51]

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

https://codereview.chromium.org/147213002/diff/1/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:326: fPixelRef->notifyAlphaTypeChanged(alphaType);
One downside is that if a bitmap representing an opaque subset calls this, the
full set has its alpha type changed. I think we all agreed we don't care about
such a use case, so that's fine.

Maybe we could put a comment in the doc that it will affect all bitmaps pointing
to the same pixel ref (which I assume Hal won't like because it means more
exposure for pixel refs). I'm fine with or without the added comment.

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

https://codereview.chromium.org/147213002/diff/1/src/core/SkPixelRef.cpp#newc...
src/core/SkPixelRef.cpp:270: this->callGenIDChangeListeners();
I kind of think these two functions should be the same (or perhaps one should
call the other). The only time we call one but not the other is in the
destructor, when we don't need to call needsNewGenID, but setting two fields
needlessly might be a low price to pay to ensure we never accidentally call one
without the other.

[reed1, 2014-01-27 13:46:14]

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

https://codereview.chromium.org/147213002/diff/1/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:326: fPixelRef->notifyAlphaTypeChanged(alphaType);
On 2014/01/24 20:50:52, scroggo wrote:
> One downside is that if a bitmap representing an opaque subset calls this, the
> full set has its alpha type changed. I think we all agreed we don't care about
> such a use case, so that's fine.
> 
> Maybe we could put a comment in the doc that it will affect all bitmaps
pointing
> to the same pixel ref (which I assume Hal won't like because it means more
> exposure for pixel refs). I'm fine with or without the added comment.

Done.

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

https://codereview.chromium.org/147213002/diff/1/src/core/SkPixelRef.cpp#newc...
src/core/SkPixelRef.cpp:270: this->callGenIDChangeListeners();
On 2014/01/24 20:50:52, scroggo wrote:
> I kind of think these two functions should be the same (or perhaps one should
> call the other). The only time we call one but not the other is in the
> destructor, when we don't need to call needsNewGenID, but setting two fields
> needlessly might be a low price to pay to ensure we never accidentally call
one
> without the other.

I'll sync with the GPU guys, who invented this pattern I think.

[reed1, 2014-01-27 14:12:00]

Change the pixelref API to reflect that changing alphatype is just as heavy as
changing pixels themselves.

[robertphillips, 2014-01-27 14:24:26]

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

https://codereview.chromium.org/147213002/diff/80005/include/core/SkPixelRef....
include/core/SkPixelRef.h:133: *
rm 'it' in "If it"?

https://codereview.chromium.org/147213002/diff/80005/include/core/SkPixelRef....
include/core/SkPixelRef.h:341: 
fAlphaType?

[reed1, 2014-01-27 14:27:13]

ptal

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

https://codereview.chromium.org/147213002/diff/80005/include/core/SkPixelRef....
include/core/SkPixelRef.h:133: *
On 2014/01/27 14:24:27, robertphillips wrote:
> rm 'it' in "If it"?

Done.

https://codereview.chromium.org/147213002/diff/80005/include/core/SkPixelRef....
include/core/SkPixelRef.h:341: 
On 2014/01/27 14:24:27, robertphillips wrote:
> fAlphaType?

Done.

[robertphillips, 2014-01-27 14:31:28]

lgtm

[commit-bot: I haz the power, 2014-01-27 14:45:23]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/147213002/130002

[commit-bot: I haz the power, 2014-01-27 15:41:11]

Change committed as 13192