services/ui: Get rid of IPC::ChannelHandle type converter

services/ui: Get rid of IPC::ChannelHandle type converter

BUG=625557
Committed: https://crrev.com/a7280ca5addaf050ea058416990a3eb1a12aba5e
Cr-Commit-Position: refs/heads/master@{#412832}

[Peng, 2016-08-17 16:07:32]

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

[commit-bot: I haz the power, 2016-08-17 16:07:56]

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

[Peng, 2016-08-17 16:08:49]

penghuang@chromium.org changed reviewers:
+ sky@chromium.org

[Peng, 2016-08-17 16:08:50]

Hi Scott, PTAL. Thanks.

[commit-bot: I haz the power, 2016-08-17 16:18:27]

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

[commit-bot: I haz the power, 2016-08-17 16:18:28]

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-...)

[Peng, 2016-08-17 17:45:28]

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

[commit-bot: I haz the power, 2016-08-17 17:45:57]

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, 2016-08-17 18:33:01]

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

[commit-bot: I haz the power, 2016-08-17 18:33:02]

Dry run: This issue passed the CQ dry run.

[sky, 2016-08-17 18:56:21]

LGTM

[Peng, 2016-08-17 19:02:46]

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

[Peng, 2016-08-17 19:02:46]

Hi Daniel, PTAL. Thanks.

[dcheng, 2016-08-17 19:38:29]

https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service_impl.cc (right):

https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service_impl.cc:21:
mojo::ScopedMessagePipeHandle(channel_handle.mojo_handle),
Does ChannelHandle not have strong ownership of its mojo_handle? I'm not
familiar with this part of the system, so I'm trying to understand how it works.

https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:183:
IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
Nit: I think std::move() might work here.

[Peng, 2016-08-17 19:59:17]

https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service_impl.cc (right):

https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service_impl.cc:21:
mojo::ScopedMessagePipeHandle(channel_handle.mojo_handle),
On 2016/08/17 19:38:29, dcheng wrote:
> Does ChannelHandle not have strong ownership of its mojo_handle? I'm not
> familiar with this part of the system, so I'm trying to understand how it
works.

IPC::ChannelHandle is more like a data struct instead of class. It doesn't
maintain the life cycle of its members. It is just used for passing data I
think.

In here, the mojo::ScopedMessagePipleHandle() will take the ownership of the
channel_handle.mojo_handle.

https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu_service.cc (right):

https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/gpu_service.cc:183:
IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
On 2016/08/17 19:38:29, dcheng wrote:
> Nit: I think std::move() might work here.

Just tried. std::move() doesn't work. It is because IP::ChannelHandle() only
accepts mojo::MessagePipeHandle, it dons't
accept mojo::ScopedMessagePipeHandle.

[dcheng, 2016-08-17 21:52:36]

On 2016/08/17 19:59:17, Peng wrote:
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> File services/ui/gpu/gpu_service_impl.cc (right):
> 
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> services/ui/gpu/gpu_service_impl.cc:21:
> mojo::ScopedMessagePipeHandle(channel_handle.mojo_handle),
> On 2016/08/17 19:38:29, dcheng wrote:
> > Does ChannelHandle not have strong ownership of its mojo_handle? I'm not
> > familiar with this part of the system, so I'm trying to understand how it
> works.
> 
> IPC::ChannelHandle is more like a data struct instead of class. It doesn't
> maintain the life cycle of its members. It is just used for passing data I
> think.
> 
> In here, the mojo::ScopedMessagePipleHandle() will take the ownership of the
> channel_handle.mojo_handle.
> 
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> File services/ui/public/cpp/gpu_service.cc (right):
> 
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> services/ui/public/cpp/gpu_service.cc:183:
> IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
> On 2016/08/17 19:38:29, dcheng wrote:
> > Nit: I think std::move() might work here.
> 
> Just tried. std::move() doesn't work. It is because IP::ChannelHandle() only
> accepts mojo::MessagePipeHandle, it dons't
> accept mojo::ScopedMessagePipeHandle.

