[Chromoting] Request the daemon to open a terminal once a connection has been accepted.

[Chromoting] Request the daemon to open a terminal once a connection has been accepted.

Opening a terminal will invoke (once this functionality is in place) the desktop integration code. The latter will pass a client end of the IPC channel back to the desktop environment object.

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

[alexeypa (please no reviews), 2012-10-10 22:26:09]

PTAL or ping me if you are busy.

[alexeypa (please no reviews), 2012-10-12 14:54:22]

ping

[Wez, 2012-10-13 00:47:14]

http://codereview.chromium.org/11028128/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/11028128/diff/1/remoting/host/chromoting_host....
remoting/host/chromoting_host.h:151: // Disconnects a client with matching
|desktop_environment|
nit: "Disconnects the client that is using |desktop_environment|, if any."

http://codereview.chromium.org/11028128/diff/1/remoting/host/daemon_sender.h
File remoting/host/daemon_sender.h (right):

http://codereview.chromium.org/11028128/diff/1/remoting/host/daemon_sender.h#...
remoting/host/daemon_sender.h:14: class DaemonSender {
This class is called DaemonSender, but has two APIs that talk about "terminals",
both of which accept a DesktopEnvironment; the naming is really confusing,
especially without any comments to explain what DaemonSender is for, or what a
"terminal" is. ;)

