Add methods to base::SharedMemory to replace common SharedMemoryHandle operations.

Add methods to base::SharedMemory to replace common SharedMemoryHandle operations.

Existing Chrome code assumes that base::SharedMemoryHandle is typedefed to
"int", and that base::SharedMemoryHandle is backed by an fd on POSIX. This CL
adds the static methods ShallowCopyOfHandle() and DeepCopyOfHandle(), which will
replace existing "int" copies and calls to dup().

BUG=466437
Committed: https://crrev.com/8539d85a060f79993354ba68b1fb3c5a66b119ad
Cr-Commit-Position: refs/heads/master@{#332098}

[erikchen, 2015-05-28 20:43:41]

erikchen@chromium.org changed reviewers:
+ thakis@chromium.org

[erikchen, 2015-05-28 20:43:42]

thakis: Please review.

In preparation for some major changes to the implementation of Shared Memory on
Mac, I'm going to be refactoring a lot of existing code. 

(e.g. https://codereview.chromium.org/1154613006/)

[Nico, 2015-05-29 06:50:24]

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:111: #if defined(OS_WIN) || defined(OS_POSIX)
this is just "everywhere", right?

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:115: return handle;
The CL you link to doesn't modify this function. Is it necessary to wrap this,
or can callers just use " = "? Assignment of handles is fairly well understood,
I feel.

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:121: SharedMemoryHandle
SharedMemory::DeepCopyOfHandle(
CloneHandle() maybe?

[erikchen, 2015-05-29 18:30:24]

thakis: PTAL

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:111: #if defined(OS_WIN) || defined(OS_POSIX)
On 2015/05/29 06:50:23, Nico wrote:
> this is just "everywhere", right?

I guess so. Removed preprocessor conditionals.

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:115: return handle;
On 2015/05/29 06:50:24, Nico wrote:
> The CL you link to doesn't modify this function. Is it necessary to wrap this,
> or can callers just use " = "? Assignment of handles is fairly well
understood,
> I feel.

I think it's semantically important to not use assignment of handles, since it
loses information about whether the copy is intended to be shallow or deep. For
example, right now, on POSIX, SharedMemoryHandle is typedefed to FileDescriptor,
and assignment copies the "auto_close" parameter without duping the fd, which
semantically doesn't make any sense. (The existing code happens to work because
locations that make shallow copies of SharedMemoryHandle end up dup()ing the
handle before turning it into a base::SharedMemory.

I updated the definition to fix the "auto_close" bug.

https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
base/memory/shared_memory_posix.cc:121: SharedMemoryHandle
SharedMemory::DeepCopyOfHandle(
On 2015/05/29 06:50:24, Nico wrote:
> CloneHandle() maybe?

If you really want me to change the verb, the canonical verb we use in Chrome is
"Duplicate" when it comes to SharedMemoryHandles. That being said, that verb is
less explicit about shallow/deep copy, which is why I prefer this version.

[Nico, 2015-05-29 22:46:05]

On 2015/05/29 18:30:24, erikchen wrote:
> thakis: PTAL
> 
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> File base/memory/shared_memory_posix.cc (right):
> 
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> base/memory/shared_memory_posix.cc:111: #if defined(OS_WIN) ||
defined(OS_POSIX)
> On 2015/05/29 06:50:23, Nico wrote:
> > this is just "everywhere", right?
> 
> I guess so. Removed preprocessor conditionals.
> 
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> base/memory/shared_memory_posix.cc:115: return handle;
> On 2015/05/29 06:50:24, Nico wrote:
> > The CL you link to doesn't modify this function. Is it necessary to wrap
this,
> > or can callers just use " = "? Assignment of handles is fairly well
> understood,
> > I feel.
> 
> I think it's semantically important to not use assignment of handles, since it
> loses information about whether the copy is intended to be shallow or deep.
For
> example, right now, on POSIX, SharedMemoryHandle is typedefed to
FileDescriptor,
> and assignment copies the "auto_close" parameter without duping the fd, which
> semantically doesn't make any sense. (The existing code happens to work
because
> locations that make shallow copies of SharedMemoryHandle end up dup()ing the
> handle before turning it into a base::SharedMemory.
> 
> I updated the definition to fix the "auto_close" bug.
> 
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> base/memory/shared_memory_posix.cc:121: SharedMemoryHandle
> SharedMemory::DeepCopyOfHandle(
> On 2015/05/29 06:50:24, Nico wrote:
> > CloneHandle() maybe?
> 
> If you really want me to change the verb, the canonical verb we use in Chrome
is
> "Duplicate" when it comes to SharedMemoryHandles. That being said, that verb
is
> less explicit about shallow/deep copy, which is why I prefer this version.

If you want to keep your names, please follow the "methods should be named like
verbs" style guide rule (which probably just means dropping the "Of")

[erikchen, 2015-05-29 22:50:40]

On 2015/05/29 22:46:05, Nico wrote:
> On 2015/05/29 18:30:24, erikchen wrote:
> > thakis: PTAL
> > 
> >
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> > File base/memory/shared_memory_posix.cc (right):
> > 
> >
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> > base/memory/shared_memory_posix.cc:111: #if defined(OS_WIN) ||
> defined(OS_POSIX)
> > On 2015/05/29 06:50:23, Nico wrote:
> > > this is just "everywhere", right?
> > 
> > I guess so. Removed preprocessor conditionals.
> > 
> >
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> > base/memory/shared_memory_posix.cc:115: return handle;
> > On 2015/05/29 06:50:24, Nico wrote:
> > > The CL you link to doesn't modify this function. Is it necessary to wrap
> this,
> > > or can callers just use " = "? Assignment of handles is fairly well
> > understood,
> > > I feel.
> > 
> > I think it's semantically important to not use assignment of handles, since
it
> > loses information about whether the copy is intended to be shallow or deep.
> For
> > example, right now, on POSIX, SharedMemoryHandle is typedefed to
> FileDescriptor,
> > and assignment copies the "auto_close" parameter without duping the fd,
which
> > semantically doesn't make any sense. (The existing code happens to work
> because
> > locations that make shallow copies of SharedMemoryHandle end up dup()ing the
> > handle before turning it into a base::SharedMemory.
> > 
> > I updated the definition to fix the "auto_close" bug.
> > 
> >
>
https://codereview.chromium.org/1164473003/diff/1/base/memory/shared_memory_p...
> > base/memory/shared_memory_posix.cc:121: SharedMemoryHandle
> > SharedMemory::DeepCopyOfHandle(
> > On 2015/05/29 06:50:24, Nico wrote:
> > > CloneHandle() maybe?
> > 
> > If you really want me to change the verb, the canonical verb we use in
Chrome
> is
> > "Duplicate" when it comes to SharedMemoryHandles. That being said, that verb
> is
> > less explicit about shallow/deep copy, which is why I prefer this version.
> 
> If you want to keep your names, please follow the "methods should be named
like
> verbs" style guide rule (which probably just means dropping the "Of")

thakis: PTAL

Done. Next April I'll change Chrome to use ObjC style rules so that methods that
return objects must be named like a noun, and have at least 87 characters.

[Nico, 2015-05-29 22:52:02]

lgtm

[erikchen, 2015-05-29 22:53:20]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-05-29 22:53:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164473003/60001

[erikchen, 2015-05-29 23:40:41]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2015-05-29 23:44:16]

thakis: PTAL

I realized that "#if defined(OS_POSIX)" has no effect in
"shared_memory_posix.cc", and added the relevant logic to shared_memory_win.cc.

[Nico, 2015-05-29 23:53:09]

still lgtm

[erikchen, 2015-05-29 23:55:16]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-05-29 23:55:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164473003/80001

[commit-bot: I haz the power, 2015-05-30 01:49:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-30 01:50:08]

Patchset 5 (id:??) landed as
https://crrev.com/8539d85a060f79993354ba68b1fb3c5a66b119ad
Cr-Commit-Position: refs/heads/master@{#332098}