allow SkBitmapCache to operate on a local instance, for testability

allow SkBitmapCache to operate on a local instance, for testability

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/30ad5306be25565484a3dd76237984e071b7c4b3

[reed1, 2014-09-16 14:23:47]

reed@google.com changed reviewers:
+ danakj@chromium.org, junov@chromium.org, mtklein@google.com,
piotaixr@chromium.org

[reed1, 2014-09-16 15:13:43]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-16 15:14:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576763002/1

[commit-bot: I haz the power, 2014-09-16 15:14:52]

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-09-16 21:14 UTC

[reed1, 2014-09-16 15:15:32]

The CQ bit was unchecked by reed@google.com

[danakj, 2014-09-16 15:26:27]

LGTM thanks for getting that test enabled.

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

https://codereview.chromium.org/576763002/diff/1/src/core/SkBitmapCache.cpp#n...
src/core/SkBitmapCache.cpp:84: return CHECK_LOCAL(localCache, find, Find, key,
BitmapRec::Visitor, result);
This is really awkward to read :(

Tho, given you have to use a static method or a local method, I'm not sure what
else to suggest either, other than to stop using static methods to access
SkResourceCache (that'd be nice for many reasons).

https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cpp
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cp...
tests/SkResourceCacheTest.cpp:154:
SkGetGlobalDiscardableMemoryPool()->dumpPool();
Thanks, it's great that we can enable this test. I want to point out that this
is still a global, so we won't be able to write another test that uses
discardable memory without them stomping on each other until SkResourceCache can
be given a discardable memory pool instead of using a global.

[mtklein, 2014-09-16 15:39:20]

On 2014/09/16 15:26:27, danakj wrote:
> LGTM thanks for getting that test enabled.
> 
> https://codereview.chromium.org/576763002/diff/1/src/core/SkBitmapCache.cpp
> File src/core/SkBitmapCache.cpp (right):
> 
>
https://codereview.chromium.org/576763002/diff/1/src/core/SkBitmapCache.cpp#n...
> src/core/SkBitmapCache.cpp:84: return CHECK_LOCAL(localCache, find, Find, key,
> BitmapRec::Visitor, result);
> This is really awkward to read :(
> 
> Tho, given you have to use a static method or a local method, I'm not sure
what
> else to suggest either, other than to stop using static methods to access
> SkResourceCache (that'd be nice for many reasons).
> 
> https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cpp
> File tests/SkResourceCacheTest.cpp (right):
> 
>
https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cp...
> tests/SkResourceCacheTest.cpp:154:
> SkGetGlobalDiscardableMemoryPool()->dumpPool();
> Thanks, it's great that we can enable this test. I want to point out that this
> is still a global, so we won't be able to write another test that uses
> discardable memory without them stomping on each other until SkResourceCache
can
> be given a discardable memory pool instead of using a global.

Wow, was writing the same comments.  +1 to everything Dana said.

[reed1, 2014-09-16 17:31:12]

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

https://codereview.chromium.org/576763002/diff/1/src/core/SkBitmapCache.cpp#n...
src/core/SkBitmapCache.cpp:84: return CHECK_LOCAL(localCache, find, Find, key,
BitmapRec::Visitor, result);
On 2014/09/16 15:26:27, danakj wrote:
> This is really awkward to read :(
> 
> Tho, given you have to use a static method or a local method, I'm not sure
what
> else to suggest either, other than to stop using static methods to access
> SkResourceCache (that'd be nice for many reasons).

I find it very nice for calls sites that want to access the global cache to
consolidate their access pattern (scoping the lock on the mutex).

https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cpp
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/576763002/diff/1/tests/SkResourceCacheTest.cp...
tests/SkResourceCacheTest.cpp:154:
SkGetGlobalDiscardableMemoryPool()->dumpPool();
On 2014/09/16 15:26:27, danakj wrote:
> Thanks, it's great that we can enable this test. I want to point out that this
> is still a global, so we won't be able to write another test that uses
> discardable memory without them stomping on each other until SkResourceCache
can
> be given a discardable memory pool instead of using a global.

Agreed, that could be a separate issue/CL to allow use of the global discardable
pool to be mockable/testable across threads.

[reed1, 2014-09-16 17:31:19]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-09-16 17:32:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576763002/20001

[commit-bot: I haz the power, 2014-09-16 17:32:09]

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-09-16 23:32 UTC

[mtklein, 2014-09-16 17:37:29]

lgtm

[commit-bot: I haz the power, 2014-09-16 17:39:58]

Committed patchset #2 (id:20001) as 30ad5306be25565484a3dd76237984e071b7c4b3