Add SharedMemory version of PersistentMemoryAllocator.

Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019
Committed: https://crrev.com/5451c58652c582c6f144d155c692f7b312f5708e
Cr-Commit-Position: refs/heads/master@{#375217}

[bcwhite, 2016-02-04 19:21:58]

Description was changed from

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

BUG=546019
==========

to

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019
==========

[bcwhite, 2016-02-04 19:24:02]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[bcwhite, 2016-02-04 22:49:04]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2016-02-05 21:54:02]

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:664: scoped_ptr<SharedMemory> shmem,
Nit: Make param consistent with header.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:318:
SharedPersistentMemoryAllocator(scoped_ptr<SharedMemory> mem, uint64_t id,
Nit: 1 param per line.

|mem| -> |memory|

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:322: SharedMemory* GetSharedMemory()
{ return shared_memory_.get(); }
Nit: Either a method is trivial and should be named in hacker_style() or it is
not trivial and should not be inlined.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:328: static bool
IsSharedMemoryAcceptable(const SharedMemory& mem);
Hmm, it's hard to understand Acceptable without reading the comment. How about,
IsSharedMemoryValid()?

Also, |mem| -> |memory|.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:331: scoped_ptr<SharedMemory>
shared_memory_;
DISALLOW_COPY_AND_ASSIGN()

[bcwhite, 2016-02-06 04:25:45]

Patchset #3 (id:60001) has been deleted

[bcwhite, 2016-02-06 04:26:12]

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:664: scoped_ptr<SharedMemory> shmem,
On 2016/02/05 21:54:02, Alexei Svitkine (very slow) wrote:
> Nit: Make param consistent with header.

Done.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:318:
SharedPersistentMemoryAllocator(scoped_ptr<SharedMemory> mem, uint64_t id,
On 2016/02/05 21:54:02, Alexei Svitkine (very slow) wrote:
> Nit: 1 param per line.
> 
> |mem| -> |memory|

Done.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:322: SharedMemory* GetSharedMemory()
{ return shared_memory_.get(); }
On 2016/02/05 21:54:02, Alexei Svitkine (very slow) wrote:
> Nit: Either a method is trivial and should be named in hacker_style() or it is
> not trivial and should not be inlined.

Done.

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:328: static bool
IsSharedMemoryAcceptable(const SharedMemory& mem);
On 2016/02/05 21:54:02, Alexei Svitkine (very slow) wrote:
> Hmm, it's hard to understand Acceptable without reading the comment. How
about,
> IsSharedMemoryValid()?

It doesn't check that it's valid, just that it's acceptable to pass in without
causing a crash.  The check for "valid" would be too complex

https://codereview.chromium.org/1660143009/diff/40001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:331: scoped_ptr<SharedMemory>
shared_memory_;
On 2016/02/05 21:54:02, Alexei Svitkine (very slow) wrote:
> DISALLOW_COPY_AND_ASSIGN()

I always forget that...  below, too.

[Alexei Svitkine (slow), 2016-02-09 19:58:44]

lgtm

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:673: }
Nit: run git cl format?

I am assuming whatever it provides would format line 670 and line 672
consistently - in terms of whether the } is on the next line or not.

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:328: static bool
IsSharedMemoryAcceptable(const SharedMemory& memory);
I understand why you don't want to use IsValid() but I find "Acceptable" very
vague and hard to understand without reading the comment.

But looking at the impl, I guess you use "Acceptable" already elsewhere, so I
guess it should be consistent. But I would have suggesting something like
"SanityCheckSharedMemory()" or some other wording that's more precise than
"Acceptable".

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:354:
DISALLOW_COPY_AND_ASSIGN(FilePersistentMemoryAllocator);
Nit: Put an empty line above these lines.

[bcwhite, 2016-02-10 16:43:53]

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.cc (right):

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.cc:673: }
On 2016/02/09 19:58:44, Alexei Svitkine wrote:
> Nit: run git cl format?
> 
> I am assuming whatever it provides would format line 670 and line 672
> consistently - in terms of whether the } is on the next line or not.

Done.

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
File base/metrics/persistent_memory_allocator.h (right):

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:328: static bool
IsSharedMemoryAcceptable(const SharedMemory& memory);
On 2016/02/09 19:58:44, Alexei Svitkine wrote:
> I understand why you don't want to use IsValid() but I find "Acceptable" very
> vague and hard to understand without reading the comment.
> 
> But looking at the impl, I guess you use "Acceptable" already elsewhere, so I
> guess it should be consistent. But I would have suggesting something like
> "SanityCheckSharedMemory()" or some other wording that's more precise than
> "Acceptable".

Acknowledged.

https://codereview.chromium.org/1660143009/diff/80001/base/metrics/persistent...
base/metrics/persistent_memory_allocator.h:354:
DISALLOW_COPY_AND_ASSIGN(FilePersistentMemoryAllocator);
On 2016/02/09 19:58:44, Alexei Svitkine wrote:
> Nit: Put an empty line above these lines.

Done.

[bcwhite, 2016-02-11 15:08:17]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-02-11 15:08:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1660143009/#ps100001
(title: "minor formatting fixes")

[commit-bot: I haz the power, 2016-02-11 15:08:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660143009/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660143009/100001

[commit-bot: I haz the power, 2016-02-11 15:13:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-11 15:13:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bcwhite, 2016-02-12 17:34:53]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-02-12 17:34:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1660143009/#ps140001
(title: "rebased")

[commit-bot: I haz the power, 2016-02-12 17:35:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1660143009/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1660143009/140001

[commit-bot: I haz the power, 2016-02-12 18:47:24]

Description was changed from

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019
==========

to

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019
==========

[commit-bot: I haz the power, 2016-02-12 18:47:25]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:44:06]

Description was changed from

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019
==========

to

==========
Add SharedMemory version of PersistentMemoryAllocator.

This is a simple wrapper around PMA that knows how to
interface with the SharedMemory class.

Also changed interface of File... to reflect that it takes
ownership of the passed object.

BUG=546019

Committed: https://crrev.com/5451c58652c582c6f144d155c692f7b312f5708e
Cr-Commit-Position: refs/heads/master@{#375217}
==========

[commit-bot: I haz the power, 2016-02-16 22:44:06]

Patchset 6 (id:??) landed as
https://crrev.com/5451c58652c582c6f144d155c692f7b312f5708e
Cr-Commit-Position: refs/heads/master@{#375217}