IPC::ChannelMojo: Introduce IPC::MojoBootstrap for Windows

IPC::ChannelMojo: Introduce IPC::MojoBootstrap for Windows

ChannelMojo doesn't work on Windows with existing implementaion
and this CL fixes it.

On Windows, ChannelHandle isn't immediately usable: The handle has
to be activated through ConnectNamedPipe() windows API, which is done in
its own Connect() handlshaking phase. ChannelMojo didn't Connect()
underlying channel and took the ChannelHandle over so the handle
wasn't activated.

Instead of hijacking underlying ChannelHandle, this CL actually Connect()s
underlying channel, creates a pipe on the server side, send one side of the
pipe to the client process, and use the pipe for the MessagePipe
initialization.

These initialization task is encapsulated behind new MojoBootstrap class.
ChannelMojo creates MojoBootstrap class to get the PlatformHandle which
is already activated and usable.

BUG=377980
TEST=ipc_mojo_bootstrap_unittest.cc, ipc_channel_mojo_unittest.cc
R=viettrungluu@chromium.org, darin@chromium.org, yzshen@chromium.org

Committed: https://crrev.com/54f6f80c3623a6fb9d3049b6f5e0e23b1d76c34d
Cr-Commit-Position: refs/heads/master@{#296248}

[Hajime Morrita, 2014-09-10 22:45:51]

Fixing windows build error

[viettrungluu, 2014-09-15 20:19:44]

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2090:
channel_->OnClientLaunched(child_process_launcher_->GetHandle());
It feels a bit suboptimal to require an extra explicit call....

I wonder since the listener already get a process ID back (in
OnChannelConnected), we couldn't just use that (and OpenProcess) to get a handle
to the child there....

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:182: // Tell the server channel its client process handle.
"the" -> "a"

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:183: // A Channel implementation needs this to establish the
connection
"needs" -> "may need"

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:184: // to the client. This only has to be called on
server-side channel.
Probably be a little more explicit: "This only ..." -> "This should only be
called for a server channel."

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel_proxy.h#...
ipc/ipc_channel_proxy.h:123: // See Channel::OnClientLaunched for detail.
"detail" -> "details"

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_test_base.h
File ipc/ipc_test_base.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_test_base.h#newc...
ipc/ipc_test_base.h:80: bool StartClient(int ipcfd);
a) Our style guide generally forbids overloads. (Probably you should call it
"StartClientWithFD" or something like that.)
b) This doesn't make a lot of sense for non-POSIX, does it?

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:79: DCHECK(state() == STATE_INITIALIZED);
DCHECK_EQ

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:80: DCHECK(client_process_ !=
base::kNullProcessHandle);
DCHECK_NE

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:81: 
DCHECK(connected_) also?

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:100: // Has the bootstrap channel made?
"made" -> "been made"

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:108: DCHECK(state() == STATE_INITIALIZED);
DCHECK_EQ

