Cleanup: Removing unnecessary args/complexity in SkSurface_Base and friends

Cleanup: Removing unnecessary args/complexity in SkSurface_Base and friends
Committed: https://code.google.com/p/skia/source/detail?r=8708

[Justin Novosad, 2013-04-15 18:40:40]

PTAL

[Justin Novosad, 2013-04-15 18:44:36]

On 2013/04/15 18:40:40, junov wrote:
> PTAL

Context: In a WIP CL I encountered a bug caused by the aboutToDraw(NULL) call in
SkSurface_Base::notifyContentChanged.  This caused me to question the necessity
of passing around canvas ptrs at all.

[reed1, 2013-04-15 18:56:33]

I think removing unused parameters is probably fine, but I don't understand all
of the new assumptions in SkSurface.cpp

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp#newco...
src/image/SkSurface.cpp:64: 
How do we know that fCachedCanvas is always present?

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp#newco...
src/image/SkSurface.cpp:66: // getSurfaceBase will be non-NULL if there are
SkImages
Why is this test more robust than just checking fCachedImage for null?

[Justin Novosad, 2013-04-15 19:08:57]

New Patch.

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp#newco...
src/image/SkSurface.cpp:64: 
On 2013/04/15 18:56:33, reed1 wrote:
> How do we know that fCachedCanvas is always present?

Done.

https://codereview.chromium.org/14263017/diff/1/src/image/SkSurface.cpp#newco...
src/image/SkSurface.cpp:66: // getSurfaceBase will be non-NULL if there are
SkImages
On 2013/04/15 18:56:33, reed1 wrote:
> Why is this test more robust than just checking fCachedImage for null?

Done.

[Justin Novosad, 2013-04-16 15:02:42]

I think the assumptions are clearer in the new version of SkSurface.cpp by
putting the check on fCachedImage first.  I also documented the assumption that
you can't have a cached image without a cached canvas.  Makes sense?

[reed1, 2013-04-16 15:31:23]

https://codereview.chromium.org/14263017/diff/3002/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/14263017/diff/3002/src/image/SkSurface.cpp#ne...
src/image/SkSurface.cpp:69: SkASSERT(fCachedCanvas);  // Can't have a cached
image without a cached canvas.
Why not? I'm trying trace the code to see why this must be true. And even if it
happens to be true today, is that a real invariant from this method's
perspective?

At least with today's code, if I do this, I don't have a cached-canvas when I
get to aboutToDraw.

            SkSurface* sur = SkSurface::NewRaster(info);
            sur->notifyContentChanged();

[Justin Novosad, 2013-04-16 15:38:05]

Patch.

[Justin Novosad, 2013-04-16 15:52:43]

New Patch. Added unit tests

[reed1, 2013-04-16 16:04:14]

https://codereview.chromium.org/14263017/diff/13004/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/14263017/diff/13004/src/image/SkSurface.cpp#n...
src/image/SkSurface.cpp:65: if (NULL == fCachedImage || NULL == fCachedCanvas) {
Not sure we should do nothing if we have an image but no cached-canvas.

new surface
newSnapShot()
notifyPixelsChanged() -- calls aboutToDraw
// caller directly wacks the pixels
newSnapShot() -- we return the same image object, but new bits I think

The old flow just treated these as two separate tests. Was that wrong?

[Justin Novosad, 2013-04-16 18:11:47]

https://codereview.chromium.org/14263017/diff/13004/src/image/SkSurface.cpp
File src/image/SkSurface.cpp (right):

https://codereview.chromium.org/14263017/diff/13004/src/image/SkSurface.cpp#n...
src/image/SkSurface.cpp:65: if (NULL == fCachedImage || NULL == fCachedCanvas) {
On 2013/04/16 16:04:14, reed1 wrote:
> Not sure we should do nothing if we have an image but no cached-canvas.
> 
> new surface
> newSnapShot()
> notifyPixelsChanged() -- calls aboutToDraw
> // caller directly wacks the pixels
> newSnapShot() -- we return the same image object, but new bits I think
> 
> The old flow just treated these as two separate tests. Was that wrong?

You're right I was completely ignoring that use case.

Fixed it in new patch, and augmented test to cover that case and verify that the
images are forked.

[reed1, 2013-04-16 19:39:15]

lgtm