I don't think I understand the lifetime of the mojo_handle associated with
ChannelHandle. In some places, it's "owned" (since we set it to the value
released from a ScopedMessagePipeHandle). In other places, it's not really owned
(since we steal ownership with a ScopdeMessagePipeHandle... is this an accurate
summary of the situation? =(

[Ken Rockot(use gerrit already), 2016-08-17 23:19:14]

On 2016/08/17 at 21:52:36, dcheng wrote:
> On 2016/08/17 19:59:17, Peng wrote:
> >
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> > File services/ui/gpu/gpu_service_impl.cc (right):
> > 
> >
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> > services/ui/gpu/gpu_service_impl.cc:21:
> > mojo::ScopedMessagePipeHandle(channel_handle.mojo_handle),
> > On 2016/08/17 19:38:29, dcheng wrote:
> > > Does ChannelHandle not have strong ownership of its mojo_handle? I'm not
> > > familiar with this part of the system, so I'm trying to understand how it
> > works.
> > 
> > IPC::ChannelHandle is more like a data struct instead of class. It doesn't
> > maintain the life cycle of its members. It is just used for passing data I
> > think.
> > 
> > In here, the mojo::ScopedMessagePipleHandle() will take the ownership of the
> > channel_handle.mojo_handle.
> > 
> >
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> > File services/ui/public/cpp/gpu_service.cc (right):
> > 
> >
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> > services/ui/public/cpp/gpu_service.cc:183:
> > IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
> > On 2016/08/17 19:38:29, dcheng wrote:
> > > Nit: I think std::move() might work here.
> > 
> > Just tried. std::move() doesn't work. It is because IP::ChannelHandle() only
> > accepts mojo::MessagePipeHandle, it dons't
> > accept mojo::ScopedMessagePipeHandle.
> 
> I don't think I understand the lifetime of the mojo_handle associated with
ChannelHandle. In some places, it's "owned" (since we set it to the value
released from a ScopedMessagePipeHandle). In other places, it's not really owned
(since we steal ownership with a ScopdeMessagePipeHandle... is this an accurate
summary of the situation? =(

ChannelHandle does not own the thing it references. This is unfortunate and I
don't have any context for why it works this way, but there is already precedent
for it -  consider that on POSIX it can also hold a base::FileDescriptor with no
lifetime management whatsoever.

[dcheng, 2016-08-17 23:21:59]

On 2016/08/17 23:19:14, Ken Rockot wrote:
> On 2016/08/17 at 21:52:36, dcheng wrote:
> > On 2016/08/17 19:59:17, Peng wrote:
> > >
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> > > File services/ui/gpu/gpu_service_impl.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/gpu/gpu_ser...
> > > services/ui/gpu/gpu_service_impl.cc:21:
> > > mojo::ScopedMessagePipeHandle(channel_handle.mojo_handle),
> > > On 2016/08/17 19:38:29, dcheng wrote:
> > > > Does ChannelHandle not have strong ownership of its mojo_handle? I'm not
> > > > familiar with this part of the system, so I'm trying to understand how
it
> > > works.
> > > 
> > > IPC::ChannelHandle is more like a data struct instead of class. It doesn't
> > > maintain the life cycle of its members. It is just used for passing data I
> > > think.
> > > 
> > > In here, the mojo::ScopedMessagePipleHandle() will take the ownership of
the
> > > channel_handle.mojo_handle.
> > > 
> > >
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> > > File services/ui/public/cpp/gpu_service.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2247183005/diff/20001/services/ui/public/cpp/...
> > > services/ui/public/cpp/gpu_service.cc:183:
> > > IPC::ChannelHandle(channel_handle.release()), &shutdown_event_,
> > > On 2016/08/17 19:38:29, dcheng wrote:
> > > > Nit: I think std::move() might work here.
> > > 
> > > Just tried. std::move() doesn't work. It is because IP::ChannelHandle()
only
> > > accepts mojo::MessagePipeHandle, it dons't
> > > accept mojo::ScopedMessagePipeHandle.
> > 
> > I don't think I understand the lifetime of the mojo_handle associated with
> ChannelHandle. In some places, it's "owned" (since we set it to the value
> released from a ScopedMessagePipeHandle). In other places, it's not really
owned
> (since we steal ownership with a ScopdeMessagePipeHandle... is this an
accurate
> summary of the situation? =(
> 
> ChannelHandle does not own the thing it references. This is unfortunate and I
> don't have any context for why it works this way, but there is already
precedent
> for it -  consider that on POSIX it can also hold a base::FileDescriptor with
no
> lifetime management whatsoever.

Yes, I'm familiar with base::FileDescriptor. The reason it (and
IPC::ChannelHandle) are used this way is due to limitations of the current IPC
system (which passes everything around as const T&).

Here, though, we're not constrained by that, so I would strongly prefer if we'd
pass it by mutable pointer to indicate that mischief is in progress. Once we
don't have legacy IPC anymore, we might be able to revisit IPC::ChannelHandle
and fix the scoping issues.

[Fady Samuel, 2016-08-17 23:24:14]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-08-17 23:24:14]

drive-by: This is super cool! IPC::ChannelHandles ~= MessagePipe now so we can
trivially pass them over mojo! w00t

[Peng, 2016-08-17 23:55:04]

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

[commit-bot: I haz the power, 2016-08-17 23:55:29]

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

[dcheng, 2016-08-18 00:06:01]

https://codereview.chromium.org/2247183005/diff/40001/services/ui/gpu/gpu_ser...
File services/ui/gpu/gpu_service_mus.cc (right):

https://codereview.chromium.org/2247183005/diff/40001/services/ui/gpu/gpu_ser...
services/ui/gpu/gpu_service_mus.cc:79: callback.Run(-1, nullptr);
I would probably just throw a IPC::ChannelHandle on the stack here and pass the
address to the callback. Then we shouldn't need to handle the null case in
either of the callbacks, simplifying everything overall.

[Peng, 2016-08-18 00:29:20]

On 2016/08/18 00:06:01, dcheng wrote:
>
https://codereview.chromium.org/2247183005/diff/40001/services/ui/gpu/gpu_ser...
> File services/ui/gpu/gpu_service_mus.cc (right):
> 
>
https://codereview.chromium.org/2247183005/diff/40001/services/ui/gpu/gpu_ser...
> services/ui/gpu/gpu_service_mus.cc:79: callback.Run(-1, nullptr);
> I would probably just throw a IPC::ChannelHandle on the stack here and pass
the
> address to the callback. Then we shouldn't need to handle the null case in
> either of the callbacks, simplifying everything overall.

I updated my CL to use IPC::ChannelHandle as few as possible.  PTAL.

[dcheng, 2016-08-18 00:34:54]

LGTM, thanks. I like this a lot better.

[Peng, 2016-08-18 13:32:41]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2016-08-18 13:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2247183005/#ps60001
(title: "Fix review issues")

[commit-bot: I haz the power, 2016-08-18 13:33:00]

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-08-18 15:20:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-18 15:22:43]

Description was changed from

==========
services/ui: Get rid of IPC::ChannelHandle type converter

BUG=625557
==========

to

==========
services/ui: Get rid of IPC::ChannelHandle type converter

BUG=625557

Committed: https://crrev.com/a7280ca5addaf050ea058416990a3eb1a12aba5e
Cr-Commit-Position: refs/heads/master@{#412832}
==========

[commit-bot: I haz the power, 2016-08-18 15:22:44]

Patchset 4 (id:??) landed as
https://crrev.com/a7280ca5addaf050ea058416990a3eb1a12aba5e
Cr-Commit-Position: refs/heads/master@{#412832}