Changing multiprocess test SpawnChild to return a struct.

Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
TBR=sky@chromium.org,jochen@chromium.org,jam@chromium.org,rockot@chromium.org,jschuh@chromium.org

Review-Url: https://codereview.chromium.org/2733323002
Cr-Commit-Position: refs/heads/master@{#456756}
Committed: https://chromium.googlesource.com/chromium/src/+/87c322b0dc2f4a7595fd288ab0c529d4aa1aafd9

[Jay Civelli, 2017-03-07 23:55:41]

Patchset #1 (id:1) has been deleted

[Jay Civelli, 2017-03-07 23:55:50]

Patchset #1 (id:20001) has been deleted

[Jay Civelli, 2017-03-07 23:55:58]

Patchset #1 (id:40001) has been deleted

[Jay Civelli, 2017-03-07 23:56:06]

Patchset #1 (id:60001) has been deleted

[Jay Civelli, 2017-03-08 00:13:53]

Description was changed from

==========
Adding a side channel to add parcelable support to Mojo IPC channels.

BUG=
==========

to

==========
Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
==========

[Jay Civelli, 2017-03-08 00:36:24]

Patchset #1 (id:80001) has been deleted

[Jay Civelli, 2017-03-08 00:36:31]

Patchset #1 (id:100001) has been deleted

[Jay Civelli, 2017-03-08 00:37:09]

jcivelli@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Jay Civelli, 2017-03-08 00:38:47]

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

[commit-bot: I haz the power, 2017-03-08 00:39:46]

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-03-08 00:51:03]

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

[commit-bot: I haz the power, 2017-03-08 00:51:04]

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

[Jay Civelli, 2017-03-08 17:50:36]

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

[commit-bot: I haz the power, 2017-03-08 17:51: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-03-08 19:06:23]

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

[commit-bot: I haz the power, 2017-03-08 19:06:24]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-03-08 21:25:08]

jcivelli@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-03-08 22:53:44]

It seems a bit unusual to std::move() the value out, since this partially
consumes the result struct. If the struct is ever passed somewhere else, the
struct is only partially valid at that point. How about just referencing the
field directly?

https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
File base/debug/stack_trace_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
base/debug/stack_trace_unittest.cc:175: Process child =
std::move(spawn_result.process);
Strictly speaking, this seems unnecessary, we could just use the value in the
struct as-is, right?

[Jay Civelli, 2017-03-08 23:12:40]

On 2017/03/08 22:53:44, dcheng wrote:
> It seems a bit unusual to std::move() the value out, since this partially
> consumes the result struct. If the struct is ever passed somewhere else, the
> struct is only partially valid at that point. How about just referencing the
> field directly?
> 
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> File base/debug/stack_trace_unittest.cc (right):
> 
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> base/debug/stack_trace_unittest.cc:175: Process child =
> std::move(spawn_result.process);
> Strictly speaking, this seems unnecessary, we could just use the value in the
> struct as-is, right?

Right, but sometimes we actually consume the process
(base/files/file_locking_unittest.cc for example).
How about I make SpawnChildResult a class with a TakeProcess() method? Still
leaves the result potentially incomplete but it makes it clearer that this is
what happens.

[jcivelli, 2017-03-10 16:58:56]

On 2017/03/08 23:12:40, Jay Civelli wrote:
> On 2017/03/08 22:53:44, dcheng wrote:
> > It seems a bit unusual to std::move() the value out, since this partially
> > consumes the result struct. If the struct is ever passed somewhere else, the
> > struct is only partially valid at that point. How about just referencing the
> > field directly?
> > 
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > File base/debug/stack_trace_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > base/debug/stack_trace_unittest.cc:175: Process child =
> > std::move(spawn_result.process);
> > Strictly speaking, this seems unnecessary, we could just use the value in
the
> > struct as-is, right?
> 
> Right, but sometimes we actually consume the process
> (base/files/file_locking_unittest.cc for example).
> How about I make SpawnChildResult a class with a TakeProcess() method? Still
> leaves the result potentially incomplete but it makes it clearer that this is
> what happens.