(etc.)

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:117: SendClientPipeIfReady();
Is the order of OnClientLaunched and OnChannelConnected not fixed? (I'm confused
why we need the SendClientPipeIfReady in two places....)

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:37: explicit MojoBootstrap();
nit: no need for explicit

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:74: // MojoBootstrap for the server process. You
should create the instance
Does this (and MojoClientBootstrap) need to be in the header at all?

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:78: explicit MojoServerBootstrap();
no explicit

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:87: virtual bool OnMessageReceived(const Message&)
OVERRIDE;
nit: include the name of the argument ("message", I guess

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:93: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:99: explicit MojoClientBootstrap();
no explicit

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:105: virtual bool OnMessageReceived(const
Message&) OVERRIDE;
nit: include the name of the argument ("message", I guess

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:107: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap_delegate.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap_delegate.h:12: class MojoBootstrapDelegate {
Normally, I'm a fan of standalone delegate classes, but in this case is it
really worth having a separate header file (and separating it from
MojoBootstrap)?

(Unless there's a compelling reason, I'd just have this as an inner class of
MojoBootstrap.)

[Hajime Morrita, 2014-09-15 22:01:38]

Thanks for the review Trung! PTAL?

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2090:
channel_->OnClientLaunched(child_process_launcher_->GetHandle());
On 2014/09/15 20:19:43, viettrungluu wrote:
> It feels a bit suboptimal to require an extra explicit call....
> 
> I wonder since the listener already get a process ID back (in
> OnChannelConnected), we couldn't just use that (and OpenProcess) to get a
handle
> to the child there....

Yeah...
I tried that but it didn't work.
A handle returned from OpenProcess() is different from one that 
is returned from CreateProcess(), and DuplicateHandle() call with that 
handle declines the request saying "access denied" :-/

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:182: // Tell the server channel its client process handle.
On 2014/09/15 20:19:43, viettrungluu wrote:
> "the" -> "a"

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:183: // A Channel implementation needs this to establish the
connection
On 2014/09/15 20:19:43, viettrungluu wrote:
> "needs" -> "may need"

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:184: // to the client. This only has to be called on
server-side channel.
On 2014/09/15 20:19:43, viettrungluu wrote:
> Probably be a little more explicit: "This only ..." -> "This should only be
> called for a server channel."

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_channel_proxy.h#...
ipc/ipc_channel_proxy.h:123: // See Channel::OnClientLaunched for detail.
On 2014/09/15 20:19:43, viettrungluu wrote:
> "detail" -> "details"

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_test_base.h
File ipc/ipc_test_base.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/ipc_test_base.h#newc...
ipc/ipc_test_base.h:80: bool StartClient(int ipcfd);
On 2014/09/15 20:19:43, viettrungluu wrote:
> a) Our style guide generally forbids overloads. (Probably you should call it
> "StartClientWithFD" or something like that.)
> b) This doesn't make a lot of sense for non-POSIX, does it?
Right. Rearrange a bit to expose it to OS_POSIX platforms.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:80: DCHECK(client_process_ !=
base::kNullProcessHandle);
On 2014/09/15 20:19:43, viettrungluu wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:81: 
On 2014/09/15 20:19:43, viettrungluu wrote:
> DCHECK(connected_) also?

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:100: // Has the bootstrap channel made?
On 2014/09/15 20:19:43, viettrungluu wrote:
> "made" -> "been made"

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:108: DCHECK(state() == STATE_INITIALIZED);
On 2014/09/15 20:19:44, viettrungluu wrote:
> DCHECK_EQ
> 
> (etc.)

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:117: SendClientPipeIfReady();
On 2014/09/15 20:19:43, viettrungluu wrote:
> Is the order of OnClientLaunched and OnChannelConnected not fixed? (I'm
confused
> why we need the SendClientPipeIfReady in two places....)

Unfortunately not. RenderHost launches the child process in different thread
and the order of  OnClientLaunched and OnChannelConnected isn't fixed.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:37: explicit MojoBootstrap();
On 2014/09/15 20:19:44, viettrungluu wrote:
> nit: no need for explicit

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:74: // MojoBootstrap for the server process. You
should create the instance
On 2014/09/15 20:19:44, viettrungluu wrote:
> Does this (and MojoClientBootstrap) need to be in the header at all?
Good point. Moved to .cc file.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:78: explicit MojoServerBootstrap();
On 2014/09/15 20:19:44, viettrungluu wrote:
> no explicit

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:87: virtual bool OnMessageReceived(const Message&)
OVERRIDE;
On 2014/09/15 20:19:44, viettrungluu wrote:
> nit: include the name of the argument ("message", I guess

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:93: };
On 2014/09/15 20:19:44, viettrungluu wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:99: explicit MojoClientBootstrap();
On 2014/09/15 20:19:44, viettrungluu wrote:
> no explicit

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:105: virtual bool OnMessageReceived(const
Message&) OVERRIDE;
On 2014/09/15 20:19:44, viettrungluu wrote:
> nit: include the name of the argument ("message", I guess

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.h:107: };
On 2014/09/15 20:19:44, viettrungluu wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap_delegate.h (right):

https://codereview.chromium.org/553283002/diff/60001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap_delegate.h:12: class MojoBootstrapDelegate {
On 2014/09/15 20:19:44, viettrungluu wrote:
> Normally, I'm a fan of standalone delegate classes, but in this case is it
> really worth having a separate header file (and separating it from
> MojoBootstrap)?
> 
> (Unless there's a compelling reason, I'd just have this as an inner class of
> MojoBootstrap.)
Made it an inner class.
I hoped it help to hide some dependency from ipc_channel_mojo.h in
ipc_mojo_bootstrap.h but it didn't work well :-/

[viettrungluu, 2014-09-15 22:37:28]

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/553283002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2090:
channel_->OnClientLaunched(child_process_launcher_->GetHandle());
On 2014/09/15 22:01:36, morrita wrote:
> On 2014/09/15 20:19:43, viettrungluu wrote:
> > It feels a bit suboptimal to require an extra explicit call....
> > 
> > I wonder since the listener already get a process ID back (in
> > OnChannelConnected), we couldn't just use that (and OpenProcess) to get a
> handle
> > to the child there....
> 
> Yeah...
> I tried that but it didn't work.
> A handle returned from OpenProcess() is different from one that 
> is returned from CreateProcess(), and DuplicateHandle() call with that 
> handle declines the request saying "access denied" :-/
> 

I was afraid of something like that.

This design still seems a bit odd and fragile to me. (E.g., are there other
places where we launch processes, where we don't call OnClientLaunched()?
Obviously, this doesn't affect us now, but it could be a source of problems in
the future.)

Unfortunately, I don't really have any idea that's much better (short of much
bigger refactoring). Maybe darin or jam might have some good ideas?

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:105: return StartClientInternal(0);
I don't know if I'm a huge fan of this.

I'd like something like:

#if defined(OS_WIN)
// definition for StartClient()
#elif define(OS_POSIX)
// definition for StartClient, just calling StartClientWithFD
// definition for StartClientWithFD
#endif

The only question is how to reasonably share code between the Windows and POSIX
cases.

I suppose this is just test code, so I'm not going to get to hung up about it
though.

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:124: DCHECK(client_process_ == base::kNullProcessHandle);
DCHECK_EQ here too

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:141: channel_->OnClientLaunched(client_process_);
It seems dubious to call these even when client_process_ is the null process
handle, no?

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.cc:78: class HelloMessage {
I don't think you need (or use) this anywhere?

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:18: // using MojoBootstrap::Create()
nit: Needs a period at the end.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:41: // using MojoBootstrap::Create()
"

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:117: MojoServerBootstrap::MojoServerBootstrap()
Apparently, it's legal to define stuff that's declared in an anonymous namespace
without putting the definition (explicitly) in the anonymous namespace. (It
makes sense now that I think about it.)

But it's a bit weird, so you should probably put the definitions inside the
"namespace { ... }". (Maybe just move the MojoServerBootstrap methods up to
immediately follow the class declaration, and ditto for MojoClientBootstrap.)

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap_delegate.h (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap_delegate.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
I think you wanted to remove this file.... :)

[Hajime Morrita, 2014-09-15 23:51:33]

PTAL?

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:105: return StartClientInternal(0);
On 2014/09/15 22:37:27, viettrungluu wrote:
> I don't know if I'm a huge fan of this.
> 
> I'd like something like:
> 
> #if defined(OS_WIN)
> // definition for StartClient()
> #elif define(OS_POSIX)
> // definition for StartClient, just calling StartClientWithFD
> // definition for StartClientWithFD
> #endif
> 
> The only question is how to reasonably share code between the Windows and
POSIX
> cases.
> 
> I suppose this is just test code, so I'm not going to get to hung up about it
> though.
Tried this and now looks simpler. Thanks for the suggestion!

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:124: DCHECK(client_process_ == base::kNullProcessHandle);
On 2014/09/15 22:37:27, viettrungluu wrote:
> DCHECK_EQ here too

Done.

https://codereview.chromium.org/553283002/diff/80001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:141: channel_->OnClientLaunched(client_process_);
On 2014/09/15 22:37:27, viettrungluu wrote:
> It seems dubious to call these even when client_process_ is the null process
> handle, no?

True. Added DCHECK() as there are no cases where the handle should be null.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo.cc:78: class HelloMessage {
On 2014/09/15 22:37:27, viettrungluu wrote:
> I don't think you need (or use) this anywhere?

Right. Removing.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:18: // using MojoBootstrap::Create()
On 2014/09/15 22:37:28, viettrungluu wrote:
> nit: Needs a period at the end.

Done.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:41: // using MojoBootstrap::Create()
On 2014/09/15 22:37:27, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap.cc:117: MojoServerBootstrap::MojoServerBootstrap()
On 2014/09/15 22:37:28, viettrungluu wrote:
> Apparently, it's legal to define stuff that's declared in an anonymous
namespace
> without putting the definition (explicitly) in the anonymous namespace. (It
> makes sense now that I think about it.)
> 
> But it's a bit weird, so you should probably put the definitions inside the
> "namespace { ... }". (Maybe just move the MojoServerBootstrap methods up to
> immediately follow the class declaration, and ditto for MojoClientBootstrap.)

Ouch. This is indeed weird. Surprised that the compiler didn't complain :-(
Fixed.

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
File ipc/mojo/ipc_mojo_bootstrap_delegate.h (right):

https://codereview.chromium.org/553283002/diff/80001/ipc/mojo/ipc_mojo_bootst...
ipc/mojo/ipc_mojo_bootstrap_delegate.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/09/15 22:37:28, viettrungluu wrote:
> I think you wanted to remove this file.... :)

Done.

[Hajime Morrita, 2014-09-15 23:52:54]

On passing the process handle, we could probably have global ID/handle map
globally and look it. 
I'm not sure if it's good approach though.

[Hajime Morrita, 2014-09-17 01:15:58]

Trung: PTAL?

I gave up to touch ChannelProxy and Channel interface
and added new class (ChannelMojoHost) instead so that ChannelMojo-specific
complication doesn't pollute existing IPC classes. ChannelMojoHost is also meant
to
be a signal that ChannelMojo clients need some extra work.

[viettrungluu, 2014-09-17 17:33:15]

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:430: :
channel_host_(new IPC::ChannelMojoHost(
Do you want to create this even if ShouldUseMojoChannel() is false?

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:259:
scoped_ptr<IPC::ChannelMojoHost> channel_host_;
I wonder if you should call this channel_mojo_host_ instead. Hmm.

https://codereview.chromium.org/553283002/diff/120001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/ipc_test_base.cc#ne...
ipc/ipc_test_base.cc:102: namespace {}  // namespace
delete

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:91: // Create MojoBootstrap aftera all members are
set as it touches
aftera -> after

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:64: // Create ChannelMojo. A bootstrap channel is
created as well.
You should indicate when |host| must be non-null.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:137: ChannelMojoHost* host_;
ChannelMojoHost* const (so it's obvious it's never changed)?

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:22: // its client provides, including child
preocess' process handle. Every
"including the child process's process handle"

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:36: void OnChannelDeleted();
"destroyed" would be more idiomatic?

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:43: };
nits:
- should have a DISALLOW_COPY_AND_ASSIGN (and a base/macros.h include)
- blank line after the "};"
- the last "}" should have a "// namespace IPC" comment

[Hajime Morrita, 2014-09-17 19:33:48]

PTAL?

On base/ changes, I'm thinking about addressing it 
in a bit larger way as the feedback at 
https://codereview.chromium.org/555503007/ implies.

So once ipc/ get looking good, I'll try to clean there
and land this after that.

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:430: :
channel_host_(new IPC::ChannelMojoHost(
On 2014/09/17 17:33:15, viettrungluu wrote:
> Do you want to create this even if ShouldUseMojoChannel() is false?
Good point. make it lazy.

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/553283002/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:259:
scoped_ptr<IPC::ChannelMojoHost> channel_host_;
On 2014/09/17 17:33:15, viettrungluu wrote:
> I wonder if you should call this channel_mojo_host_ instead. Hmm.

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/ipc_test_base.cc#ne...
ipc/ipc_test_base.cc:102: namespace {}  // namespace
On 2014/09/17 17:33:15, viettrungluu wrote:
> delete
Oops. done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:91: // Create MojoBootstrap aftera all members are
set as it touches
On 2014/09/17 17:33:15, viettrungluu wrote:
> aftera -> after

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:64: // Create ChannelMojo. A bootstrap channel is
created as well.
On 2014/09/17 17:33:15, viettrungluu wrote:
> You should indicate when |host| must be non-null.

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:137: ChannelMojoHost* host_;
On 2014/09/17 17:33:15, viettrungluu wrote:
> ChannelMojoHost* const (so it's obvious it's never changed)?

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:22: // its client provides, including child
preocess' process handle. Every
On 2014/09/17 17:33:15, viettrungluu wrote:
> "including the child process's process handle"

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:36: void OnChannelDeleted();
On 2014/09/17 17:33:15, viettrungluu wrote:
> "destroyed" would be more idiomatic?

Done.

https://codereview.chromium.org/553283002/diff/120001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:43: };
On 2014/09/17 17:33:15, viettrungluu wrote:
> nits:
> - should have a DISALLOW_COPY_AND_ASSIGN (and a base/macros.h include)
> - blank line after the "};"
> - the last "}" should have a "// namespace IPC" comment

Done.

[Hajime Morrita, 2014-09-22 18:18:45]

Hi Trung, could you take another look at this?
I updated the CL to make the need of process handle passing clearer.
Also I removed non essential changes on base/ and mojo/ to keep this concise.

[viettrungluu, 2014-09-22 19:13:08]

LGTM w/nits.

https://codereview.chromium.org/553283002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/553283002/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:267: // A host object
ChannelMojo needs. The lifetime is bound to
nit: to *the* RenderProcessHostImpl, not *the* channel

(skip the *'s in the actual comment :) )

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:65: // Each client should always provide |host|. It
shouldn't be null.
nit: Probably you can skip the "Each client should always provide |host|."
sentence, and just write "|host| must not be null.".

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:73: // |host| shouldn't be null.
nit: shouldn't -> must not

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:22: // its client provides, including the child
preocess's process handle. Every
preocess -> process

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:23: // server process that uses ChannelMojo has
to have ChannelMojoHost
"has to have" -> "must have a"

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:59: CHECK(client_pipe !=
IPC::InvalidPlatformFileForTransit());
CHECK_NE

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:85: DCHECK(state() == STATE_INITIALIZED);
DCHECK_EQ

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:91: DCHECK(state() == STATE_WAITING_ACK);
"

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
File ipc/mojo/ipc_mojo_bootstrap_unittest.cc (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap_unittest.cc:67: // A long running process that
connects to us
nit: Needs a period at the end.

[Hajime Morrita, 2014-09-22 19:32:48]

Thanks!

https://codereview.chromium.org/553283002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/553283002/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:267: // A host object
ChannelMojo needs. The lifetime is bound to
On 2014/09/22 19:13:08, viettrungluu wrote:
> nit: to *the* RenderProcessHostImpl, not *the* channel
> 
> (skip the *'s in the actual comment :) )

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:65: // Each client should always provide |host|. It
shouldn't be null.
On 2014/09/22 19:13:08, viettrungluu wrote:
> nit: Probably you can skip the "Each client should always provide |host|."
> sentence, and just write "|host| must not be null.".

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.h:73: // |host| shouldn't be null.
On 2014/09/22 19:13:08, viettrungluu wrote:
> nit: shouldn't -> must not

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:22: // its client provides, including the child
preocess's process handle. Every
On 2014/09/22 19:13:08, viettrungluu wrote:
> preocess -> process
Ouch :-(

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo_host.h:23: // server process that uses ChannelMojo has
to have ChannelMojoHost
On 2014/09/22 19:13:08, viettrungluu wrote:
> "has to have" -> "must have a"

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
File ipc/mojo/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:59: CHECK(client_pipe !=
IPC::InvalidPlatformFileForTransit());
On 2014/09/22 19:13:08, viettrungluu wrote:
> CHECK_NE

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:85: DCHECK(state() == STATE_INITIALIZED);
On 2014/09/22 19:13:08, viettrungluu wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap.cc:91: DCHECK(state() == STATE_WAITING_ACK);
On 2014/09/22 19:13:08, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
File ipc/mojo/ipc_mojo_bootstrap_unittest.cc (right):

https://codereview.chromium.org/553283002/diff/180001/ipc/mojo/ipc_mojo_boots...
ipc/mojo/ipc_mojo_bootstrap_unittest.cc:67: // A long running process that
connects to us
On 2014/09/22 19:13:08, viettrungluu wrote:
> nit: Needs a period at the end.

Done.

[Hajime Morrita, 2014-09-22 19:35:53]

morrita@chromium.org changed reviewers:
+ jam@chromium.org

[Hajime Morrita, 2014-09-22 19:35:54]

Jam: Could you take a look at changes on these files?

content/browser/renderer_host/render_process_host_impl.cc
content/browser/renderer_host/render_process_host_impl.h
content/child/child_thread.cc

- Add ChannelMojoHost as as member

ipc/ipc_test_base.cc
ipc/ipc_test_base.h

- Add a hook after launching child process.

[jam, 2014-09-23 05:09:40]

lgtm

[Hajime Morrita, 2014-09-23 18:23:40]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-23 20:36:39]

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

[commit-bot: I haz the power, 2014-09-23 21:16:09]

Committed patchset #12 (id:220001) as d72e013407c787f0953a236a6d539b88fbda0f55

[commit-bot: I haz the power, 2014-09-23 21:16:58]

Patchset 12 (id:??) landed as
https://crrev.com/54f6f80c3623a6fb9d3049b6f5e0e23b1d76c34d
Cr-Commit-Position: refs/heads/master@{#296248}