Move ConnectionToClient::EventHandler from ChromotingHost to ClientSession

Move ConnectionToClient::EventHandler from ChromotingHost to ClientSession

BUG=None
TEST=None

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

[Wez, 2011-11-05 01:03:26]

Sorry, I'm not clear from the CL description what the effects of this
change are, or what the key changes in responsibility between
ChromotingHost and ConnectionToClient are, to understand why this change is
desirable?

On 4 November 2011 17:12, <sergeyu@chromium.org> wrote:

> Reviewers: Wez,
>
> Description:
> Move ConnectionToClient::**EventHandler from ChromotingHost to
> ClientSession
>
> BUG=None
> TEST=None
>
>
> Please review this at
http://codereview.chromium.**org/8476018/<http://codereview.chromium.org/8476...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M remoting/host/chromoting_host.**h
>  M remoting/host/chromoting_host.**cc
>  M remoting/host/chromoting_host_**unittest.cc
>  M remoting/host/client_session.h
>  M remoting/host/client_session.**cc
>  M remoting/host/client_session_**unittest.cc
>  M remoting/host/host_mock_**objects.h
>  M remoting/host/screen_recorder_**unittest.cc
>  M remoting/protocol/connection_**to_client.h
>  M remoting/protocol/connection_**to_client.cc
>  M remoting/protocol/connection_**to_client_unittest.cc
>  M remoting/protocol/protocol_**mock_objects.h
>  M remoting/protocol/protocol_**mock_objects.cc
>
>
>

[Sergey Ulanov, 2011-11-07 18:55:41]

On 2011/11/05 01:03:26, Wez wrote:
> Sorry, I'm not clear from the CL description what the effects of this
> change are, or what the key changes in responsibility between
> ChromotingHost and ConnectionToClient are, to understand why this change is
> desirable?

There are the following layers in the host (from bottom to top):
 - protocol::Session - represents a p2p connection and can create channels for
that connection.
 - protocol::ConnectionToClient - Creates necessary channels and knows how to
read/write from them.
 - ClientSession - holds per-client state.
 - ChromotingHost - accepts incoming connections and creates ClientSession
objects. Manages objects shared between client sessions (e.g. screen recorder,
desktop environment)

Originally we didn't have ClientSession - it was added later to store auth
state, filter input events and store pressed keys. In the current code
ChromotingHost still handles events from protocol::ConnectionToClient and
interaction between ChromotingHost and ClientSession is very messy. In this CL I
moved connection events handling code from ChromotingHost to ClientSession.

> 
> On 4 November 2011 17:12, <mailto:sergeyu@chromium.org> wrote:
> 
> > Reviewers: Wez,
> >
> > Description:
> > Move ConnectionToClient::**EventHandler from ChromotingHost to
> > ClientSession
> >
> > BUG=None
> > TEST=None
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8476018/%3Chttp://codereview.chromium.org/84...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
> >
> > Affected files:
> >  M remoting/host/chromoting_host.**h
> >  M remoting/host/chromoting_host.**cc
> >  M remoting/host/chromoting_host_**unittest.cc
> >  M remoting/host/client_session.h
> >  M remoting/host/client_session.**cc
> >  M remoting/host/client_session_**unittest.cc
> >  M remoting/host/host_mock_**objects.h
> >  M remoting/host/screen_recorder_**unittest.cc
> >  M remoting/protocol/connection_**to_client.h
> >  M remoting/protocol/connection_**to_client.cc
> >  M remoting/protocol/connection_**to_client_unittest.cc
> >  M remoting/protocol/protocol_**mock_objects.h
> >  M remoting/protocol/protocol_**mock_objects.cc
> >
> >
> >

[Sergey Ulanov, 2011-11-08 19:57:29]

ping

[Wez, 2011-11-09 01:35:06]

Mostly looking good, but I've a couple of questions around the semantics of
SetEventHandler and Disconnect.

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

