[Chromoting] Moving the daemon IPC channel to ChromotingHostContext.

[Chromoting] Moving the network-to-daemon IPC channel to HostProcess. 

BUG=134694
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152264

[alexeypa (please no reviews), 2012-08-16 20:25:41]

PTAL.

[simonmorris, 2012-08-16 20:43:02]

Does the daemon channel belong in ChromotingHostContext?

I think of ChromotingHostContext as a set of resources that must be provided to
a ChromotingHost by its owner. The daemon channel doesn't seem to be given to
the ChromotingHost in the same way.

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:74: // the daemon process. NULL can be
returned if no channel exists.
"can be" -> "is".

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:77: // Connects to the endpoint provided
by the daemon procesess.
"process", or "processes"?

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:81: // Disconnects from the daemon
procesess.
same

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:105: // The IPC channel connection us to
the daemon process.
"connection" -> "connecting"

[alexeypa (please no reviews), 2012-08-16 20:53:51]

> I think of ChromotingHostContext as a set of resources that must be provided
to
> a ChromotingHost by its owner. The daemon channel doesn't seem to be given to
> the ChromotingHost in the same way.

|ChromotingHostContext.url_request_context_getter_| is implemented the same way.
And actually all thread with the exception of |ui_task_runner_| are created by
|ChromotingHostContext| itself.

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:74: // the daemon process. NULL can be
returned if no channel exists.
On 2012/08/16 20:43:02, simonmorris wrote:
> "can be" -> "is".

Done.

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:77: // Connects to the endpoint provided
by the daemon procesess.
On 2012/08/16 20:43:02, simonmorris wrote:
> "process", or "processes"?

Done.

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:81: // Disconnects from the daemon
procesess.
On 2012/08/16 20:43:02, simonmorris wrote:
> same

Done.

http://codereview.chromium.org/10837291/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:105: // The IPC channel connection us to
the daemon process.
On 2012/08/16 20:43:02, simonmorris wrote:
> "connection" -> "connecting"

Done.

[simonmorris, 2012-08-16 21:06:17]

On 2012/08/16 20:53:51, alexeypa wrote:
> > I think of ChromotingHostContext as a set of resources that must be provided
> to
> > a ChromotingHost by its owner. The daemon channel doesn't seem to be given
to
> > the ChromotingHost in the same way.
> 
> |ChromotingHostContext.url_request_context_getter_| is implemented the same
way.
> And actually all thread with the exception of |ui_task_runner_| are created by
> |ChromotingHostContext| itself.
> 

OK. But I don't see why moving the daemon channel out of SessionEventExecutorWin
is an improvement. Will objects other then SessionEventExecutorWin use it?

[alexeypa (please no reviews), 2012-08-16 21:14:25]

> OK. But I don't see why moving the daemon channel out of
SessionEventExecutorWin
> is an improvement. Will objects other then SessionEventExecutorWin use it?

Yes. This CL is apart of multi-process host refactoring. The network-to-daemon
IPC channel will be used to manage virtual terminals and receive configuration
updates. The 1st configuration update will be delivered even before
|ChromotingHost| is created leaving basically two alternatives where the channel
can be hosted: |HostProcess| or |ChromotingHostContext|. Given that the
reference to the channel will be passed around the latter seems a better choice.

[alexeypa (please no reviews), 2012-08-16 21:16:47]

On 2012/08/16 21:14:25, alexeypa wrote:
> > OK. But I don't see why moving the daemon channel out of
> SessionEventExecutorWin
> > is an improvement. Will objects other then SessionEventExecutorWin use it?
> 
> Yes. This CL is apart of multi-process host refactoring. The network-to-daemon
> IPC channel will be used to manage virtual terminals and receive configuration
> updates. The 1st configuration update will be delivered even before
> |ChromotingHost| is created leaving basically two alternatives where the
channel
> can be hosted: |HostProcess| or |ChromotingHostContext|. Given that the
> reference to the channel will be passed around the latter seems a better
choice.

"This CL is apart..." -> "This CL is a part..."

[simonmorris, 2012-08-16 21:20:46]

lgtm

[commit-bot: I haz the power, 2012-08-16 23:07:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10837291/8001

[commit-bot: I haz the power, 2012-08-16 23:33:03]

Try job failure for 10837291-8001 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-18 15:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10837291/15002

[commit-bot: I haz the power, 2012-08-18 23:25:33]

Change committed as 152264