retool image cache to be generic cache

retool image cache to be generic cache, allowing the client to subclass "Rec", so they can provide a custom Key and arbitrary Value.

Follow-on CLs

- rename ScaledimageCache to something like GeneralCache
- explore if we can use call-backs or some mechanism to completely hide "lock/unlock", by forcing all clients to support "copying" their value out of the cache as the result of a Find.

Committed: https://skia.googlesource.com/skia/+/680fb9e8f10d24b5fe35c90338de37c57392f1aa

[qiankun, 2014-08-26 07:12:40]

qiankun.miao@intel.com changed reviewers:
+ qiankun.miao@intel.com

[qiankun, 2014-08-26 07:12:40]

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.c...
src/core/SkBitmapCache.cpp:52: BitmapKey   fKey;
Can we use pointer type for fKey to avoid using virtual function? New a Key
object when doing AddAndLock(), and delete it in Rec's destructor.

https://codereview.chromium.org/507483002/diff/20001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:31: }
Do we still need these two functions for casting between rec and id? ID is
already typedefed as "const Rec*".

[reed1, 2014-08-26 14:04:02]

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.cpp
File src/core/SkBitmapCache.cpp (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.c...
src/core/SkBitmapCache.cpp:52: BitmapKey   fKey;
On 2014/08/26 07:12:40, qiankun wrote:
> Can we use pointer type for fKey to avoid using virtual function? New a Key
> object when doing AddAndLock(), and delete it in Rec's destructor.

If the virtual call overhead shows up as a problem, we can consider having the
base-class cache the returned address.

https://codereview.chromium.org/507483002/diff/20001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:31: }
On 2014/08/26 07:12:40, qiankun wrote:
> Do we still need these two functions for casting between rec and id? <span
id="abbrex-spnAbbr_49173" class="abbrex-Abbr abbrex-Abbr_ID abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">ID</span> is
> already typedefed as "const Rec*".

Done.

[reed1, 2014-08-26 14:04:39]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-26 14:05:15]

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

[commit-bot: I haz the power, 2014-08-26 14:05:17]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-08-26 20:05 UTC

[Stephen White, 2014-08-26 14:25:08]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-08-26 14:25:08]

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.h
File src/core/SkBitmapCache.h (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.h...
src/core/SkBitmapCache.h:11: #include "SkScaledImageCache.h"
Not new to this patch, but I'm a little confused by this dependency. Is this
because it's pending a rename? Shouldn't the SkBitmapCache be the base class,
and SkScaledImageCache its client?

[reed1, 2014-08-26 14:40:51]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-26 14:45:03]

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.h
File src/core/SkBitmapCache.h (right):

https://codereview.chromium.org/507483002/diff/20001/src/core/SkBitmapCache.h...
src/core/SkBitmapCache.h:11: #include "SkScaledImageCache.h"
On 2014/08/26 14:25:08, Stephen White wrote:
> Not new to this patch, but I'm a little confused by this dependency. Is this
> because it's pending a rename? Shouldn't the SkBitmapCache be the base class,
> and SkScaledImageCache its client?

BitmapCache is a specialization of our (yet to be renamed) general cache
(currently named ScaledImageCache, but that will change).

[reed1, 2014-08-26 14:47:08]

PTAL

[mtklein, 2014-08-26 15:55:51]

lgtm

https://codereview.chromium.org/507483002/diff/80001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.cpp (right):

https://codereview.chromium.org/507483002/diff/80001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.cpp:282: const_cast<Rec*>(rec)->fLockCount -= 1;
// We're under our lock, and we're the only possible mutator.
?

[reed1, 2014-08-26 16:01:29]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-26 16:01:41]

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

[commit-bot: I haz the power, 2014-08-26 16:08:11]

Committed patchset #7 (120001) as 680fb9e8f10d24b5fe35c90338de37c57392f1aa

[Stephen White, 2014-08-26 16:10:54]

https://codereview.chromium.org/507483002/diff/100001/src/core/SkBitmapProcSt...
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/507483002/diff/100001/src/core/SkBitmapProcSt...
src/core/SkBitmapProcState.h:147: //    SkScaledImageCache::ID fScaledCacheID;
Remove commented code?

[reed1, 2014-08-26 17:59:32]

On 2014/08/26 16:10:54, Stephen White wrote:
>
https://codereview.chromium.org/507483002/diff/100001/src/core/SkBitmapProcSt...
> File src/core/SkBitmapProcState.h (right):
> 
>
https://codereview.chromium.org/507483002/diff/100001/src/core/SkBitmapProcSt...
> src/core/SkBitmapProcState.h:147: //    SkScaledImageCache::ID fScaledCacheID;
> Remove commented code?

Will do.