Any thoughts dcheng@?

[dcheng, 2017-03-10 18:37:52]

On 2017/03/10 16:58:56, jcivelli wrote:
> On 2017/03/08 23:12:40, Jay Civelli wrote:
> > On 2017/03/08 22:53:44, dcheng wrote:
> > > It seems a bit unusual to std::move() the value out, since this partially
> > > consumes the result struct. If the struct is ever passed somewhere else,
the
> > > struct is only partially valid at that point. How about just referencing
the
> > > field directly?
> > > 
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > File base/debug/stack_trace_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > base/debug/stack_trace_unittest.cc:175: Process child =
> > > std::move(spawn_result.process);
> > > Strictly speaking, this seems unnecessary, we could just use the value in
> the
> > > struct as-is, right?
> > 
> > Right, but sometimes we actually consume the process
> > (base/files/file_locking_unittest.cc for example).
> > How about I make SpawnChildResult a class with a TakeProcess() method? Still
> > leaves the result potentially incomplete but it makes it clearer that this
is
> > what happens.
> 
> Any thoughts dcheng@?

How about we only use std::move() in the locations that actually require this?
For the file locking unittest, it even seems like we could just store the entire
SpawnResult as a member, and that wouldn't be too bad.

[Jay Civelli, 2017-03-10 21:07:39]

On 2017/03/10 18:37:52, dcheng wrote:
> On 2017/03/10 16:58:56, jcivelli wrote:
> > On 2017/03/08 23:12:40, Jay Civelli wrote:
> > > On 2017/03/08 22:53:44, dcheng wrote:
> > > > It seems a bit unusual to std::move() the value out, since this
partially
> > > > consumes the result struct. If the struct is ever passed somewhere else,
> the
> > > > struct is only partially valid at that point. How about just referencing
> the
> > > > field directly?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > > File base/debug/stack_trace_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > > base/debug/stack_trace_unittest.cc:175: Process child =
> > > > std::move(spawn_result.process);
> > > > Strictly speaking, this seems unnecessary, we could just use the value
in
> > the
> > > > struct as-is, right?
> > > 
> > > Right, but sometimes we actually consume the process
> > > (base/files/file_locking_unittest.cc for example).
> > > How about I make SpawnChildResult a class with a TakeProcess() method?
Still
> > > leaves the result potentially incomplete but it makes it clearer that this
> is
> > > what happens.
> > 
> > Any thoughts dcheng@?
> 
> How about we only use std::move() in the locations that actually require this?
> For the file locking unittest, it even seems like we could just store the
entire
> SpawnResult as a member, and that wouldn't be too bad.

Good idea, done in most cases.
What about the name SpawnChildResult. I am starting to think that simply
SpawnChild may be better. Any opinion?

[Jay Civelli, 2017-03-10 21:10:47]

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

[commit-bot: I haz the power, 2017-03-10 21:11:23]

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-03-10 21:14:52]

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

[commit-bot: I haz the power, 2017-03-10 21:14:53]

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

[Jay Civelli, 2017-03-10 21:52:13]

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

[commit-bot: I haz the power, 2017-03-10 21:52:45]

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-03-10 22:01:38]

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

[commit-bot: I haz the power, 2017-03-10 22:01:39]

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

[Jay Civelli, 2017-03-13 16:42:08]

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

[commit-bot: I haz the power, 2017-03-13 16:42:48]

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-03-13 17:40:06]

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

[commit-bot: I haz the power, 2017-03-13 17:40:07]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-03-13 18:09:19]

