Move signaling connection creation out of ChromotingHost.

Move signaling connection creation out of ChromotingHost.

Also removed signaling-related events from ChromotingHostObserver interface 
and changes host observers to listen for notifications from SignalStrategy 
directly.

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

[Wez, 2012-01-03 16:25:04]

Basically LGTM, but see comments below!

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:49: DCHECK(desktop_environment_);
nit: Do we touch any of the other objects we are passed, in which case we should
DCHECK() on them, too?  Or do we only pass them on to other components?

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

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:69: // is destroyed.
nit: Reword, e.g. "Creates a ChromotingHost instance, which takes ownership of
|config|.  The caller must ensure that |context|, |signal_strategy| and
|environment| out-live the ChromotingHost."

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:44: DCHECK(state_ == CREATED || state_ ==
INITIALIZED || state_ == STOPPED);
nit: |state_| != STARTED is really what you mean here?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:64: signal_strategy_->AddListener(this);
What happens if the SignalStrategy is already connected by the time we reach
here?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:82: state_ = STOPPED;
Surely this should be INITIALIZED?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:66: // TODO(sergeyu): Is it enough to sign JID
and nothing else?
nit: Consider creating a bug for this, or removing it if the issue is resolved.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:72: // Initializes heartbeating. Returns false
if the |config| is
nit: Initializes the HeartbeatSender.

Heartbeats will start when the supplied SignalStrategy enters the CONNECTED
state?

What is the ownership of |config|?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:74: // outlive this object.
HeartbeatSender must now be initialized and deleted on the SignallingStrategy
thread?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server....
remoting/host/log_to_server.h:32: * The class is not thread-safe.
nit: C++ify this comment, and clarify that "not thread-safe" means it must only
be used on the same thread as the signalling strategy?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server_...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server_...
remoting/host/log_to_server_unittest.cc:79:
log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
nit: This test should really verify that the stanza is not sent until CONNECTED
is notified.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:504: // Beyond this point nothing can
fail,xs so save the config and request.
typo: xs

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.cc:50: if (!key_pair_.Load(config))
{
nit: Consider DCHECK()ing |signal_strategy|, since it is dereferenced below.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.cc:75: // We will reach here if
signaling fails to connect.
Or if signalling disconnects before we've received the response?

It feels like we should clean up |request_| and remove our signalling listener
when we receive the response, so that this DISCONNECTED handler is purely for
the failure case.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:34: // sends the request when
signalling channel is connected. When a
nit: ... sends a request to register the host for a Support-Id, as soon as the
associated SignalStrategy becomes connected.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:41: // amount of time until that
id expires.
nit: Could use a zero time-to-expiry to indicate failure.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:48: // Provide the configuration
to use to register the host, and a
nit: Initializes the registration to use a supplied SignalStrategy and
HostConfig, and to notify |callback| upon completion or failure.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:52: // invalid). Callback is never
called if the bot malfunctions and
nit: Would it simplify the interface to notify the callback on failure?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:53: // doesn't respond to the
request.
If the caller is responsible for implementing a sensible timeout then that
should be mentioned in the description of the class?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request_unittest.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request_unittest.cc:154:
.Times(listeners.size());
Ick.  Confusing; at what point is |listeners.size()| actually bound for this
expectation?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:95: 
nit: Lose extra newline?

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:111: // the account.
Sorry, I'm not sure whether this comment belongs in the if(), rather than
preceding it?

[Sergey Ulanov, 2012-01-03 21:51:01]

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:49: DCHECK(desktop_environment_);
On 2012/01/03 16:25:04, Wez wrote:
> nit: Do we touch any of the other objects we are passed, in which case we
should
> DCHECK() on them, too?  Or do we only pass them on to other components?

Done.

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

http://codereview.chromium.org/9004050/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:69: // is destroyed.
On 2012/01/03 16:25:04, Wez wrote:
> nit: Reword, e.g. "Creates a ChromotingHost instance, which takes ownership of
> |config|.  The caller must ensure that |context|, |signal_strategy| and
> |environment| out-live the ChromotingHost."

Done. This is a constructor, so I don't think that the comment needs to state
that it creates ChrtomotingHostInstance. Also, |config| is ref-counted, and I
removed this parameter in one of the next CLs.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:44: DCHECK(state_ == CREATED || state_ ==
INITIALIZED || state_ == STOPPED);
On 2012/01/03 16:25:04, Wez wrote:
> nit: |state_| != STARTED is really what you mean here?

Heh, I originally wrote it as |state_| != STARTED, but Albert objected:
http://codereview.chromium.org/3087003/diff/9001/remoting/host/heartbeat_send...
:)

