Access ChromotingHost::clients_ only on network thread.

Access ChromotingHost::clients_ only on network thread.

Previously ChromotingHost was doing some work on the main thread and 
some on the network thread. |clients_| and some other members were 
accessed without lock on both of these threads. Moved most of the 
ChromotingHost activity to the network thread to avoid possible 
race conditions.

BUG=96325
TEST=Chromoting works

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

[Wez, 2011-11-09 02:32:22]

LGTM, but see comments.

http://codereview.chromium.org/8495024/diff/1/remoting/client/chromoting_clie...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/client/chromoting_clie...
remoting/client/chromoting_client.cc:70: received_packets_.pop_front();
This looks leaked from another CL?

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:137: // Stop chromotocol session manager.
nit: chromotocol -> Chromotocol.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:137: // Stop chromotocol session manager.
Clarify that we're guaranteed no more callbacks after Close()?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:141: &DeletePointer<protocol::SessionManager>,
session_manager_.release()));
This looks like a DeleteSoon()?  Clarify in the comment above why we need to
defer deletion until the next time we hit the main loop?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:144: // Stop XMPP connection.
Clarify that this is synchronous?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:180: OnSessionClosed(*other_client);
If Disconnect() is made to trigger OnSessionClosed(), as previously suggested,
then this can go away.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:185: CHECK(recorder_.get() == NULL);
We're in OnSessionAuthenticated(), so surely this client is authenticated, in
which case the recorder will have been kept around?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:204: const std::string& jid =
client->connection()->session()->jid();
nit: Move this after the Notify observers comment?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:215: std::string username = jid;
Why not just take a copy of the JID in the first place, so you don't need to
take a copy into a separate variable down here?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:382: protocol_config_.reset(config);
Not part of this CL, but this is client-specific, so it belongs on the
ClientSession, not here.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:412: desktop_environment_->OnPause(pause);
Is this callable on the network thread?

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:471: recorder_ = NULL;
Does ScreenRecorder::Stop() guarantee to NOT invoke the callback before it
returns?  If not then |this| may not exist by the time Stop() returns in that
situation.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:509: (*it)->OnShutdown();
We're notifying observers on a different thread to previously; are all the
observers designed to cope with that?

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:200: // network thread.
nit: Consider bundling all the members which are network-thread only together,
and similarly for the other thread-specific members.

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
File remoting/host/host_status_observer.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:24: // Called when status of the XMPP
changes.
nit: status of the XMPP signalling layer?

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:30: // to connect to the host.
nit: line-wrap.

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:35: // callback; it is purely
informational.
nit: Incorrect plural/singular.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.cc:64: &DeletePointer<protocol::Session>,
session_.release()));
Like DeleteSoon()?

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/jingle_sessio...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.cc:236: // Reset the callback so that we don't
call it anymore.
nit: Could reword: Ensure that we won't notify the callback again.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/pepper_sessio...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/pepper_sessio...
remoting/protocol/pepper_session.cc:194: state_change_callback_.Reset();
nit: Clarify that we do this to meet the no-more-callbacks requirement?

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:31: // with clean stack, i.e. not from event
handlers, when sigslot may be
nit: with _a_ clean stack.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:128: // after this method is called.
nit: after this method returns?

[Sergey Ulanov, 2011-11-09 21:24:00]