http://codereview.chromium.org/8476018/diff/1/remoting/host/client_session.cc...
remoting/host/client_session.cc:91: // TODO(sergeyu): Acutally authenticate the
session.
typo: Acutally

The session has already been authenticated by Talk, and then by the
session-initiation handshake.  Are you intending that OnConnectionOpened() be
called _before_ the EKE handshake has completed, in future, or is this what you
mean by "actually authenticate" here?

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

http://codereview.chromium.org/8476018/diff/1/remoting/host/client_session.h#...
remoting/host/client_session.h:61: // Disconnect the session
typo: missing .

What does it mean to "disconnect the session"?  What callbacks get invoked?  Is
this Disconnect used in all client-disconnect situations, whether locally or
remotely initiated?  Is it thread-safe?

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: OnClientDisconnected(client);
It seems strange that client->Disconnect() doesn't result in OnSessionClosed(),
and thereby implicitly OnClientDisconnected().

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:118: void AddAuthenticatedClient(ClientSession*
client,
Does this belong in the public interface, since it's an implementation detail of
OnSessionAuthenticated?

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:171: void OnClientDisconnected(ClientSession*
client);
nit: Perhaps it would be clearer to call this OnSessionDisconnected, since
"Client" refers to the same thing as "Session" in the
ClientSession::EventHandler interface, above?

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:91: // TODO(sergeyu): Acutally authenticate the
session.
typo: Acutally.
Clarify what level of authentication you are talking about here.  By the time we
reach here we have already got the client GAIA Id, authenticated by Talk, and
we've authenticated the channel against the Access Code or PIN?

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:101: Disconnect();
Could OnSessionClosed() be dispatched from Disconnect()?

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:105: protocol::ConnectionToClient* connection) {
Fetch and log some reason for the "failure"?

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:120: authenticated_ = false;
Rather than keeping a separate |authenticated_| flag and checking it here,
couldn't we just not set the input and host stubs on the connection when we're
not authenticated?

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.h:61: void Disconnect();
Clarify the semantics; does this perform the network-level Disconnect?  Does it
trigger OnSessionClosed?  Is the ConnectionToClient valid after it has returned?

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:18: #include
"remoting/base/compound_buffer.h"
Can this be removed now?  (I realise it's not part of this CL!)

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:25: static const size_t
kAverageUpdateStream = 10;
I can't see this being used anywhere, so maybe remove it while you're here?

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:40: base::Unretained(this)));
Why not do this in SetEventHandler()?  (Not saying you should, just curious as
to why it moved!)

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:152:
handler_->OnConnectionOpened(this);
Doesn't this mean we'll never get this notification if there's no handler when
the channels become ready?

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:59: // It is guaranteed that |handler|
is called only on the |message_loop|.
Update this comment now that the function signature has changed.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:62: void SetEventHandler(EventHandler*
event_handler);
Clarify the semantics?  What happens if events happen before I set a handler? 
Can I change the handler mid-session?  Can I clear the handler?

[Sergey Ulanov, 2011-11-09 19:26:13]

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: OnClientDisconnected(client);
On 2011/11/09 01:35:07, Wez wrote:
> It seems strange that client->Disconnect() doesn't result in
OnSessionClosed(),
> and thereby implicitly OnClientDisconnected().

That is intentional. I think the code is simpler when EventHandler is used only
for external events. In the next CL I also changed behavior for Session::Close()
so that it doesn't invoke StateChangeCallback.

The main problem with callbacks in response to method calls is that handler
doesn't know if the event is external. JingleSession::CloseInternal() is a good
example of how this makes code more complicated: it has to remember that the
session is being closed because CloseInternal() may be called again in
JingleSession::OnSessionState.

