Support using a Mojo message pipe in a ChannelHandle.

Support using a Mojo message pipe in a ChannelHandle.

BUG=604282
Committed: https://crrev.com/3618252c6bdc743f469b163c5b813d92ea6a064f
Cr-Commit-Position: refs/heads/master@{#402121}

[Anand Mistry (off Chromium), 2016-06-15 04:52:16]

amistry@chromium.org changed reviewers:
+ rockot@chromium.org, tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-06-23 15:03:16]

Sorry, I don't know how I missed this. Note that I think tsepez is OOO so you
may want to ping a different security owner for this one.

LGTM

[Anand Mistry (off Chromium), 2016-06-23 20:16:25]

amistry@chromium.org changed reviewers:
+ dcheng@chromium.org
- tsepez@chromium.org

[Anand Mistry (off Chromium), 2016-06-23 20:16:26]

-tsepez, +dcheng for security.

[dcheng, 2016-06-24 07:50:48]

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1021: static_cast<IPC::Message*>(m), p.mojo_handle);
Why does this need the explicit static_cast? Normally, WriteParam / ReadParam /
LogParam are the canonical way to delegate.

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_mojo_param_tra...
File ipc/ipc_mojo_param_traits.cc (right):

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_mojo_param_tra...
ipc/ipc_mojo_param_traits.cc:14: sizer->AddBool();
Nit: GetParamSize(sizer, p.is_valid())

[Anand Mistry (off Chromium), 2016-06-27 00:46:24]

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1021: static_cast<IPC::Message*>(m), p.mojo_handle);
On 2016/06/24 07:50:48, dcheng wrote:
> Why does this need the explicit static_cast? Normally, WriteParam / ReadParam
/
> LogParam are the canonical way to delegate.

Because ParamTraits<> functions operate on base::Pickle whereas the MessagePipe
one use IPC::Message. The cast is needed because C++ templates use exact type
matching and don't recognise subclasses.

I've decided to switch the MessagePipe param trait to use base::Pickle instead,
which gets rid of this cast and makes the param trait look like all the others.

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_mojo_param_tra...
File ipc/ipc_mojo_param_traits.cc (right):

https://codereview.chromium.org/2047233002/diff/200001/ipc/ipc_mojo_param_tra...
ipc/ipc_mojo_param_traits.cc:14: sizer->AddBool();
On 2016/06/24 07:50:48, dcheng wrote:
> Nit: GetParamSize(sizer, p.is_valid())

Done.

[dcheng, 2016-06-27 01:55:32]

lgtm with nits

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1041:
ParamTraits<mojo::MessagePipeHandle>::Log(p.mojo_handle, l);
Can this just be LogParam?

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
File ipc/ipc_message_utils_unittest.cc (right):

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
ipc/ipc_message_utils_unittest.cc:154: EXPECT_EQ(result_handle.socket.fd, -1);
EXPECT_EQ(expected, actual) is the convention, I think.

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
ipc/ipc_message_utils_unittest.cc:156: EXPECT_TRUE(result_handle.pipe.handle ==
NULL);
EXPECT_EQ(nullptr, result_handle.pipe.handle)

[Anand Mistry (off Chromium), 2016-06-27 02:40:43]

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils....
ipc/ipc_message_utils.cc:1041:
ParamTraits<mojo::MessagePipeHandle>::Log(p.mojo_handle, l);
On 2016/06/27 01:55:32, dcheng wrote:
> Can this just be LogParam?

Done.

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
File ipc/ipc_message_utils_unittest.cc (right):

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
ipc/ipc_message_utils_unittest.cc:154: EXPECT_EQ(result_handle.socket.fd, -1);
On 2016/06/27 01:55:32, dcheng wrote:
> EXPECT_EQ(expected, actual) is the convention, I think.

Done.

Funnily enough, I've had a post-it note on my monitor (one of three) for the
past ~7 years that says exactly that.

https://codereview.chromium.org/2047233002/diff/220001/ipc/ipc_message_utils_...
ipc/ipc_message_utils_unittest.cc:156: EXPECT_TRUE(result_handle.pipe.handle ==
NULL);
On 2016/06/27 01:55:32, dcheng wrote:
> EXPECT_EQ(nullptr, result_handle.pipe.handle)

Done. But note, the field is a HANDLE and I don't know whether the compiler will
allow this comparison. We'll see how the tryjob fairs. I may need to undo this
if the compiler doesn't allow it.

[Anand Mistry (off Chromium), 2016-06-27 05:23:44]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-06-27 05:23:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2047233002/#ps240001
(title: "comments")

[commit-bot: I haz the power, 2016-06-27 05:23:52]

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, 2016-06-27 06:35:02]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-27 06:36:34]

Description was changed from

==========
Support using a Mojo message pipe in a ChannelHandle.

BUG=604282
==========

to

==========
Support using a Mojo message pipe in a ChannelHandle.

BUG=604282

Committed: https://crrev.com/3618252c6bdc743f469b163c5b813d92ea6a064f
Cr-Commit-Position: refs/heads/master@{#402121}
==========

[commit-bot: I haz the power, 2016-06-27 06:36:35]

Patchset 13 (id:??) landed as
https://crrev.com/3618252c6bdc743f469b163c5b813d92ea6a064f
Cr-Commit-Position: refs/heads/master@{#402121}