detect if the scaledimagecache returns a purged bitmap

detect if the scaledimagecache returns a purged bitmap

BUG=
R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12654

[scroggo, 2013-12-12 20:34:54]

lgtm

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState.cpp:109: class AutoScaledCacheUnlocker {
Maybe not necessary, since this is hidden in a source file, but should this use
SK_REQUIRE_LOCAL_VAR?

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState.cpp:119: void keepLock() {
How about reset? That keeps it consistent with our other Auto classes. I also
think it's a clearer name (since "reset" applies to the class who owns it, where
as "keepLock" refers to its private member).

https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.h (right):

https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.h:156: void dump() const;
Can you add some comments? I see "dump" and I'm not sure whether it means debug
dump or purge.

Also, should it be only defined in SK_DEBUG?

[hal.canary, 2013-12-12 20:48:42]

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState.cpp:119: void keepLock() {
On 2013/12/12 20:34:55, scroggo wrote:
> How about reset? That keeps it consistent with our other Auto classes. I also
> think it's a clearer name (since "reset" applies to the class who owns it,
where
> as "keepLock" refers to its private member).

it's more of a detach() than a reset()

[scroggo, 2013-12-12 20:50:41]

On 2013/12/12 20:48:42, Hal Canary wrote:
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
> File src/core/SkBitmapProcState.cpp (right):
> 
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
> src/core/SkBitmapProcState.cpp:119: void keepLock() {
> On 2013/12/12 20:34:55, scroggo wrote:
> > How about reset? That keeps it consistent with our other Auto classes. I
also
> > think it's a clearer name (since "reset" applies to the class who owns it,
> where
> > as "keepLock" refers to its private member).
> 
> it's more of a detach() than a reset()

I disagree. Our templates use detach() to mean that the caller wants to take
ownership of the attached object, which isn't happening here.

[reed1, 2013-12-12 21:13:48]

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState.cpp:109: class AutoScaledCacheUnlocker {
On 2013/12/12 20:34:55, scroggo wrote:
> Maybe not necessary, since this is hidden in a source file, but should this
use
> SK_REQUIRE_LOCAL_VAR?

Done.

https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
src/core/SkBitmapProcState.cpp:119: void keepLock() {
On 2013/12/12 20:48:43, Hal Canary wrote:
> On 2013/12/12 20:34:55, scroggo wrote:
> > How about reset? That keeps it consistent with our other Auto classes. I
also
> > think it's a clearer name (since "reset" applies to the class who owns it,
> where
> > as "keepLock" refers to its private member).
> 
> it's more of a detach() than a reset()

How about release() -- release, since detach usually returns something, which
isn't the case here.

https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.h (right):

https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.h:156: void dump() const;
On 2013/12/12 20:34:55, scroggo wrote:
> Can you add some comments? I see "dump" and I'm not sure whether it means
debug
> dump or purge.
> 
> Also, should it be only defined in SK_DEBUG?

Not sure. SkPath has a dump() that works in release mode, and I have used it
from with a debugger, to examine the path.

[scroggo, 2013-12-12 21:16:11]

On 2013/12/12 21:13:48, reed1 wrote:
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
> File src/core/SkBitmapProcState.cpp (right):
> 
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
> src/core/SkBitmapProcState.cpp:109: class AutoScaledCacheUnlocker {
> On 2013/12/12 20:34:55, scroggo wrote:
> > Maybe not necessary, since this is hidden in a source file, but should this
> use
> > SK_REQUIRE_LOCAL_VAR?
> 
> Done.
> 
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkBitmapProcSta...
> src/core/SkBitmapProcState.cpp:119: void keepLock() {
> On 2013/12/12 20:48:43, Hal Canary wrote:
> > On 2013/12/12 20:34:55, scroggo wrote:
> > > How about reset? That keeps it consistent with our other Auto classes. I
> also
> > > think it's a clearer name (since "reset" applies to the class who owns it,
> > where
> > > as "keepLock" refers to its private member).
> > 
> > it's more of a detach() than a reset()
> 
> How about release() -- release, since detach usually returns something, which
> isn't the case here.
> 
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
> File src/core/SkScaledImageCache.h (right):
> 
>
https://codereview.chromium.org/110383005/diff/60001/src/core/SkScaledImageCa...
> src/core/SkScaledImageCache.h:156: void dump() const;
> On 2013/12/12 20:34:55, scroggo wrote:
> > Can you add some comments? I see "dump" and I'm not sure whether it means
> debug
> > dump or purge.
> > 
> > Also, should it be only defined in SK_DEBUG?
> 
> Not sure. SkPath has a dump() that works in release mode, and I have used it
> from with a debugger, to examine the path.

lgtm at patch set 5, or with reset changed to release.

[reed1, 2013-12-12 21:37:34]

Committed patchset #6 manually as r12654 (presubmit successful).