So in our code I think it is better to call event callbacks only in response to
external events (this doesn't apply to the observer pattern when there may be
multiple observers, e.g. HostStatusObserver). Let me know if you disagree with
this approach.

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:118: void AddAuthenticatedClient(ClientSession*
client,
On 2011/11/09 01:35:07, Wez wrote:
> Does this belong in the public interface, since it's an implementation detail
of
> OnSessionAuthenticated?
Made it private. I either case I removed this method in the next CL.

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:171: void OnClientDisconnected(ClientSession*
client);
On 2011/11/09 01:35:07, Wez wrote:
> nit: Perhaps it would be clearer to call this OnSessionDisconnected, since
> "Client" refers to the same thing as "Session" in the
> ClientSession::EventHandler interface, above?

This method is removed in the next CL.

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:91: // TODO(sergeyu): Acutally authenticate the
session.
On 2011/11/09 01:35:07, Wez wrote:
> typo: Acutally.
> Clarify what level of authentication you are talking about here.  By the time
we
> reach here we have already got the client GAIA Id, authenticated by Talk, and
> we've authenticated the channel against the Access Code or PIN?

Removed this TODO for now. The intention is that ClientSession is created in
response to incoming session (session-initiate message) and then it would invoke
OnSessionAuthenticated after session is completely authenticated (e.g. using
EKE). But I'm not yet sure yet if it's better to handle auth in ClientSession or
ConnectionToHost, so I just removed this comment.

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:101: Disconnect();
On 2011/11/09 01:35:07, Wez wrote:
> Could OnSessionClosed() be dispatched from Disconnect()?

No. In any case I removed this line in one of the next CLs.

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:105: protocol::ConnectionToClient* connection) {
On 2011/11/09 01:35:07, Wez wrote:
> Fetch and log some reason for the "failure"?

Added TODO for this.

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.cc:120: authenticated_ = false;
On 2011/11/09 01:35:07, Wez wrote:
> Rather than keeping a separate |authenticated_| flag and checking it here,
> couldn't we just not set the input and host stubs on the connection when we're
> not authenticated?

I agree it would be better, but currently ConnectionToClient expects stubs to be
set before channels are created. Added TODO in ClientSession::ClientSession() to
fix it.

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/client_session...
remoting/host/client_session.h:61: void Disconnect();
On 2011/11/09 01:35:07, Wez wrote:
> Clarify the semantics; does this perform the network-level Disconnect?  Does
it
> trigger OnSessionClosed?  Is the ConnectionToClient valid after it has
returned?
Done

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:18: #include
"remoting/base/compound_buffer.h"
On 2011/11/09 01:35:07, Wez wrote:
> Can this be removed now?  (I realise it's not part of this CL!)

Done.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:25: static const size_t
kAverageUpdateStream = 10;
On 2011/11/09 01:35:07, Wez wrote:
> I can't see this being used anywhere, so maybe remove it while you're here?

Done.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:40: base::Unretained(this)));
On 2011/11/09 01:35:07, Wez wrote:
> Why not do this in SetEventHandler()?  (Not saying you should, just curious as
> to why it moved!)

Just because I moved |session| parameter from Init() to here. SetEventHandler()
may be called more than once and we want to handle session state changes event
before SetEventHandler() is called.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.cc:152:
handler_->OnConnectionOpened(this);
On 2011/11/09 01:35:07, Wez wrote:
> Doesn't this mean we'll never get this notification if there's no handler when
> the channels become ready?
Fixed it. Now SetEventHandler() is required to be called when this object is
created.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:59: // It is guaranteed that |handler|
is called only on the |message_loop|.
On 2011/11/09 01:35:07, Wez wrote:
> Update this comment now that the function signature has changed.

Done.

http://codereview.chromium.org/8476018/diff/5001/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:62: void SetEventHandler(EventHandler*
event_handler);
On 2011/11/09 01:35:07, Wez wrote:
> Clarify the semantics?  What happens if events happen before I set a handler? 
> Can I change the handler mid-session?  Can I clear the handler?

Done.

