base: Refactor DiscardableMemoryProvider to build and be tested on all platforms.

base: Refactor DiscardableMemoryProvider to build and be tested on all platforms.

This also moves DiscardableMemory::CreateLockedMemory implementations
to platform specific files and allows us to choose what implementation
to use at runtime.

No change in behavior, refactor only.

BUG=327516
TEST=base_unitttest --gtest_filter=DiscardableMemory*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240924

[Philippe, 2013-12-11 08:56:27]

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_emulated.h (right):

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:12: class DiscardableMemoryEmulated :
public DiscardableMemory {
Should we have this in base::internal?

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:19: bool Initialize();
Just a suggestion, feel free to ignore :)

I see that we have this pattern now in a few files:
scoped_ptr<DiscardableMemoryEmulated> memory(
    new DiscardableMemoryEmulated(size));
if (!memory->Initialize())
  return scoped_ptr<DiscardableMemoryEmulated>();
return memory.PassAs<DiscardableMemory>();

If you make this Initialize() method and the constructor private and add a
Create() public static factory method returning a scoped_ptr then we could
decrease slightly the boilerplate on the client site.

What do you think?

[reveman, 2013-12-12 06:54:23]

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_emulated.h (right):

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:12: class DiscardableMemoryEmulated :
public DiscardableMemory {
On 2013/12/11 08:56:27, Philippe wrote:
> Should we have this in base::internal?

Done.

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:19: bool Initialize();
On 2013/12/11 08:56:27, Philippe wrote:
> Just a suggestion, feel free to ignore :)
> 
> I see that we have this pattern now in a few files:
> scoped_ptr<DiscardableMemoryEmulated> memory(
>     new DiscardableMemoryEmulated(size));
> if (!memory->Initialize())
>   return scoped_ptr<DiscardableMemoryEmulated>();
> return memory.PassAs<DiscardableMemory>();
> 
> If you make this Initialize() method and the constructor private and add a
> Create() public static factory method returning a scoped_ptr then we could
> decrease slightly the boilerplate on the client site.
> 
> What do you think?

I prefer to avoid static Create functions when possible as it forces us to write
more reusable code. ie. the functionality in DiscardableMemoryEmulated can
easily be reused by creating a derived class or moving some of the functionality
to a separate base class that can be inherited from.

We're using the same pattern for some of the types in ui/gl and
GpuMemoryBufferTypeImpl found in content/common/gpu/client/.

[Philippe, 2013-12-12 08:53:35]

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_emulated.h (right):

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:19: bool Initialize();
On 2013/12/12 06:54:23, David Reveman wrote:
> On 2013/12/11 08:56:27, Philippe wrote:
> > Just a suggestion, feel free to ignore :)
> > 
> > I see that we have this pattern now in a few files:
> > scoped_ptr<DiscardableMemoryEmulated> memory(
> >     new DiscardableMemoryEmulated(size));
> > if (!memory->Initialize())
> >   return scoped_ptr<DiscardableMemoryEmulated>();
> > return memory.PassAs<DiscardableMemory>();
> > 
> > If you make this Initialize() method and the constructor private and add a
> > Create() public static factory method returning a scoped_ptr then we could
> > decrease slightly the boilerplate on the client site.
> > 
> > What do you think?
> 
> I prefer to avoid static Create functions when possible as it forces us to
write
> more reusable code. ie. the functionality in DiscardableMemoryEmulated can
> easily be reused by creating a derived class or moving some of the
functionality
> to a separate base class that can be inherited from.
> 
> We're using the same pattern for some of the types in ui/gl and
> GpuMemoryBufferTypeImpl found in content/common/gpu/client/.

Fair enough, I would tend to avoid such inheritance though and prefer
composition. But then the static factory method doesn't play nicely either with
composition as it comes with its own heap allocation. So I guess you are
planning to reuse/extend DiscardableMemoryEmulated?

[Philippe, 2013-12-12 13:43:30]

lgtm btw, although William is the expert here :)

[reveman, 2013-12-12 16:25:16]

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_emulated.h (right):

