Move ConnectionToClient::EventHandler from ChromotingHost to ClientSession

remoting/host/chromoting_host.cc

Index: remoting/host/chromoting_host.cc
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index fe54f78561c5ce69cd4d3f7a66a1c7d35b76a467..a8f4640c65f7a0c51a19a6b4dfef4fa983e4c18a 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -130,7 +130,9 @@ void ChromotingHost::Shutdown(Task* shutdown_task) {
 
   // Disconnect all of the clients, implicitly stopping the ScreenRecorder.
   while (!clients_.empty()) {
-    OnClientDisconnected(clients_.front()->connection());
+    scoped_refptr<ClientSession> client = clients_.front();
+    client->Disconnect();
+    OnClientDisconnected(client);

[Wez, 2011/11/09 01:35:07]

It seems strange that client->Disconnect() doesn't result in OnSessionClosed(),
and thereby implicitly OnClientDisconnected().

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

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.

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

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?

[Sergey Ulanov, 2011/11/10 00:55:57]

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?
cricket::Session emits SignalState in response to Terminate() from
CloseInternal(). It then calls JingleSession::OnSessionState() which calls
OnTerminate(), and it calls CloseInternal() again.

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

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.
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().

   }
 
   ShutdownNetwork();
@@ -142,40 +144,32 @@ void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
 }
 
 ////////////////////////////////////////////////////////////////////////////
-// protocol::ConnectionToClient::EventHandler implementations
-void ChromotingHost::OnConnectionOpened(ConnectionToClient* connection) {
+// protocol::ClientSession::EventHandler implementation.
+void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
-  VLOG(1) << "Connection to client established.";
+  protocol::Session* session = client->connection()->session();
   context_->main_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ChromotingHost::ProcessPreAuthentication, this,
-                              make_scoped_refptr(connection)));
+      FROM_HERE, base::Bind(&ChromotingHost::AddAuthenticatedClient,
+                            this, make_scoped_refptr(client),
+                            session->config(), session->jid()));
 }
 
-void ChromotingHost::OnConnectionClosed(ConnectionToClient* connection) {
+void ChromotingHost::OnSessionClosed(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
   VLOG(1) << "Connection to client closed.";
   context_->main_message_loop()->PostTask(
       FROM_HERE, base::Bind(&ChromotingHost::OnClientDisconnected, this,
-                            make_scoped_refptr(connection)));
-}
-
-void ChromotingHost::OnConnectionFailed(ConnectionToClient* connection) {
-  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
-
-  LOG(ERROR) << "Connection failed unexpectedly.";
-  context_->main_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ChromotingHost::OnClientDisconnected, this,
-                            make_scoped_refptr(connection)));
+                            make_scoped_refptr(client)));
 }
 
-void ChromotingHost::OnSequenceNumberUpdated(ConnectionToClient* connection,
+void ChromotingHost::OnSessionSequenceNumber(ClientSession* session,
                                              int64 sequence_number) {
   // Update the sequence number in ScreenRecorder.
   if (MessageLoop::current() != context_->main_message_loop()) {
     context_->main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(&ChromotingHost::OnSequenceNumberUpdated, this,
-                              make_scoped_refptr(connection), sequence_number));
+        FROM_HERE, base::Bind(&ChromotingHost::OnSessionSequenceNumber, this,
+                              make_scoped_refptr(session), sequence_number));
     return;
   }
 
@@ -292,19 +286,14 @@ void ChromotingHost::OnIncomingSession(
 
   LOG(INFO) << "Client connected: " << session->jid();
 
-  // We accept the connection, so create a connection object.
-  ConnectionToClient* connection = new ConnectionToClient(
-      context_->network_message_loop(), this);
-  connection->Init(session);
-
   // Create a client object.
+  scoped_refptr<protocol::ConnectionToClient> connection =
+      new protocol::ConnectionToClient(context_->network_message_loop(),
+                                       session);
   ClientSession* client = new ClientSession(
-      this,
-      connection,
+      this, connection,
       desktop_environment_->event_executor(),
       desktop_environment_->capturer());
-  connection->set_host_stub(client);
-  connection->set_input_stub(client);
 
   clients_.push_back(client);
 }
@@ -351,32 +340,22 @@ void ChromotingHost::SetUiStrings(const UiStrings& ui_strings) {
   ui_strings_ = ui_strings;
 }
 
