skia: Added chrome implementation of SkDiscardableMemory.

skia: Added chrome implementation of SkDiscardableMemory.

The implementation resembles that of WebDiscardableMemory. The skia part of
this patch is here: https://codereview.chromium.org/22956004

R=reed@google.com, scroggo@google.com, hclam@chromium.org, nduca@chromium.org, jamesr@chromium.org
BUG=229120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219386

[nduca, 2013-08-14 22:53:37]

some nits, minor though

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
File skia/ext/sk_discardable_memory_chrome.h (right):

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
skia/ext/sk_discardable_memory_chrome.h:16: class SK_API
SkDiscardableMemoryChrome : public SkDiscardableMemory {
Also impl is a useful suffix when you're implementign an interface and not in cc
code (where we are insane).

It could be SkDiscardableMemoryImpl sk_discardable_memory_impl.h

and then have a separate file for the factory_impl.

[scroggo, 2013-08-15 14:15:24]

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
File skia/ext/sk_discardable_memory_chrome.h (right):

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
skia/ext/sk_discardable_memory_chrome.h:18: SkDiscardableMemoryChrome();
It looks like someone could create an SkDiscardableMemoryChrome directly (rather
than using the factory), and call InitializeAndLock as desired? Is this
intentional?

[ernstm, 2013-08-15 20:30:11]

Replaced SkDiscardableMemoryFactory by sk_create_discardable_memory function
(following the embedding pattern of SkThread).

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
File skia/ext/sk_discardable_memory_chrome.h (right):

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
skia/ext/sk_discardable_memory_chrome.h:16: class SK_API
SkDiscardableMemoryChrome : public SkDiscardableMemory {
On 2013/08/14 22:53:37, nduca wrote:
> Also impl is a useful suffix when you're implementign an interface and not in
cc
> code (where we are insane).
> 
> It could be SkDiscardableMemoryImpl sk_discardable_memory_impl.h
> 
> and then have a separate file for the factory_impl.

Following the embedding pattern of SkThread, I kept the name
SkDiscardableMemoryChrome and named the files SkDiscardableMemory_chrome.*

https://codereview.chromium.org/23206002/diff/1/skia/ext/sk_discardable_memor...
skia/ext/sk_discardable_memory_chrome.h:18: SkDiscardableMemoryChrome();
On 2013/08/15 14:15:25, scroggo wrote:
> It looks like someone could create an SkDiscardableMemoryChrome directly
(rather
> than using the factory), and call InitializeAndLock as desired? Is this
> intentional?

I think it's safe to create and initialize SkDiscardableMemoryChrome directly,
as the factory was not doing any book keeping (it was now replaced by a
function).

[ernstm, 2013-08-15 21:12:52]

Made factory function a method.

[Ian Vollick, 2013-08-16 11:36:30]

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
File skia/ext/SkDiscardableMemory_chrome.cc (right):

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
skia/ext/SkDiscardableMemory_chrome.cc:18: return false;
Do you really want to do this in response to being purged? Although you'll need
to recreate the data, you've got your memory locked and allocated and ready to
go at this point.

[ernstm, 2013-08-16 17:01:22]

On 2013/08/16 11:36:30, vollick wrote:
>
https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
> File skia/ext/SkDiscardableMemory_chrome.cc (right):
> 
>
https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
> skia/ext/SkDiscardableMemory_chrome.cc:18: return false;
> Do you really want to do this in response to being purged? Although you'll
need
> to recreate the data, you've got your memory locked and allocated and ready to
> go at this point.

I made the interface and the implementation essentially the same as
WebDiscardableMemory. I think for now it's fine to just return false. We can
extend the interface if this turns out to be insufficient.

[ernstm, 2013-08-16 20:59:54]

Can we move forward with this patch? We already got green light for the
interface on the Skia side.

[Ian Vollick, 2013-08-17 00:22:17]

On 2013/08/16 20:59:54, Manfred Ernst wrote:
> Can we move forward with this patch? We already got green light for the
> interface on the Skia side.

Yep! lg2m. Sorry for holding up the show.

[nduca, 2013-08-17 00:24:38]

lgtm3

[ernstm, 2013-08-19 18:33:14]

On 2013/08/17 00:24:38, nduca (ooo untill 8-26) wrote:
> lgtm3

Mike, Leon, I think we need an OWNER's okay from one of you to commit.

[scroggo, 2013-08-19 18:34:29]

lgtm, pending submit of skia change.

[reed1, 2013-08-20 15:23:47]

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
File skia/ext/SkDiscardableMemory_chrome.h (right):

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
skia/ext/SkDiscardableMemory_chrome.h:24: bool lock();
Do you not say virtual and/or override for these methods, to document that they
are from the base class?

[ernstm, 2013-08-21 23:00:22]

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
File skia/ext/SkDiscardableMemory_chrome.h (right):

https://codereview.chromium.org/23206002/diff/16001/skia/ext/SkDiscardableMem...
skia/ext/SkDiscardableMemory_chrome.h:24: bool lock();
On 2013/08/20 15:23:47, reed1 wrote:
> Do you not say virtual and/or override for these methods, to document that
they
> are from the base class?

We do. Done.

[commit-bot: I haz the power, 2013-08-21 23:02:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/23206002/29001

[commit-bot: I haz the power, 2013-08-21 23:32:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-23 16:40:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/23206002/4002

[commit-bot: I haz the power, 2013-08-23 17:28:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-23 20:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/23206002/52001

[commit-bot: I haz the power, 2013-08-24 00:03:51]

Change committed as 219386