[Wez, 2011-11-09 23:34:50]

LGTM, subject to a couple of comments, below!

http://codereview.chromium.org/8476018/diff/1/remoting/host/screen_recorder_u...
File remoting/host/screen_recorder_unittest.cc (right):

http://codereview.chromium.org/8476018/diff/1/remoting/host/screen_recorder_u...
remoting/host/screen_recorder_unittest.cc:112: TEST_F(ScreenRecorderTest,
OneRecordCycle) {
Did you mean to remove this?

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: OnClientDisconnected(client);
On 2011/11/09 19:26:13, sergeyu wrote:
> On 2011/11/09 01:35:07, Wez wrote:
> > It seems strange that client->Disconnect() doesn't result in
> OnSessionClosed(),
> > and thereby implicitly OnClientDisconnected().
> 
> That is intentional. I think the code is simpler when EventHandler is used
only
> for external events. In the next CL I also changed behavior for
Session::Close()
> so that it doesn't invoke StateChangeCallback.
> 
> The main problem with callbacks in response to method calls is that handler
> doesn't know if the event is external. JingleSession::CloseInternal() is a
good
> example of how this makes code more complicated: it has to remember that the
> session is being closed because CloseInternal() may be called again in
> JingleSession::OnSessionState.
>
> So in our code I think it is better to call event callbacks only in response
to
> external events (this doesn't apply to the observer pattern when there may be
> multiple observers, e.g. HostStatusObserver). Let me know if you disagree with
> this approach.

I think it's different in this case.  ChromotingHost just wants to know when the
session is closed.  It's never going to try to Disconnect() it in response to
that, under any circumstances.  It might Disconnect() a session in
OnSessionAuthenticated(), for example, but that shouldn't be a problem case.  So
in this case it seems simpler to have Disconnect() trigger the OnSessionClosed()
notification, so that ChromotingHost doesn't need to care why it was closed.

In the case of JingleSession, what is the session state change we get caused by
CloseInternal?

http://codereview.chromium.org/8476018/diff/15001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8476018/diff/15001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:90: DCHECK(handler_);
Doesn't this mean we can't handle state change events until a handler is set?  I
thought that was something you did want to allow?

[Sergey Ulanov, 2011-11-10 00:55:56]

http://codereview.chromium.org/8476018/diff/1/remoting/host/screen_recorder_u...
File remoting/host/screen_recorder_unittest.cc (right):

http://codereview.chromium.org/8476018/diff/1/remoting/host/screen_recorder_u...
remoting/host/screen_recorder_unittest.cc:112: TEST_F(ScreenRecorderTest,
OneRecordCycle) {
On 2011/11/09 23:34:50, Wez wrote:
> Did you mean to remove this?
Alpha removed it (http://codereview.chromium.org/8342040 ), not sure why. I just
synced his change.

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: OnClientDisconnected(client);
On 2011/11/09 23:34:50, Wez wrote:
> On 2011/11/09 19:26:13, sergeyu wrote:
> > On 2011/11/09 01:35:07, Wez wrote:
> > > It seems strange that client->Disconnect() doesn't result in
> > OnSessionClosed(),
> > > and thereby implicitly OnClientDisconnected().
> > 
> > That is intentional. I think the code is simpler when EventHandler is used
> only
> > for external events. In the next CL I also changed behavior for
> Session::Close()
> > so that it doesn't invoke StateChangeCallback.
> > 
> > The main problem with callbacks in response to method calls is that handler
> > doesn't know if the event is external. JingleSession::CloseInternal() is a
> good
> > example of how this makes code more complicated: it has to remember that the
> > session is being closed because CloseInternal() may be called again in
> > JingleSession::OnSessionState.
> >
> > So in our code I think it is better to call event callbacks only in response
> to
> > external events (this doesn't apply to the observer pattern when there may
be
> > multiple observers, e.g. HostStatusObserver). Let me know if you disagree
with
> > this approach.
> 
> I think it's different in this case.  ChromotingHost just wants to know when
the
> session is closed.  It's never going to try to Disconnect() it in response to
> that, under any circumstances.  It might Disconnect() a session in
> OnSessionAuthenticated(), for example, but that shouldn't be a problem case. 
So
> in this case it seems simpler to have Disconnect() trigger the
OnSessionClosed()
> notification, so that ChromotingHost doesn't need to care why it was closed.

I agree that it may not be a problem in this particular case, but I think it's
better when callback semantic is consistent within our code. Calling
OnSessionClosed() in response to Disconnect may be confusing, e.g. if
Disconnect() is called multiple times, do we call OnSessionClosed() every time?

> In the case of JingleSession, what is the session state change we get caused
by
> CloseInternal?

cricket::Session emits SignalState in response to Terminate() from
CloseInternal(). It then calls JingleSession::OnSessionState() which calls
OnTerminate(), and it calls CloseInternal() again.

http://codereview.chromium.org/8476018/diff/15001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8476018/diff/15001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:90: DCHECK(handler_);
On 2011/11/09 23:34:50, Wez wrote:
> Doesn't this mean we can't handle state change events until a handler is set? 
I
> thought that was something you did want to allow?

SetEventHandler() is required to be called after ConnectionToClient is created.
I think it's an acceptable requirement, as we want to make sure that connection
events are handled by the upper layer.

[Wez, 2011-11-10 01:25:12]

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

http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:135: OnClientDisconnected(client);
On 2011/11/10 00:55:57, sergeyu wrote:
> I agree that it may not be a problem in this particular case, but I think it's
> better when callback semantic is consistent within our code. Calling
> OnSessionClosed() in response to Disconnect may be confusing, e.g. if
> Disconnect() is called multiple times, do we call OnSessionClosed() every
time?

The caller can't touch the session object after Disconnect(), so that can't
happen in the first place.  If it could happen, OnSessionClosed() would only
trigger the first time, since that's when the session is closed; the
notification should fire on the transition into the closed state.

> > In the case of JingleSession, what is the session state change we get caused
> by
> > CloseInternal?
> 
> cricket::Session emits SignalState in response to Terminate() from
> CloseInternal(). It then calls JingleSession::OnSessionState() which calls
> OnTerminate(), and it calls CloseInternal() again.

That strikes me as a but in cricket::Session; if the state hasn't changed then
it shouldn't be emitting anything.  Given that it does, OnTerminate shouldn't
really be getting notified multiple times by OnSessionState().

[Sergey Ulanov, 2011-11-10 02:30:46]

On Wed, Nov 9, 2011 at 5:25 PM, <wez@chromium.org> wrote:

>
> http://codereview.chromium.**org/8476018/diff/5001/**
>
remoting/host/chromoting_host.**cc<http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_host.cc>
> File remoting/host/chromoting_host.**cc (right):
>
> http://codereview.chromium.**org/8476018/diff/5001/**
>
remoting/host/chromoting_host.**cc#newcode135<http://codereview.chromium.org/8476018/diff/5001/remoting/host/chromoting_host.cc#newcode135>
> remoting/host/chromoting_host.**cc:135: OnClientDisconnected(client);
> On 2011/11/10 00:55:57, sergeyu wrote:
>
>> I agree that it may not be a problem in this particular case, but I
>>
> think it's
>
>> better when callback semantic is consistent within our code. Calling
>> OnSessionClosed() in response to Disconnect may be confusing, e.g. if
>> Disconnect() is called multiple times, do we call OnSessionClosed()
>>
> every time?
>
> The caller can't touch the session object after Disconnect(), so that
> can't happen in the first place.  If it could happen, OnSessionClosed()
> would only trigger the first time, since that's when the session is
> closed; the notification should fire on the transition into the closed
> state.


Ok. I'll switch to calling callback in response to Disconnect() (I'm still
not it a better approach :)). I cannot do it in this CL because
ConnectionToHost.Disconnect() switches threads. But I'll see you in the
next CL you reviewed.


>
>  > In the case of JingleSession, what is the session state change we
>>
> get caused
>
>> by
>> > CloseInternal?
>>
>
>  cricket::Session emits SignalState in response to Terminate() from
>> CloseInternal(). It then calls JingleSession::OnSessionState(**) which
>>
> calls
>
>> OnTerminate(), and it calls CloseInternal() again.
>>
>
> That strikes me as a but in cricket::Session; if the state hasn't
> changed then it shouldn't be emitting anything.  Given that it does,
> OnTerminate shouldn't really be getting notified multiple times by
> OnSessionState().
>
It correctly emits this signal only the first time. The problem is that
OnTerminate doesn't know the reason it is changing (i.g. if it because peer
has terminated the session or because Close() is called locally) and so it
calls CloseInternal() again..


