Move ConnectionToClient::EventHandler from ChromotingHost to ClientSession

remoting/host/chromoting_host.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/chromoting_host.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
@@ skipping 112 matching lines @@
     }
     if (shutdown_task)
       shutdown_tasks_.push_back(shutdown_task);
     if (state_ == kStopping)
       return;
     state_ = kStopping;
   }
 
   // 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();
 }
 
 void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
   DCHECK_EQ(state_, kInitial);
   status_observers_.push_back(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)));
+                            make_scoped_refptr(client)));
 }
 
-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)));
-}
-
-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;
   }
 
   if (recorder_.get())
     recorder_->UpdateSequenceNumber(sequence_number);
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // SignalStrategy::StatusObserver implementations
 void ChromotingHost::OnStateChange(
@@ skipping 96 matching lines @@
   session->set_receiver_token(
       GenerateHostAuthToken(session->initiator_token()));
 
   // Provide the Access Code as shared secret for SSL channel authentication.
   session->set_shared_secret(access_code_);
 
   *response = protocol::SessionManager::ACCEPT;
 
   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);
 }
 
 void ChromotingHost::set_protocol_config(
     protocol::CandidateSessionConfig* config) {
   DCHECK(config_.get());
   DCHECK_EQ(state_, kInitial);
   protocol_config_.reset(config);
 }
@@ skipping 26 matching lines @@
   desktop_environment_->OnPause(pause);
 }
 
 void ChromotingHost::SetUiStrings(const UiStrings& ui_strings) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
   DCHECK_EQ(state_, kInitial);
 
   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());
   }
 
   if (AuthenticatedClientsCount() == 0) {
     if (recorder_.get()) {
       // Stop the recorder if there are no more clients.
       StopScreenRecorder();
     }
 
     // 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?
 Encoder* ChromotingHost::CreateEncoder(const protocol::SessionConfig& config) {
   const protocol::ChannelConfig& video_config = config.video_config();
 
   if (video_config.codec == protocol::ChannelConfig::CODEC_VERBATIM) {
     return EncoderRowBased::CreateVerbatimEncoder();
   } else if (video_config.codec == protocol::ChannelConfig::CODEC_ZIP) {
     return EncoderRowBased::CreateZlibEncoder();
@@ skipping 23 matching lines @@
 void ChromotingHost::EnableCurtainMode(bool enable) {
   // TODO(jamiewalch): This will need to be more sophisticated when we think
   // about proper crash recovery and daemon mode.
   // TODO(wez): CurtainMode shouldn't be driven directly by ChromotingHost.
   if (is_it2me_ || enable == is_curtained_)
     return;
   desktop_environment_->curtain()->EnableCurtainMode(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());
 
   // Disconnect all other clients.
   // 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);
 
   // Create a new RecordSession if there was none.
   if (!recorder_.get()) {
     // Then we create a ScreenRecorder passing the message loops that
     // it should run on.
     Encoder* encoder = CreateEncoder(config);
 
     recorder_ = new ScreenRecorder(context_->main_message_loop(),
                                    context_->encode_message_loop(),
                                    context_->network_message_loop(),
                                    desktop_environment_->capturer(),
                                    encoder);
   }
 
   // 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();
        it != status_observers_.end(); ++it) {
     (*it)->OnClientAuthenticated(jid);
   }
   // TODO(jamiewalch): Tidy up actions to be taken on connect/disconnect,
   // including closing the connection on failure of a critical operation.
   EnableCurtainMode(true);
   if (is_it2me_) {
     std::string username = jid;
     size_t pos = username.find('/');
     if (pos != std::string::npos)
       username.replace(pos, std::string::npos, "");
     desktop_environment_->OnConnect(username);
   }
 }
 
-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());
 
   ++stopping_recorders_;
   recorder_->Stop(base::Bind(&ChromotingHost::OnScreenRecorderStopped, this));
   recorder_ = NULL;
 }
 
 void ChromotingHost::OnScreenRecorderStopped() {
@@ skipping 81 matching lines @@
 
   for (std::vector<Task*>::iterator it = shutdown_tasks_.begin();
        it != shutdown_tasks_.end(); ++it) {
     (*it)->Run();
     delete *it;
   }
   shutdown_tasks_.clear();
 }
 
 }  // namespace remoting