On 2017/03/10 21:07:39, Jay Civelli wrote:
> On 2017/03/10 18:37:52, dcheng wrote:
> > On 2017/03/10 16:58:56, jcivelli wrote:
> > > On 2017/03/08 23:12:40, Jay Civelli wrote:
> > > > On 2017/03/08 22:53:44, dcheng wrote:
> > > > > It seems a bit unusual to std::move() the value out, since this
> partially
> > > > > consumes the result struct. If the struct is ever passed somewhere
else,
> > the
> > > > > struct is only partially valid at that point. How about just
referencing
> > the
> > > > > field directly?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > > > File base/debug/stack_trace_unittest.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
> > > > > base/debug/stack_trace_unittest.cc:175: Process child =
> > > > > std::move(spawn_result.process);
> > > > > Strictly speaking, this seems unnecessary, we could just use the value
> in
> > > the
> > > > > struct as-is, right?
> > > > 
> > > > Right, but sometimes we actually consume the process
> > > > (base/files/file_locking_unittest.cc for example).
> > > > How about I make SpawnChildResult a class with a TakeProcess() method?
> Still
> > > > leaves the result potentially incomplete but it makes it clearer that
this
> > is
> > > > what happens.
> > > 
> > > Any thoughts dcheng@?
> > 
> > How about we only use std::move() in the locations that actually require
this?
> > For the file locking unittest, it even seems like we could just store the
> entire
> > SpawnResult as a member, and that wouldn't be too bad.
> 
> Good idea, done in most cases.
> What about the name SpawnChildResult. I am starting to think that simply
> SpawnChild may be better. Any opinion?

@dcheng Does that look better?

[dcheng, 2017-03-14 05:28:51]

LGTM

I'm ok with CL as-is, but I'd prefer less moving of the process handle still. I
suggested one alternative to do that, if you think it makes sense.

https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
base/mac/mach_port_broker_unittest.cc:100: base::Process test_child_process =
std::move(spawn_result.process);
I think this std::move is unnecessary. Maybe we can just  alias this into a
base::Process& to have a more convenient name.

https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
base/process/process_unittest.cc:62: Process
process1(std::move(spawn_result.process));
Ditto

https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
File content/browser/memory/memory_coordinator_impl_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
content/browser/memory/memory_coordinator_impl_unittest.cc:538: spawn_result =
SpawnChild("process2");
Maybe make this a separate spawn_result so this can also use the aliasing trick.

https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
File sandbox/mac/bootstrap_sandbox_unittest.mm (right):

https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
sandbox/mac/bootstrap_sandbox_unittest.mm:478: ASSERT_TRUE(process.IsValid());
Ditto

[Jay Civelli, 2017-03-14 16:22:49]

Description was changed from

==========
Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
==========

to

==========
Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
TBR=sky@chromium.org,jochen@chromium.org,jam@chromium.org,rockot@chromium.org...
==========

[Jay Civelli, 2017-03-14 16:27:01]

On 2017/03/14 05:28:51, dcheng wrote:
> LGTM
> 
> I'm ok with CL as-is, but I'd prefer less moving of the process handle still.
I
> suggested one alternative to do that, if you think it makes sense.
Yes, it does make things better. Thanks!

>
https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
> File base/mac/mach_port_broker_unittest.cc (right):
> 
>
https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
> base/mac/mach_port_broker_unittest.cc:100: base::Process test_child_process =
> std::move(spawn_result.process);
> I think this std::move is unnecessary. Maybe we can just  alias this into a
> base::Process& to have a more convenient name.
> 
>
https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
> File base/process/process_unittest.cc (right):
> 
>
https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
> base/process/process_unittest.cc:62: Process
> process1(std::move(spawn_result.process));
> Ditto
> 
>
https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
> File content/browser/memory/memory_coordinator_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
> content/browser/memory/memory_coordinator_impl_unittest.cc:538: spawn_result =
> SpawnChild("process2");
> Maybe make this a separate spawn_result so this can also use the aliasing
trick.
> 
>
https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
> File sandbox/mac/bootstrap_sandbox_unittest.mm (right):
> 
>
https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
> sandbox/mac/bootstrap_sandbox_unittest.mm:478: ASSERT_TRUE(process.IsValid());
> Ditto

