Get rid of SharedMemory::GiveToProcess.

Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
TBR=avi@chromium.org

Review-Url: https://codereview.chromium.org/2847033003
Cr-Commit-Position: refs/heads/master@{#468766}
Committed: https://chromium.googlesource.com/chromium/src/+/63840887ae819fb18b2103ac939db69e35ffe111

[erikchen, 2017-04-28 01:45:23]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 01:46:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 01:53:23]

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

[commit-bot: I haz the power, 2017-04-28 01:53:23]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-04-28 21:09:47]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 21:10:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-04-28 21:12:01]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 21:12:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-04-28 21:13:07]

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

[erikchen, 2017-04-28 21:13:08]

thakis: Please review.

[commit-bot: I haz the power, 2017-04-28 21:33:14]

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

[commit-bot: I haz the power, 2017-04-28 21:33:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-04-29 17:19:41]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 17:19:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-04-29 17:28:09]

Description was changed from

==========
Get rid of SharedMemory::GiveToProcess.

All handle transportation should be done by the IPC subsystem.

BUG=716206
==========

to

==========
Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
==========

[commit-bot: I haz the power, 2017-04-29 17:37:32]

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

[commit-bot: I haz the power, 2017-04-29 17:37:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2017-04-29 17:43:32]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 17:43:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 18:24:46]

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

[commit-bot: I haz the power, 2017-04-29 18:24:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[erikchen, 2017-04-29 21:46:59]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 21:47:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 22:25:10]

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

[commit-bot: I haz the power, 2017-04-29 22:25:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[erikchen, 2017-04-29 23:05:57]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-29 23:06:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-29 23:46:05]

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

[commit-bot: I haz the power, 2017-04-29 23:46:06]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2017-05-01 03:46:16]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 03:46:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 04:52:23]

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

[commit-bot: I haz the power, 2017-05-01 04:52:23]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-05-02 01:54:50]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 01:55:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 03:39:08]

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

[commit-bot: I haz the power, 2017-05-02 03:39:09]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-02 15:58:46]

lgtm, but the Dup() / Close() / Unmap() sequences in latter files feel strange.

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle_win.cc (right):

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:43: // Duplicates the underlying OS
resource.
nit: useless comment

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:51: if (success) {
nit:

  if (!success)
    return SharedMemoryHandle();
  ...happy case...

keeps the happy path unindented, which imho is a nice thing to aim for

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (left):

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:193: FALSE, DUPLICATE_SAME_ACCESS);
Oh, you just moved this around. Nits still apply, I suppose.

https://codereview.chromium.org/2847033003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2847033003/diff/140001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:244:
result->font_data.Close();
This code looks kind of weird to me. Don't you really want something like

handle = result->font_data.Release()

or

handle = std::move(result->font_data)

?

[erikchen, 2017-05-02 18:45:57]

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
File base/memory/shared_memory_handle_win.cc (right):

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:43: // Duplicates the underlying OS
resource.
On 2017/05/02 15:58:45, Nico wrote:
> nit: useless comment

Done.

https://codereview.chromium.org/2847033003/diff/140001/base/memory/shared_mem...
base/memory/shared_memory_handle_win.cc:51: if (success) {
On 2017/05/02 15:58:45, Nico wrote:
> nit:
> 
>   if (!success)
>     return SharedMemoryHandle();
>   ...happy case...
> 
> keeps the happy path unindented, which imho is a nice thing to aim for

I was wondering if you would nit on this...and bet that you wouldn't. =P

Done.

https://codereview.chromium.org/2847033003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2847033003/diff/140001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:244:
result->font_data.Close();
On 2017/05/02 15:58:45, Nico wrote:
> This code looks kind of weird to me. Don't you really want something like
> 
> handle = result->font_data.Release()
> 
> or
> 
> handle = std::move(result->font_data)
> 
> ?

[Duplicate changes the ownership_passes_to_ipc member of the underlying handle].


We could add a new public method to base::SharedMemory, but there are only two
consumers and honestly this *is* a really weird use case. The original semantics
of GiveToProcess were to synchronously "transfer" a handle from one process to
another. 

I don't want to make it easy for consumers of base::SharedMemory to do this
really weird thing.

[erikchen, 2017-05-02 18:46:03]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 18:46:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-05-02 18:46:51]

Description was changed from

==========
Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
==========

to

==========
Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
TBR=avi@chromium.org
==========

[erikchen, 2017-05-02 18:47:09]

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

[erikchen, 2017-05-02 18:47:11]

avi: TBR-ing you for content/ changes in this refactor.

[erikchen, 2017-05-02 19:19:00]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-05-02 19:19:00]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-02 19:19:01]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2847033003/#ps160001
(title: "Comments from thakis.")

[commit-bot: I haz the power, 2017-05-02 19:19:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-05-02 19:46:08]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2017-05-02 19:46:21]

not lgtm

https://codereview.chromium.org/2847033003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (left):

https://codereview.chromium.org/2847033003/diff/160001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Don't remove copyright lines.

[erikchen, 2017-05-02 19:48:40]

avi: PTAL

https://codereview.chromium.org/2847033003/diff/160001/content/browser/render...
File content/browser/renderer_host/render_message_filter.cc (left):

https://codereview.chromium.org/2847033003/diff/160001/content/browser/render...
content/browser/renderer_host/render_message_filter.cc:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2017/05/02 19:46:21, Avi (ping after 24h) wrote:
> Don't remove copyright lines.

whoops, not sure how that happened. Fixed.

[Avi (use Gerrit), 2017-05-02 19:50:36]

lgtm stamp

[erikchen, 2017-05-02 19:52:45]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-02 19:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2847033003/#ps180001
(title: "Comments from avi.")

[commit-bot: I haz the power, 2017-05-02 19:53:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 20:52:35]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493754765550630,
"parent_rev": "5e17a9358f03488e71cba7c4b6786afba95fed6e", "commit_rev":
"63840887ae819fb18b2103ac939db69e35ffe111"}

[commit-bot: I haz the power, 2017-05-02 20:52:46]

Description was changed from

==========
Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
TBR=avi@chromium.org
==========

to

==========
Get rid of SharedMemory::GiveToProcess.

This CL is a refactor with no intended behavior change.

The method existed to allow a process to directly give a handle to another
process, before the IPC subsystem supported handle transport. Now that handle
transport is supported, all instances of directly giving a handle have already
been removed, and this is just a clean up.

BUG=716206
TBR=avi@chromium.org

Review-Url: https://codereview.chromium.org/2847033003
Cr-Commit-Position: refs/heads/master@{#468766}
Committed:
https://chromium.googlesource.com/chromium/src/+/63840887ae819fb18b2103ac939d...
==========

[commit-bot: I haz the power, 2017-05-02 20:52:47]

Committed patchset #11 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/63840887ae819fb18b2103ac939d...