http://codereview.chromium.org/8495024/diff/1/remoting/client/chromoting_clie...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/client/chromoting_clie...
remoting/client/chromoting_client.cc:70: received_packets_.pop_front();
On 2011/11/09 02:32:22, Wez wrote:
> This looks leaked from another CL?
Yeah, it should be in a separate CL. I thought my change triggered DCHECK due to
this issue, but it is actually not related to my change.

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:137: // Stop chromotocol session manager.
On 2011/11/09 02:32:22, Wez wrote:
> nit: chromotocol -> Chromotocol.
Changed to Stop session manager.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:137: // Stop chromotocol session manager.
On 2011/11/09 02:32:22, Wez wrote:
> Clarify that we're guaranteed no more callbacks after Close()?
This is specified in SessionManager header. Don't think it is necessary to
repeat it here.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:141: &DeletePointer<protocol::SessionManager>,
session_manager_.release()));
On 2011/11/09 02:32:22, Wez wrote:
> This looks like a DeleteSoon()?  Clarify in the comment above why we need to
> defer deletion until the next time we hit the main loop?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:144: // Stop XMPP connection.
On 2011/11/09 02:32:22, Wez wrote:
> Clarify that this is synchronous?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:180: OnSessionClosed(*other_client);
On 2011/11/09 02:32:22, Wez wrote:
> If Disconnect() is made to trigger OnSessionClosed(), as previously suggested,
> then this can go away.
I don't think that Disconnect() should trigger OnSessionClosed()

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:185: CHECK(recorder_.get() == NULL);
On 2011/11/09 02:32:22, Wez wrote:
> We're in OnSessionAuthenticated(), so surely this client is authenticated, in
> which case the recorder will have been kept around?
Recorder is created after client is authenticated, i.e. in this method. Because
we allow only one client at a time recorder_ should not exist when this method
is called.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:204: const std::string& jid =
client->connection()->session()->jid();
On 2011/11/09 02:32:22, Wez wrote:
> nit: Move this after the Notify observers comment?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:215: std::string username = jid;
On 2011/11/09 02:32:22, Wez wrote:
> Why not just take a copy of the JID in the first place, so you don't need to
> take a copy into a separate variable down here?
Simplified this code to use substr().

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:382: protocol_config_.reset(config);
On 2011/11/09 02:32:22, Wez wrote:
> Not part of this CL, but this is client-specific, so it belongs on the
> ClientSession, not here.
Actually it is not client-specific - this sets CandidateSessionConfig which
contains list of supported protocol configurations. It is used by simple_host to
force specific configuration.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:412: desktop_environment_->OnPause(pause);
On 2011/11/09 02:32:22, Wez wrote:
> Is this callable on the network thread?
Yes. All DesktopEnvironment methods can be called on any thread - it needs to
post to the ui thread anyway.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:471: recorder_ = NULL;
On 2011/11/09 02:32:22, Wez wrote:
> Does ScreenRecorder::Stop() guarantee to NOT invoke the callback before it
> returns?  If not then |this| may not exist by the time Stop() returns in that
> situation.
Good catch. Fixed, though it should not be a problem with the current
ScreenRecorder.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:509: (*it)->OnShutdown();
On 2011/11/09 02:32:22, Wez wrote:
> We're notifying observers on a different thread to previously; are all the
> observers designed to cope with that?
There is only one observer that cares about this event (see HostNPScriptObject)
and I've updated it.

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:200: // network thread.
On 2011/11/09 02:32:22, Wez wrote:
> nit: Consider bundling all the members which are network-thread only together,
> and similarly for the other thread-specific members.
Done. Practically all members of this class are used on the network thread only.
Updated comments to reflect it.
Also removed |lock_| as it is not needed anymore.

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
File remoting/host/host_status_observer.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:24: // Called when status of the XMPP
changes.
On 2011/11/09 02:32:22, Wez wrote:
> nit: status of the XMPP signalling layer?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:30: // to connect to the host.
On 2011/11/09 02:32:22, Wez wrote:
> nit: line-wrap.

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:35: // callback; it is purely
informational.
On 2011/11/09 02:32:22, Wez wrote:
> nit: Incorrect plural/singular.

