fixing backing store access in SkDeferredCanvas

[junov1, 2012-02-03 21:15:19]

This fixes the webkit layout tests that were failing with deferred canvas
enabled.  I also plugged an embarrassing memory leak.

http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (left):

http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cp...
src/utils/SkDeferredCanvas.cpp:474: SkSafeRef(fImmediateCanvas);
Big whoops: The main canvas, along with its device were being leaked!

[junov1, 2012-02-03 21:17:00]

BUG=http://code.google.com/p/skia/issues/detail?id=475

[reed1, 2012-02-03 22:01:14]

are either of these necessary for M18 (i.e. does M18 use DeferredCanvas?)

http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cp...
src/utils/SkDeferredCanvas.cpp:603: // (the caller of this method) manipulates
the right bitmap.
Is it guaranteed that the bitmap will have previously been locked?

I wonder if we need to change the public API to something that requires that we
make a copy of the bitmap and return that to the caller (or the caller passes in
a bitmap). That way we don't do this cloudy trick of changing the device's
bitmap as a "side-effect" of calling access.

[junov1, 2012-02-06 15:02:59]

On 2012/02/03 22:01:14, reed1 wrote:
> are either of these necessary for M18 (i.e. does M18 use DeferredCanvas?)

No. This code is not used in M18.
> 
> http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cpp
> File src/utils/SkDeferredCanvas.cpp (right):
> 
>
http://codereview.appspot.com/5626047/diff/2001/src/utils/SkDeferredCanvas.cp...
> src/utils/SkDeferredCanvas.cpp:603: // (the caller of this method) manipulates
> the right bitmap.
> Is it guaranteed that the bitmap will have previously been locked?
> 
> I wonder if we need to change the public API to something that requires that
we
> make a copy of the bitmap and return that to the caller (or the caller passes
in
> a bitmap). That way we don't do this cloudy trick of changing the device's
> bitmap as a "side-effect" of calling access.

The current implementation does guarantee that the device holds a lock on the
bitmap.  But that lock is not strictly required as far as I can tell, so I am
forced to admit that this change makes the code more fragile. I think there is a
good way to make this more robust, without too much churn in the public API: we
could make onAccessBitmap (which is virtual) responsible for retrieving the
bitmap.  That way, the API change will only impact overrides of onAccessBitmap,
rather than impacting all the call sites of accessBitmap.  

What say you?

[junov1, 2012-02-06 21:20:32]

In the latest patch, I made a small API change to make the bitmap override safer
and cleaner.  This change will require a small companion change on the chromium
side in order to roll, which I posted here:
https://chromiumcodereview.appspot.com/9341003

[reed1, 2012-02-07 13:10:08]

Skia likes to return a const & instead of a const *, unless you're suggesting
that the method can sometimes return NULL.

lgtm otherwise.

[junov1, 2012-02-07 14:22:02]

OK, I'll take care of that.  At this point are we planing any more skia
rolls in the M18 branch?  If so, maybe I can keep this on hold for a few
days... The companion change for rolling into chromium in not very scary
though, so maybe it's not a big deal?

On Tue, Feb 7, 2012 at 8:10 AM, <reed@google.com> wrote:

> Skia likes to return a const & instead of a const *, unless you're
> suggesting that the method can sometimes return NULL.
>
> lgtm otherwise.
>
>
http://codereview.appspot.com/**5626047/<http://codereview.appspot.com/5626047/>
>

[reed1, 2012-02-07 14:31:11]

We are done rolling skia for M18. Any changes we want from here out will
mean creating a branch and cherry-picking.

On Tue, Feb 7, 2012 at 9:22 AM, Justin Novosad <junov@chromium.org> wrote:

> OK, I'll take care of that.  At this point are we planing any more skia
> rolls in the M18 branch?  If so, maybe I can keep this on hold for a few
> days... The companion change for rolling into chromium in not very scary
> though, so maybe it's not a big deal?
>
>
> On Tue, Feb 7, 2012 at 8:10 AM, <reed@google.com> wrote:
>
>> Skia likes to return a const & instead of a const *, unless you're
>> suggesting that the method can sometimes return NULL.
>>
>> lgtm otherwise.
>>
>>
http://codereview.appspot.com/**5626047/<http://codereview.appspot.com/5626047/>
>>
>
>

[junov1, 2012-02-07 14:47:55]

On 2012/02/07 13:10:08, reed1 wrote:
> Skia likes to return a const & instead of a const *, unless you're suggesting
> that the method can sometimes return NULL.

Felt more coherent to also change the bitmap argument of onAccessBitmap to be
passed by ref rather than a pointer.  Reinforces the concept that the bitmap is
never NULL.  lgty?

[reed1, 2012-02-07 15:44:51]

actually, skia is pretty consistent that we use & only with const, since the
call-site "looks" like its passing by value (which it semantically is). I think
the previous version is correct:

    const SkBitmap& onAccessBitmap(SkBitmap*);

Takes a mutable argument, returns an immutable one.