https://codereview.chromium.org/111713008/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_emulated.h:19: bool Initialize();
On 2013/12/12 08:53:35, Philippe wrote:
> On 2013/12/12 06:54:23, David Reveman wrote:
> > On 2013/12/11 08:56:27, Philippe wrote:
> > > Just a suggestion, feel free to ignore :)
> > > 
> > > I see that we have this pattern now in a few files:
> > > scoped_ptr<DiscardableMemoryEmulated> memory(
> > >     new DiscardableMemoryEmulated(size));
> > > if (!memory->Initialize())
> > >   return scoped_ptr<DiscardableMemoryEmulated>();
> > > return memory.PassAs<DiscardableMemory>();
> > > 
> > > If you make this Initialize() method and the constructor private and add a
> > > Create() public static factory method returning a scoped_ptr then we could
> > > decrease slightly the boilerplate on the client site.
> > > 
> > > What do you think?
> > 
> > I prefer to avoid static Create functions when possible as it forces us to
> write
> > more reusable code. ie. the functionality in DiscardableMemoryEmulated can
> > easily be reused by creating a derived class or moving some of the
> functionality
> > to a separate base class that can be inherited from.
> > 
> > We're using the same pattern for some of the types in ui/gl and
> > GpuMemoryBufferTypeImpl found in content/common/gpu/client/.
> 
> Fair enough, I would tend to avoid such inheritance though and prefer
> composition. But then the static factory method doesn't play nicely either
with
> composition as it comes with its own heap allocation. So I guess you are
> planning to reuse/extend DiscardableMemoryEmulated?

Composition can definitely make more sense in some cases and this doesn't
prevent us from using that. This is more about putting some pattern in place
that makes the code easier to extend and maintain. This pattern works well in my
experience for code that requires platform specific implementations.

DiscardableMemoryEmulated might not be reused specifically but future ashmem
code that needs to be used on both android and chromeos will be.

[reveman, 2013-12-13 18:29:52]

willchan, ping for review.

Here's a follow up patch that might be useful to understand the direction I'm
going: https://codereview.chromium.org/114923005/

The other patch that will build on top of this is ashmem support on
linux/chromeos, which will require runtime detection of ashmem support.

[willchan no longer on Chromium, 2013-12-13 19:45:38]

Nice refactor, I like it. And I like that you kept the refactor separate from
any behavioral changes. LGTM. Only had one nit.

https://codereview.chromium.org/111713008/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory_provider.h (right):

https://codereview.chromium.org/111713008/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_provider.h:103: DiscardableMemoryProvider*
provider,
If you're going to add the provider, why isn't this just a member function?

[reveman, 2013-12-13 20:12:58]

https://codereview.chromium.org/111713008/diff/20001/base/memory/discardable_...
File base/memory/discardable_memory_provider.h (right):

https://codereview.chromium.org/111713008/diff/20001/base/memory/discardable_...
base/memory/discardable_memory_provider.h:103: DiscardableMemoryProvider*
provider,
On 2013/12/13 19:45:39, willchan wrote:
> If you're going to add the provider, why isn't this just a member function?

Good call. Fixed.

[commit-bot: I haz the power, 2013-12-13 20:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/40001

[commit-bot: I haz the power, 2013-12-13 21:50:10]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-12-13 22:32:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/40001

[commit-bot: I haz the power, 2013-12-13 23:48:43]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[reveman, 2013-12-14 05:32:58]

https://codereview.chromium.org/111713008/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/111713008/diff/60001/build/common.gypi#newcod...
build/common.gypi:641: ['OS=="mac" or OS=="ios"', {
How this worked before is a mystery to me.

[commit-bot: I haz the power, 2013-12-14 05:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/60001

[commit-bot: I haz the power, 2013-12-14 07:16:09]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-14 07:17:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/60001

[reveman, 2013-12-14 07:42:01]

https://codereview.chromium.org/111713008/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/111713008/diff/60001/build/common.gypi#newcod...
build/common.gypi:641: ['OS=="mac" or OS=="ios"', {
On 2013/12/14 05:32:59, David Reveman wrote:
> How this worked before is a mystery to me.

Ok, looks like we built both discardable_memory_mac.cc and
discardable_memory_emulated.cc on ios before this change and
DiscardableMemory::SupportedNatively symbol was defined twice withiout the
linker complaining!

[commit-bot: I haz the power, 2013-12-14 09:54:34]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-14 15:44:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/60001

[commit-bot: I haz the power, 2013-12-14 18:11:09]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-12-14 20:10:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/111713008/60001

[commit-bot: I haz the power, 2013-12-16 18:02:07]

Change committed as 240924