In any case, I don't think we don't need this DCHECK anymore. Removed it.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:64: signal_strategy_->AddListener(this);
On 2012/01/03 16:25:04, Wez wrote:
> What happens if the SignalStrategy is already connected by the time we reach
> here?
Added OnSignalStrategyStateChange() call below to handle this case.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.cc:82: state_ = STOPPED;
On 2012/01/03 16:25:04, Wez wrote:
> Surely this should be INITIALIZED?
STOPPED is correct here. Anyway, in my next CL I removed INITIALIZED and CREATED
states - they are useless now.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:66: // TODO(sergeyu): Is it enough to sign JID
and nothing else?
On 2012/01/03 16:25:04, Wez wrote:
> nit: Consider creating a bug for this, or removing it if the issue is
resolved.

Just removed it.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:72: // Initializes heartbeating. Returns false
if the |config| is
On 2012/01/03 16:25:04, Wez wrote:
> nit: Initializes the HeartbeatSender.
> 
> Heartbeats will start when the supplied SignalStrategy enters the CONNECTED
> state?

Done.

> 
> What is the ownership of |config|?

|config| is ref-counted. I removed it in on the next CLs.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:74: // outlive this object.
On 2012/01/03 16:25:04, Wez wrote:
> HeartbeatSender must now be initialized and deleted on the SignallingStrategy
> thread?

Not yet in this CL - they are still initialized on plugin/main thread, but in
the next CL I made XmppSignalStrategy NonThreadSafe.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server.h
File remoting/host/log_to_server.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server....
remoting/host/log_to_server.h:32: * The class is not thread-safe.
On 2012/01/03 16:25:04, Wez wrote:
> nit: C++ify this comment, and clarify that "not thread-safe" means it must
only
> be used on the same thread as the signalling strategy?
Done. Removed the comment about non thread-safe. Will mark it as NonThreadSafe
in another CL.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server_...
File remoting/host/log_to_server_unittest.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/log_to_server_...
remoting/host/log_to_server_unittest.cc:79:
log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
On 2012/01/03 16:25:04, Wez wrote:
> nit: This test should really verify that the stanza is not sent until
CONNECTED
> is notified.

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:504: // Beyond this point nothing can
fail,xs so save the config and request.
On 2012/01/03 16:25:04, Wez wrote:
> typo: xs

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.cc:50: if (!key_pair_.Load(config))
{
On 2012/01/03 16:25:04, Wez wrote:
> nit: Consider DCHECK()ing |signal_strategy|, since it is dereferenced below.
Fixed in a different CL.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.cc:75: // We will reach here if
signaling fails to connect.
On 2012/01/03 16:25:04, Wez wrote:
> Or if signalling disconnects before we've received the response?
> 
> It feels like we should clean up |request_| and remove our signalling listener
> when we receive the response, so that this DISCONNECTED handler is purely for
> the failure case.

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request.h (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:34: // sends the request when
signalling channel is connected. When a
On 2012/01/03 16:25:04, Wez wrote:
> nit: ... sends a request to register the host for a Support-Id, as soon as the
> associated SignalStrategy becomes connected.

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:41: // amount of time until that
id expires.
On 2012/01/03 16:25:04, Wez wrote:
> nit: Could use a zero time-to-expiry to indicate failure.

I think that using a separate parameter is cleaner. In either case it's not
related to this CL.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:48: // Provide the configuration
to use to register the host, and a
On 2012/01/03 16:25:04, Wez wrote:
> nit: Initializes the registration to use a supplied SignalStrategy and
> HostConfig, and to notify |callback| upon completion or failure.

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:52: // invalid). Callback is never
called if the bot malfunctions and
On 2012/01/03 16:25:04, Wez wrote:
> nit: Would it simplify the interface to notify the callback on failure?

Having result here is useful to detect config errors before starting the host.
But I refactored this code in the next CL and removed this method.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:53: // doesn't respond to the
request.
On 2012/01/03 16:25:04, Wez wrote:
> If the caller is responsible for implementing a sensible timeout then that
> should be mentioned in the description of the class?

Normally GTalk would reply with an error when the bot is down, so it is very
unlikely that we don't get any response. Ideally this class should have timeout
and call the callback with error when timeout expires. I've added TODO to fix
it.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
File remoting/host/register_support_host_request_unittest.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/register_suppo...
remoting/host/register_support_host_request_unittest.cc:154:
.Times(listeners.size());
On 2012/01/03 16:25:04, Wez wrote:
> Ick.  Confusing; at what point is |listeners.size()| actually bound for this
> expectation?

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:95: 
On 2012/01/03 16:25:04, Wez wrote:
> nit: Lose extra newline?

Done.

http://codereview.chromium.org/9004050/diff/5001/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:111: // the account.
On 2012/01/03 16:25:04, Wez wrote:
> Sorry, I'm not sure whether this comment belongs in the if(), rather than
> preceding it?
It belongs inside because the if just tries to read this option from the config
and the next line applies only if the value is not present in the config.

[commit-bot: I haz the power, 2012-01-03 21:52:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9004050/8004

[commit-bot: I haz the power, 2012-01-03 23:10:18]

Try job failure for 9004050-8004 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "update" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-03 23:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9004050/8004

[commit-bot: I haz the power, 2012-01-04 01:10:13]

Change committed as 116256