Changed s/the observer/observers/, but not sure if that's the problem.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.cc:64: &DeletePointer<protocol::Session>,
session_.release()));
On 2011/11/09 02:32:22, Wez wrote:
> Like DeleteSoon()?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/jingle_sessio...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.cc:236: // Reset the callback so that we don't
call it anymore.
On 2011/11/09 02:32:22, Wez wrote:
> nit: Could reword: Ensure that we won't notify the callback again.

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/pepper_sessio...
File remoting/protocol/pepper_session.cc (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/pepper_sessio...
remoting/protocol/pepper_session.cc:194: state_change_callback_.Reset();
On 2011/11/09 02:32:22, Wez wrote:
> nit: Clarify that we do this to meet the no-more-callbacks requirement?

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:31: // with clean stack, i.e. not from event
handlers, when sigslot may be
On 2011/11/09 02:32:22, Wez wrote:
> nit: with _a_ clean stack.

Done.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:128: // after this method is called.
On 2011/11/09 02:32:22, Wez wrote:
> nit: after this method returns?
No - callback is not expected during this call. Previously it would call
StateChangeCallback(CLOSED) in response to this call. Not anymore.

[Wez, 2011-11-09 23:18:45]

LGTM w/ a few remaining comments.  The main one is around the Disconnect()
semantics; I'm not convinced it's wrong for Disconnect() to trigger
OnSessionClosed().

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:180: OnSessionClosed(*other_client);
On 2011/11/09 21:24:00, sergeyu wrote:
> On 2011/11/09 02:32:22, Wez wrote:
> > If Disconnect() is made to trigger OnSessionClosed(), as previously
suggested,
> > then this can go away.
> I don't think that Disconnect() should trigger OnSessionClosed()

Why not?  The EventHandler really only cares that the session has closed -
whether that's because the peer closed it, or we did, doesn't seem relevant to
the EventHandler.

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:185: CHECK(recorder_.get() == NULL);
On 2011/11/09 21:24:00, sergeyu wrote:
> On 2011/11/09 02:32:22, Wez wrote:
> > We're in OnSessionAuthenticated(), so surely this client is authenticated,
in
> > which case the recorder will have been kept around?
> Recorder is created after client is authenticated, i.e. in this method.
Because
> we allow only one client at a time recorder_ should not exist when this method
> is called.

But by the time we reach here the client we're adding is marked as authenticated
(or might be), such that when we disconnect all other clients the recorder won't
be torn down?

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
File remoting/host/host_status_observer.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/host/host_status_obser...
remoting/host/host_status_observer.h:35: // callback; it is purely
informational.
On 2011/11/09 21:24:00, sergeyu wrote:
> On 2011/11/09 02:32:22, Wez wrote:
> > nit: Incorrect plural/singular.
> 
> Changed s/the observer/observers/, but not sure if that's the problem.

Sorry, not a very helpful comment from me.  New text looks fine.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:128: // after this method is called.
On 2011/11/09 21:24:00, sergeyu wrote:
> On 2011/11/09 02:32:22, Wez wrote:
> > nit: after this method returns?
> No - callback is not expected during this call. Previously it would call
> StateChangeCallback(CLOSED) in response to this call. Not anymore.

OK, I think it's clearer to say "from the moment this method is called";
otherwise it's ambiguous whether it's only _after_ it has been called and
returned, or whether it's also guaranteed not to make callbacks mid call.

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

http://codereview.chromium.org/8495024/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: // this method may be invoked in resonse
to a libjingle event and
typo: resonse

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

http://codereview.chromium.org/8495024/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:212: // immidiately after previous one
disconnected.
typo: immidiately

[Sergey Ulanov, 2011-11-10 21:06:14]

Two annoying things when Disconnect() method is expected to trigger a callback:
 - Disconnect() method needs to expect that the object may be destroyed in the
callback, so it needs to call the callback only at the end.
 - It's harder to mock ConnectionToHost, ClientSession because Disconnect()
method is supposed to call a callback.

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

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:180: OnSessionClosed(*other_client);
On 2011/11/09 23:18:45, Wez wrote:
> On 2011/11/09 21:24:00, sergeyu wrote:
> > On 2011/11/09 02:32:22, Wez wrote:
> > > If Disconnect() is made to trigger OnSessionClosed(), as previously
> suggested,
> > > then this can go away.
> > I don't think that Disconnect() should trigger OnSessionClosed()
> 
> Why not?  The EventHandler really only cares that the session has closed -
> whether that's because the peer closed it, or we did, doesn't seem relevant to
> the EventHandler.

Ok. I've changed Disconnect() to trigger OnSessionClosed().

http://codereview.chromium.org/8495024/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:185: CHECK(recorder_.get() == NULL);
On 2011/11/09 23:18:45, Wez wrote:
> On 2011/11/09 21:24:00, sergeyu wrote:
> > On 2011/11/09 02:32:22, Wez wrote:
> > > We're in OnSessionAuthenticated(), so surely this client is authenticated,
> in
> > > which case the recorder will have been kept around?
> > Recorder is created after client is authenticated, i.e. in this method.
> Because
> > we allow only one client at a time recorder_ should not exist when this
method
> > is called.
> 
> But by the time we reach here the client we're adding is marked as
authenticated
> (or might be), such that when we disconnect all other clients the recorder
won't
> be torn down?

I see your point now. Removed this CHECK because it is useless anyway.

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8495024/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:128: // after this method is called.
On 2011/11/09 23:18:45, Wez wrote:
> On 2011/11/09 21:24:00, sergeyu wrote:
> > On 2011/11/09 02:32:22, Wez wrote:
> > > nit: after this method returns?
> > No - callback is not expected during this call. Previously it would call
> > StateChangeCallback(CLOSED) in response to this call. Not anymore.
> 
> OK, I think it's clearer to say "from the moment this method is called";
> otherwise it's ambiguous whether it's only _after_ it has been called and
> returned, or whether it's also guaranteed not to make callbacks mid call.
Reverted behavior of this method and this comment.

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

http://codereview.chromium.org/8495024/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: // this method may be invoked in resonse
to a libjingle event and
On 2011/11/09 23:18:45, Wez wrote:
> typo: resonse

Done.

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

http://codereview.chromium.org/8495024/diff/6001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:212: // immidiately after previous one
disconnected.
On 2011/11/09 23:18:45, Wez wrote:
> typo: immidiately

Done.

[Wez, 2011-11-10 23:26:05]

On 2011/11/10 21:06:14, sergeyu wrote:
> Two annoying things when Disconnect() method is expected to trigger a
callback:
>  - Disconnect() method needs to expect that the object may be destroyed in the
> callback, so it needs to call the callback only at the end.

Unfortunately it's generally true that we have to be careful when we choose to
invoke simple callbacks.

>  - It's harder to mock ConnectionToHost, ClientSession because Disconnect()
> method is supposed to call a callback.

ConnectionToHost is really an internal implementation detail of ChromotingHost,
though, so do we even want to be mocking it for use with ChromotingHost?

[Wez, 2011-11-10 23:26:36]

On 2011/11/10 23:26:05, Wez wrote:
> On 2011/11/10 21:06:14, sergeyu wrote:
> > Two annoying things when Disconnect() method is expected to trigger a
> callback:
> >  - Disconnect() method needs to expect that the object may be destroyed in
the
> > callback, so it needs to call the callback only at the end.
> 
> Unfortunately it's generally true that we have to be careful when we choose to
> invoke simple callbacks.
> 
> >  - It's harder to mock ConnectionToHost, ClientSession because Disconnect()
> > method is supposed to call a callback.
> 
> ConnectionToHost is really an internal implementation detail of
ChromotingHost,
> though, so do we even want to be mocking it for use with ChromotingHost?

(Sorry, I mean that for ConnectionToClient..)

[Sergey Ulanov, 2011-11-10 23:46:52]

On Thu, Nov 10, 2011 at 3:26 PM, <wez@chromium.org> wrote:

> On 2011/11/10 21:06:14, sergeyu wrote:
>
>> Two annoying things when Disconnect() method is expected to trigger a
>>
> callback:
>
>>  - Disconnect() method needs to expect that the object may be destroyed
>> in the
>> callback, so it needs to call the callback only at the end.
>>
>
> Unfortunately it's generally true that we have to be careful when we
> choose to
> invoke simple callbacks.
>

True, but in this case the callback is not called explicitly:
ClientSession::Disconnect() just calls ConnectionToClient::Disconnect(),
and which triggers call ClientSession::OnConnectionClosed(), etc. Each such
case needs to be documented, and it makes interaction between layers very
convoluted.


>
>   - It's harder to mock ConnectionToHost, ClientSession because
>> Disconnect()
>> method is supposed to call a callback.
>>
>
> ConnectionToHost is really an internal implementation detail of
> ChromotingHost,
> though, so do we even want to be mocking it for use with ChromotingHost?
>

Sorry, I meant ConnectionToClient. Currently it is mocked in ChromotingHost
unittests. We could mock protocol::Session instead (not sure if that would
make the tests simpler), but protocol::Session has exactly the same problem
- state change callback is expected in response to Close() call..



>
>
>
http://codereview.chromium.**org/8495024/<http://codereview.chromium.org/8495...
>