>
>
http://codereview.chromium.**org/8476018/<http://codereview.chromium.org/8476...
>

[commit-bot: I haz the power, 2011-11-10 02:30:51]

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

[commit-bot: I haz the power, 2011-11-10 02:30:54]

Can't apply patch for file remoting/host/chromoting_host_unittest.cc.
While running patch -p1 --forward --force;
patching file remoting/host/chromoting_host_unittest.cc
Hunk #5 FAILED at 171.
Hunk #6 succeeded at 205 (offset -5 lines).
Hunk #7 FAILED at 239.
Hunk #8 succeeded at 279 (offset -5 lines).
Hunk #9 succeeded at 378 (offset -5 lines).
Hunk #10 succeeded at 444 (offset -5 lines).
2 out of 10 hunks FAILED -- saving rejects to file
remoting/host/chromoting_host_unittest.cc.rej

[Wez, 2011-11-10 02:45:48]

On 2011/11/10 02:30:46, sergeyu wrote:
> Ok. I'll switch to calling callback in response to Disconnect() (I'm still
> not it a better approach :)). I cannot do it in this CL because
> ConnectionToHost.Disconnect() switches threads. But I'll see you in the
> next CL you reviewed.

Thank you for humouring me!  Let's see how it then looks and if it causes pain,
we can switch back.