Is the intent fundamentally that these calls just notify the daemon process of
DesktopEnvironment lifetimes, in which case more appropriate names might be
TerminalCreated and TerminalDestroyed (assuming that "terminal" refers to the
client"), or NotifyClientAttached/Detached, for example?

Or should these calls actually create the binding between the DesktopEnvironment
and the desktop session, so that you'd have e.g.
AttachTerminal(IpcDesktopEnvironment) and the IpcDesktopEnvironment would then
receive calls every time a new IPC channel to the desktop process is allocated?

http://codereview.chromium.org/11028128/diff/1/remoting/host/daemon_sender.h#...
remoting/host/daemon_sender.h:24: virtual void OpenTerminal(DesktopEnvironment*
desktop_environment) = 0;
nit: Put this above CloseTerminal, since it's the first of the two that will be
called.

http://codereview.chromium.org/11028128/diff/1/remoting/host/ipc_desktop_envi...
File remoting/host/ipc_desktop_environment.h (right):

http://codereview.chromium.org/11028128/diff/1/remoting/host/ipc_desktop_envi...
remoting/host/ipc_desktop_environment.h:16: class IpcDesktopEnvironment : public
DesktopEnvironment {
Add a description of what IpcDesktopEnvironment is, i.e. that it's the network
process' interface to capture & event processing components running in a
separate process.

http://codereview.chromium.org/11028128/diff/1/remoting/host/ipc_desktop_envi...
File remoting/host/ipc_desktop_environment_factory.h (right):

http://codereview.chromium.org/11028128/diff/1/remoting/host/ipc_desktop_envi...
remoting/host/ipc_desktop_environment_factory.h:14: class
IpcDesktopEnvironmentFactory : public DesktopEnvironmentFactory {
Add a comment to explain what this class actually does.

http://codereview.chromium.org/11028128/diff/1/remoting/host/ipc_desktop_envi...
remoting/host/ipc_desktop_environment_factory.h:16: // |daemon_sender| must
outlive |this|.
Clarify here what |daemon_sender| will be used for.

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
typo: |terminal_id|

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
typo: corresponding

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
nit: desktop environment -> DesktopEnvironment

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
nit: Reword "Closes any client connection that is using the specified
|terminal_id|".

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:722: std::map<int, DesktopEnvironment*>
registered_terminals_;
nit: registered -> active

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:726: static
base::StaticAtomicSequenceNumber terminal_id_;
Do we need StatucAtomicSequenceNumber; we're not running OpenTerminal() on
multiple threads...

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:731: #if defined(REMOTING_MULTI_PROCESS)
I really think we need to move the multi-process stuff out of here.

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:755: void
HostProcess::OpenTerminal(DesktopEnvironment* desktop_environment) {
OpenTerminal -> CreateTerminal / AttachTerminal

http://codereview.chromium.org/11028128/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:778: registered_terminals_.erase(i);
Why not let the client disconnection trigger CloseTerminal() is it would if the
client disconnected normally, so that the daemon code will always get a close
IPC for every terminal?

[alexeypa (please no reviews), 2012-10-16 19:32:25]

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/chromoti...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/chromoti...
remoting/host/chromoting_host.h:151: // Disconnects a client with matching
|desktop_environment|
On 2012/10/13 00:47:15, Wez wrote:
> nit: "Disconnects the client that is using |desktop_environment|, if any."

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/daemon_s...
File remoting/host/daemon_sender.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/daemon_s...
remoting/host/daemon_sender.h:14: class DaemonSender {
On 2012/10/13 00:47:15, Wez wrote:
> Or should these calls actually create the binding between the
DesktopEnvironment
> and the desktop session, so that you'd have e.g.
> AttachTerminal(IpcDesktopEnvironment) and the IpcDesktopEnvironment would then
> receive calls every time a new IPC channel to the desktop process is
allocated?

This is the case. The class has been renamed to DesktopSessionConnector.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/daemon_s...
remoting/host/daemon_sender.h:24: virtual void OpenTerminal(DesktopEnvironment*
desktop_environment) = 0;
On 2012/10/13 00:47:15, Wez wrote:
> nit: Put this above CloseTerminal, since it's the first of the two that will
be
> called.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment.h:16: class IpcDesktopEnvironment : public
DesktopEnvironment {
On 2012/10/13 00:47:15, Wez wrote:
> Add a description of what IpcDesktopEnvironment is, i.e. that it's the network
> process' interface to capture & event processing components running in a
> separate process.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/ipc_desk...
File remoting/host/ipc_desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_factory.h:14: class
IpcDesktopEnvironmentFactory : public DesktopEnvironmentFactory {
On 2012/10/13 00:47:15, Wez wrote:
> Add a comment to explain what this class actually does.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/ipc_desk...
remoting/host/ipc_desktop_environment_factory.h:16: // |daemon_sender| must
outlive |this|.
On 2012/10/13 00:47:15, Wez wrote:
> Clarify here what |daemon_sender| will be used for.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
On 2012/10/13 00:47:15, Wez wrote:
> typo: |terminal_id|

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
On 2012/10/13 00:47:15, Wez wrote:
> typo: corresponding

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
On 2012/10/13 00:47:15, Wez wrote:
> nit: desktop environment -> DesktopEnvironment

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:329: // Maps |termonal_id| to the
correspondinf desktop environment instance and
On 2012/10/13 00:47:15, Wez wrote:
> nit: Reword "Closes any client connection that is using the specified
> |terminal_id|".

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:722: std::map<int, DesktopEnvironment*>
registered_terminals_;
On 2012/10/13 00:47:15, Wez wrote:
> nit: registered -> active

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:726: static
base::StaticAtomicSequenceNumber terminal_id_;
On 2012/10/13 00:47:15, Wez wrote:
> Do we need StatucAtomicSequenceNumber; we're not running OpenTerminal() on
> multiple threads...

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:731: #if defined(REMOTING_MULTI_PROCESS)
On 2012/10/13 00:47:15, Wez wrote:
> I really think we need to move the multi-process stuff out of here.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:755: void
HostProcess::OpenTerminal(DesktopEnvironment* desktop_environment) {
On 2012/10/13 00:47:15, Wez wrote:
> OpenTerminal -> CreateTerminal / AttachTerminal

Done.

https://chromiumcodereview.appspot.com/11028128/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:778: registered_terminals_.erase(i);
On 2012/10/13 00:47:15, Wez wrote:
> Why not let the client disconnection trigger CloseTerminal() is it would if
the
> client disconnected normally, so that the daemon code will always get a close
> IPC for every terminal?

This gives one less message sent over IPC and makes it easier to reason about
the race between CloseTerminal and TerminalClosed.

[Wez, 2012-10-17 17:51:56]

Apologies for the delay; for the most part this layout looks great!

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
File remoting/host/desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_environment_factory.h:30: scoped_refptr<ChromotingHost>
host);
Since DesktopEnvironments are created per-client, why not pass the client here?
:)

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_environment_factory.h:30: scoped_refptr<ChromotingHost>
host);
If you really need to pass the |host| here then pass it to the factory-specific
ctor, rather than to the Create() method.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
File remoting/host/desktop_session_connector.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_session_connector.h:9: #include
"base/memory/ref_counted.h"
No need for this include

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_session_connector.h:32: // session from the accsociated
descktop environment.
typo: associated

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:22: // Passes relevant task runners,
|desktop_session_connector| and |host|.
nit: Lose this line.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:23: // |desktop_session_connector| is
used to bind the IpcDesktopEnvironment and
nit: and the -> to a

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:25: // a new IPC channel to the desktop
process is allocated.
nit: "... so that the former... " -> "... to be notified with a new IPC channel
every time the desktop process is changed."

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // |host| specifis the host instance
that indirectly owns |this|.
typo: specifies

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // |host| specifis the host instance
that indirectly owns |this|.
nit: Explain what |host| is used for and restrictions, e.g. |host| must refer to
the ChromotingHost for which this instance was created. It is used to disconnect
the client, e.g. if the desktop session is logged-out.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:30: scoped_ptr<VideoFrameCapturer>
video_capturer,
nit: Does the factory really need to pass these, or could it pass the task
runners and let the desktop environment itself create them?

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:39: void DisconnectClient();
Won't this be triggered by an IPC from the daemon, in which case should it be a
private API within IpcDesktopEnvironment?

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:43: bool connected_;
nit: Put this after the two fields that come from the ctor

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:32: IPC::ChannelProxy*
daemon_channel,
nit: Is there a way to get a ref-counted or owned-and-multiplexed handle to an
IPC channel?

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:49: // A list of
DesktopEnvironment instances known to the daemon process.
nit: "List of instances we've told the daemon process about."

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:51: ActiveConnectionsList
active_connections_;
nit: Move this down below the members that come from ctor parameters.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:56: // Task runner the bulk of
DesktopSessionConnector implementation runs on.
nit: Clarify this; e.g. "Used to service calls to the DesktopSessionConnector
APIs".

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:62: static int next_id_;
nit: This doesn't need to be static, I don't think, since there's only one
factory per-process?

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:278:
IPC_MESSAGE_HANDLER(ChromotingDaemonNetworkMsg_Configuration,
nit: Two spaces, plz.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:299: // Create the proper kind of the
desktop environment factory.
nit: Create a desktop environment factory appropriate to the build type &
platform.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:302: #if defined(REMOTING_MULTI_PROCESS)
nit: Unless we define REMOTING_MULTI_PROCESS on other platforms, you could turn
these ifdefs into:

if defined(multi-process)
elif defined(os_win)
else

[alexeypa (please no reviews), 2012-10-17 21:35:53]

PTAL

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
File remoting/host/desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_environment_factory.h:30: scoped_refptr<ChromotingHost>
host);
On 2012/10/17 17:51:57, Wez wrote:
> Since DesktopEnvironments are created per-client, why not pass the client
here?
> :)

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_environment_factory.h:30: scoped_refptr<ChromotingHost>
host);
On 2012/10/17 17:51:57, Wez wrote:
> If you really need to pass the |host| here then pass it to the
factory-specific
> ctor, rather than to the Create() method.

Ack.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
File remoting/host/desktop_session_connector.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_session_connector.h:9: #include
"base/memory/ref_counted.h"
On 2012/10/17 17:51:57, Wez wrote:
> No need for this include

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_session_connector.h:32: // session from the accsociated
descktop environment.
On 2012/10/17 17:51:57, Wez wrote:
> typo: associated

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:22: // Passes relevant task runners,
|desktop_session_connector| and |host|.
On 2012/10/17 17:51:57, Wez wrote:
> nit: Lose this line.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:23: // |desktop_session_connector| is
used to bind the IpcDesktopEnvironment and
On 2012/10/17 17:51:57, Wez wrote:
> nit: and the -> to a

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:25: // a new IPC channel to the desktop
process is allocated.
On 2012/10/17 17:51:57, Wez wrote:
> nit: "... so that the former... " -> "... to be notified with a new IPC
channel
> every time the desktop process is changed."

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // |host| specifis the host instance
that indirectly owns |this|.
On 2012/10/17 17:51:57, Wez wrote:
> typo: specifies

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // |host| specifis the host instance
that indirectly owns |this|.
On 2012/10/17 17:51:57, Wez wrote:
> nit: Explain what |host| is used for and restrictions, e.g. |host| must refer
to
> the ChromotingHost for which this instance was created. It is used to
disconnect
> the client, e.g. if the desktop session is logged-out.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:30: scoped_ptr<VideoFrameCapturer>
video_capturer,
On 2012/10/17 17:51:57, Wez wrote:
> nit: Does the factory really need to pass these, or could it pass the task
> runners and let the desktop environment itself create them?

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:39: void DisconnectClient();
On 2012/10/17 17:51:57, Wez wrote:
> Won't this be triggered by an IPC from the daemon, in which case should it be
a
> private API within IpcDesktopEnvironment?

IpcDesktopEnvironmentFactory calls it, so it needs to be public.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:43: bool connected_;
On 2012/10/17 17:51:57, Wez wrote:
> nit: Put this after the two fields that come from the ctor

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:32: IPC::ChannelProxy*
daemon_channel,
On 2012/10/17 17:51:57, Wez wrote:
> nit: Is there a way to get a ref-counted or owned-and-multiplexed handle to an
> IPC channel?

I don't think it is possible without a custom ChannelProxy.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:49: // A list of
DesktopEnvironment instances known to the daemon process.
On 2012/10/17 17:51:57, Wez wrote:
> nit: "List of instances we've told the daemon process about."

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:51: ActiveConnectionsList
active_connections_;
On 2012/10/17 17:51:57, Wez wrote:
> nit: Move this down below the members that come from ctor parameters.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:56: // Task runner the bulk of
DesktopSessionConnector implementation runs on.
On 2012/10/17 17:51:57, Wez wrote:
> nit: Clarify this; e.g. "Used to service calls to the DesktopSessionConnector
> APIs".

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/ipc_...
remoting/host/ipc_desktop_environment_factory.h:62: static int next_id_;
On 2012/10/17 17:51:57, Wez wrote:
> nit: This doesn't need to be static, I don't think, since there's only one
> factory per-process?

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:278:
IPC_MESSAGE_HANDLER(ChromotingDaemonNetworkMsg_Configuration,
On 2012/10/17 17:51:57, Wez wrote:
> nit: Two spaces, plz.

I have two on my machine. It is code review bugs, it seems.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:299: // Create the proper kind of the
desktop environment factory.
On 2012/10/17 17:51:57, Wez wrote:
> nit: Create a desktop environment factory appropriate to the build type &
> platform.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/remo...
remoting/host/remoting_me2me_host.cc:302: #if defined(REMOTING_MULTI_PROCESS)
On 2012/10/17 17:51:57, Wez wrote:
> nit: Unless we define REMOTING_MULTI_PROCESS on other platforms,

Currently we don't but I assume we might one day.

[Wez, 2012-10-17 22:04:24]

LGTM w/ a few items we should follow-up on separately.

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
File remoting/host/desktop_environment_factory.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/10001/remoting/host/desk...
remoting/host/desktop_environment_factory.h:30: scoped_refptr<ChromotingHost>
host);
On 2012/10/17 21:35:53, alexeypa wrote:
> On 2012/10/17 17:51:57, Wez wrote:
> > If you really need to pass the |host| here then pass it to the
> factory-specific
> > ctor, rather than to the Create() method.
> 
> Ack.

The more I think about it, the more I think I'd prefer that you pass the
ChromotingHost to the factory-specific ctor, but arguably this approach is more
future-proof.

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
remoting/host/client_session.h:94: DesktopEnvironmentFactory*
desktop_environment_factory,
nit: Add a comment to clarify that |desktop_environment_factory_| is used only
in this ctor; otherwise it's not clear that passing an un-owned object to a
ref-counted one is safe!

On a related note, we should make sure our DesktopEnvironment[Factory]
implementations cope properly with the instances outliving the factory.

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
remoting/host/client_session_unittest.cc:58:
desktop_environment_factory_.reset(new MockDesktopEnvironmentFactory());
nit: Strictly we should use Fakes here, since we're not actually mocking
anything, we just want to fake DE creation for ClientSession.  With them Mocks,
we should keep the |capturer_| member, so that it's possible for a future test
to mock it if we wanted.

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // a desktop session to be notified
with a new IPC channel every time
nit: ... session to ... -> ... session, to ...

[alexeypa (please no reviews), 2012-10-17 22:46:56]

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
remoting/host/client_session.h:94: DesktopEnvironmentFactory*
desktop_environment_factory,
On 2012/10/17 22:04:24, Wez wrote:
> nit: Add a comment to clarify that |desktop_environment_factory_| is used only
> in this ctor; otherwise it's not clear that passing an un-owned object to a
> ref-counted one is safe!
> 
> On a related note, we should make sure our DesktopEnvironment[Factory]
> implementations cope properly with the instances outliving the factory.

Done.

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/clie...
remoting/host/client_session_unittest.cc:58:
desktop_environment_factory_.reset(new MockDesktopEnvironmentFactory());
On 2012/10/17 22:04:24, Wez wrote:
> nit: Strictly we should use Fakes here, since we're not actually mocking
> anything, we just want to fake DE creation for ClientSession.  With them
Mocks,
> we should keep the |capturer_| member, so that it's possible for a future test
> to mock it if we wanted.

Ack.

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/ipc_...
File remoting/host/ipc_desktop_environment.h (right):

https://chromiumcodereview.appspot.com/11028128/diff/19002/remoting/host/ipc_...
remoting/host/ipc_desktop_environment.h:27: // a desktop session to be notified
with a new IPC channel every time
On 2012/10/17 22:04:24, Wez wrote:
> nit: ... session to ... -> ... session, to ...

Done.

[commit-bot: I haz the power, 2012-10-18 02:17:19]

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

[commit-bot: I haz the power, 2012-10-18 04:16:29]

Change committed as 162644