-void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
+void ChromotingHost::OnClientDisconnected(ClientSession* client) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
-  // Find the client session corresponding to the given connection.
+  scoped_refptr<ClientSession> client_ref = client;
+
   ClientList::iterator it;
   for (it = clients_.begin(); it != clients_.end(); ++it) {
-    if (it->get()->connection() == connection)
+    if (it->get() == client)
       break;
   }
-  if (it == clients_.end())
-    return;
-
-  scoped_refptr<ClientSession> client = *it;
-
   clients_.erase(it);
 
   if (recorder_.get()) {
-    recorder_->RemoveConnection(connection);
+    recorder_->RemoveConnection(client->connection());
   }
 
-  // Close the connection to client just to be safe.
-  // TODO(garykac): This should be removed when we revisit our shutdown and
-  // disconnect code. This should only need to be done in
-  // ClientSession::Disconnect().
-  connection->Disconnect();
-
   for (StatusObserverList::iterator it = status_observers_.begin();
        it != status_observers_.end(); ++it) {
     (*it)->OnClientDisconnected(client->client_jid());
@@ -390,12 +369,8 @@ void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
 
     // Disable the "curtain" if there are no more active clients.
     EnableCurtainMode(false);
-    if (is_it2me_) {
-      desktop_environment_->OnLastDisconnect();
-    }
+    desktop_environment_->OnLastDisconnect();
   }
-
-  client->OnDisconnected();
 }
 
 // TODO(sergeyu): Move this to SessionManager?
@@ -439,18 +414,8 @@ void ChromotingHost::EnableCurtainMode(bool enable) {
   is_curtained_ = enable;
 }
 
-void ChromotingHost::OnAuthenticationComplete(
-    scoped_refptr<ConnectionToClient> connection) {
-  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
-
-  context_->main_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ChromotingHost::AddAuthenticatedClient,
-                            this, connection, connection->session()->config(),
-                            connection->session()->jid()));
-}
-
 void ChromotingHost::AddAuthenticatedClient(
-    scoped_refptr<ConnectionToClient> connection,
+    ClientSession* client,
     const protocol::SessionConfig& config,
     const std::string& jid) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
@@ -459,13 +424,14 @@ void ChromotingHost::AddAuthenticatedClient(
   // Iterate over a copy of the list of clients, to avoid mutating the list
   // while iterating over it.
   ClientList clients_copy(clients_);
-  for (ClientList::const_iterator client = clients_copy.begin();
-       client != clients_copy.end(); client++) {
-    ConnectionToClient* connection_other = client->get()->connection();
-    if (connection_other != connection) {
-      OnClientDisconnected(connection_other);
+  for (ClientList::const_iterator other_client = clients_copy.begin();
+       other_client != clients_copy.end(); ++other_client) {
+    if ((*other_client) != client) {
+      (*other_client)->Disconnect();
+      OnClientDisconnected(*other_client);
     }
   }
+
   // Those disconnections should have killed the screen recorder.
   CHECK(recorder_.get() == NULL);
 
@@ -483,7 +449,7 @@ void ChromotingHost::AddAuthenticatedClient(
   }
 
   // Immediately add the connection and start the session.
-  recorder_->AddConnection(connection);
+  recorder_->AddConnection(client->connection());
   recorder_->Start();
   // Notify observers that there is at least one authenticated client.
   for (StatusObserverList::iterator it = status_observers_.begin();
@@ -502,22 +468,6 @@ void ChromotingHost::AddAuthenticatedClient(
   }
 }
 
-void ChromotingHost::ProcessPreAuthentication(
-    const scoped_refptr<ConnectionToClient>& connection) {
-  DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
-  // Find the client session corresponding to the given connection.
-  ClientList::iterator client;
-  for (client = clients_.begin(); client != clients_.end(); ++client) {
-    if (client->get()->connection() == connection)
-      break;
-  }
-  CHECK(client != clients_.end());
-
-  context_->network_message_loop()->PostTask(
-      FROM_HERE, base::Bind(&ClientSession::OnAuthenticationComplete,
-                            client->get()));
-}
-
 void ChromotingHost::StopScreenRecorder() {
   DCHECK(MessageLoop::current() == context_->main_message_loop());
   DCHECK(recorder_.get());