> > That strikes me as a but in cricket::Session; if the state hasn't
> > changed then it shouldn't be emitting anything.  Given that it does,
> > OnTerminate shouldn't really be getting notified multiple times by
> > OnSessionState().
> >
> It correctly emits this signal only the first time. The problem is that
> OnTerminate doesn't know the reason it is changing (i.g. if it because peer
> has terminated the session or because Close() is called locally) and so it
> calls CloseInternal() again..

Ah, right.  That can surely be addressed by splitting CloseInternal into two
functions, one akin to Disconnect() above, that calling code uses to cause a
disconnect, and a separate handler for the state change to closed, that does the
rest of CloseInternal's work?

[commit-bot: I haz the power, 2011-11-10 03:01:54]

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

[commit-bot: I haz the power, 2011-11-10 03:01:58]

Can't apply patch for file remoting/host/chromoting_host_unittest.cc.
While running patch -p1 --forward --force;
patching file remoting/host/chromoting_host_unittest.cc
Hunk #5 FAILED at 171.
Hunk #6 succeeded at 205 (offset -5 lines).
Hunk #7 FAILED at 239.
Hunk #8 succeeded at 279 (offset -5 lines).
Hunk #9 succeeded at 378 (offset -5 lines).
Hunk #10 succeeded at 444 (offset -5 lines).
2 out of 10 hunks FAILED -- saving rejects to file
remoting/host/chromoting_host_unittest.cc.rej

[commit-bot: I haz the power, 2011-11-10 03:04:44]

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

[commit-bot: I haz the power, 2011-11-10 05:02:50]

Try job failure for 8476018-20003 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...