[Jay Civelli, 2017-03-14 16:29:11]

jcivelli@chromium.org changed reviewers:
+ jam@chromium.org, jochen@chromium.org, jschuh@chromium.org,
rockot@chromium.org, sky@chromium.org

[Jay Civelli, 2017-03-14 16:29:13]

TBRing for minimal callsite changes:
sky@ from chrome/
jochen@ for components/
jam@ for content/
rockot@ for mojo/
jschuh@ for sandbox/

https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
File base/debug/stack_trace_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/160001/base/debug/stack_trace...
base/debug/stack_trace_unittest.cc:175: Process child =
std::move(spawn_result.process);
On 2017/03/08 22:53:44, dcheng wrote:
> Strictly speaking, this seems unnecessary, we could just use the value in the
> struct as-is, right?

Now using the struct directly.

https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/base/mac/mach_port_bro...
base/mac/mach_port_broker_unittest.cc:100: base::Process test_child_process =
std::move(spawn_result.process);
On 2017/03/14 05:28:51, dcheng wrote:
> I think this std::move is unnecessary. Maybe we can just  alias this into a
> base::Process& to have a more convenient name.

It's short enough that I kept the full spawn_result.process.

https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/base/process/process_u...
base/process/process_unittest.cc:62: Process
process1(std::move(spawn_result.process));
On 2017/03/14 05:28:51, dcheng wrote:
> Ditto

Aliasing here, since it makes the test clearer.

https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
File content/browser/memory/memory_coordinator_impl_unittest.cc (right):

https://codereview.chromium.org/2733323002/diff/220001/content/browser/memory...
content/browser/memory/memory_coordinator_impl_unittest.cc:538: spawn_result =
SpawnChild("process2");
On 2017/03/14 05:28:51, dcheng wrote:
> Maybe make this a separate spawn_result so this can also use the aliasing
trick.

Good idea, done.

https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
File sandbox/mac/bootstrap_sandbox_unittest.mm (right):

https://codereview.chromium.org/2733323002/diff/220001/sandbox/mac/bootstrap_...
sandbox/mac/bootstrap_sandbox_unittest.mm:478: ASSERT_TRUE(process.IsValid());
On 2017/03/14 05:28:51, dcheng wrote:
> Ditto

Done.

[Jay Civelli, 2017-03-14 16:29:35]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-03-14 16:29:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2733323002/#ps260001
(title: "Synced")

[commit-bot: I haz the power, 2017-03-14 16:30:00]

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

[sky, 2017-03-14 17:51:54]

LGTM

[commit-bot: I haz the power, 2017-03-14 17:55:58]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489508975263920,
"parent_rev": "26195437ed623ce9236e47a491018eb38657411c", "commit_rev":
"87c322b0dc2f4a7595fd288ab0c529d4aa1aafd9"}

[commit-bot: I haz the power, 2017-03-14 17:56:44]

Description was changed from

==========
Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
TBR=sky@chromium.org,jochen@chromium.org,jam@chromium.org,rockot@chromium.org...
==========

to

==========
Changing multiprocess test SpawnChild to return a struct instead of a
process. The struct only contains the process for now but will be
augmented in a follow up patch to contain other objects on Android.


BUG=699311
TBR=sky@chromium.org,jochen@chromium.org,jam@chromium.org,rockot@chromium.org...

Review-Url: https://codereview.chromium.org/2733323002
Cr-Commit-Position: refs/heads/master@{#456756}
Committed:
https://chromium.googlesource.com/chromium/src/+/87c322b0dc2f4a7595fd288ab0c5...
==========

[commit-bot: I haz the power, 2017-03-14 17:56:45]

Committed patchset #8 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/87c322b0dc2f4a7595fd288ab0c5...