Adding the base ChromotingInstance implementation and unittests. This class will be used by the ap…

remoting/test/chromoting_instance.cc

+// Copyright 2015 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/test/chromoting_instance.h"
+
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/thread_task_runner_handle.h"
+#include "jingle/glue/thread_wrapper.h"
+#include "net/base/request_priority.h"
+#include "net/socket/client_socket_factory.h"
+#include "remoting/base/url_request_context_getter.h"
+#include "remoting/client/audio_player.h"
+#include "remoting/client/chromoting_client.h"
+#include "remoting/client/client_context.h"
+#include "remoting/client/token_fetcher_proxy.h"
+#include "remoting/protocol/authentication_method.h"
+#include "remoting/protocol/chromium_port_allocator.h"
+#include "remoting/protocol/host_stub.h"
+#include "remoting/protocol/libjingle_transport_factory.h"
+#include "remoting/protocol/negotiating_client_authenticator.h"
+#include "remoting/protocol/network_settings.h"
+#include "remoting/protocol/session_config.h"
+#include "remoting/protocol/third_party_client_authenticator.h"
+#include "remoting/signaling/xmpp_signal_strategy.h"
+#include "remoting/test/remote_host_info_fetcher.h"
+#include "remoting/test/video_renderer.h"
+
+namespace {
+const char kRemotingCapabilities[] =

[Sergey Ulanov, 2015/03/09 06:44:40]

nit: empty line above this one

[joedow, 2015/03/09 21:09:25]

Done.

+    "sendInitialResolution "
+    "rateLimitResizeRequests "
+    "googleDrive "
+    "videoRecorder "
+    "casting";
+
+const char kXmppHostName[] = "talk.google.com";
+const int kXmppPortNumber = 5222;
+
+const char kConnectionStateInitializing[]  = "INITIALIZING";

[Sergey Ulanov, 2015/03/09 06:44:41]

These consts are used only to convert enums to strings for logging. It's not
useful to define consts in cases such as this. Just inline the value sin the
ToFriendlyString() functions that would make code shorter and more readable. 

[joedow, 2015/03/09 21:09:25]

Done.

+const char kConnectionStateConnecting[]    = "CONNECTING";
+const char kConnectionStateAuthenticated[] = "AUTHENTICATED";
+const char kConnectionStateConnected[]     = "CONNECTED";
+const char kConnectionStateClosed[]        = "CLOSED";
+const char kConnectionStateFailed[]        = "FAILED";
+const char kConnectionStateUnknown[]       = "UNKNOWN";
+
+const char kProtocolErrorCodeOK[]                  = "NONE";
+const char kProtocolErrorCodePeerIsOffline[]       = "PEER_IS_OFFLINE";
+const char kProtocolErrorCodeSessionRejected[]     = "SESSION_REJECTED";
+const char kProtocolErrorCodeAuthFailed[]          = "AUTHENTICATION_FAILED";
+const char kProtocolErrorCodeIncompatibleProtocol[] = "INCOMPATIBLE_PROTOCOL";
+const char kProtocolErrorCodeHostOverload[]        = "HOST_OVERLOAD";
+const char kProtocolErrorCodeChannelConnection[]   = "CHANNEL_CONNECTION_ERROR";
+const char kProtocolErrorCodeSignalingError[]      = "SIGNALING_ERROR";
+const char kProtocolErrorCodeSignalingTimeout[]    = "SIGNALING_TIMEOUT";
+const char kProtocolErrorCodeUnknown[]             = "UNKNOWN_ERROR";
+}  // namespace
+
+namespace remoting {
+namespace test {
+
+ChromotingInstance::ChromotingInstance() :
+    connection_to_host_state_(protocol::ConnectionToHost::INITIALIZING),
+    connection_error_code_(protocol::OK) {}

[Sergey Ulanov, 2015/03/09 06:44:41]

Move } to a separate line. I believe clang-format should do that for you
automatically.

[joedow, 2015/03/09 21:09:25]

Done.

+
+ChromotingInstance::~ChromotingInstance() {
+  // Since we are being destroyed, all observers should have unregistered
+  // themselves already, but we will clear the list here just in case.

[Sergey Ulanov, 2015/03/09 06:44:40]

DCHECK that the observer list is empty? Actually ObserverList can do it
automatically. Declare connection_observers_ as
ObserverList<RemoteConnectionObserver, true> for that.

[joedow, 2015/03/09 21:09:25]

Done.

+  connection_observers_.Clear();
+
+  // Ensure any connections are closed and the members are destroyed in the
+  // appropriate order.
+  EndConnection();
+}
+
+void ChromotingInstance::StartConnection(
+    const std::string& user_name,
+    const std::string& access_token,
+    const RemoteHostInfo& remote_host_info) {
+  DCHECK(!user_name.empty());
+  DCHECK(!access_token.empty());
+  DCHECK(remote_host_info.IsReadyForConnection());
+  // A valid message loop is required to create a connection.

[Sergey Ulanov, 2015/03/09 06:44:40]

nit: I don't think this check is really useful. Any code that actually needs the
message loop may check it itself or it will just crash. Also you are calling
base::ThreadTaskRunnerHandle::Get() below - it already DCHECKs that there is a
message loop. 

[joedow, 2015/03/09 21:09:25]

Done.

+  DCHECK(base::MessageLoop::current());
+
+  // Required to establish a connection to the host.
+  jingle_glue::JingleThreadWrapper::EnsureForCurrentMessageLoop();
+
+  scoped_refptr<remoting::URLRequestContextGetter> request_context_getter;

[Sergey Ulanov, 2015/03/09 06:44:41]

here and everywhere else remove remoting::

[joedow, 2015/03/09 21:09:25]

Done.

+  request_context_getter = new remoting::URLRequestContextGetter(
+      base::ThreadTaskRunnerHandle::Get(),   // network_runner
+      base::ThreadTaskRunnerHandle::Get());  // file_runner
+
+  client_context_.reset(
+      new remoting::ClientContext(base::ThreadTaskRunnerHandle::Get()));
+
+  video_renderer_.reset(new remoting::test::VideoRenderer());
+
+  chromoting_client_.reset(new remoting::ChromotingClient(
+      client_context_.get(),
+      this,  // ClientUserInterface* client_user_interface.
+      video_renderer_.get(),
+      scoped_ptr<AudioPlayer>()));
+
+  if (test_connection_to_host_) {
+    chromoting_client_->SetConnectionToHostForTests(
+        test_connection_to_host_.Pass());
+  }
+
+  XmppSignalStrategy::XmppServerConfig xmpp_server_config;
+  xmpp_server_config.host = kXmppHostName;
+  xmpp_server_config.port = kXmppPortNumber;
+  xmpp_server_config.use_tls = true;
+  xmpp_server_config.username = user_name;
+  xmpp_server_config.auth_token = access_token;
+
+  // Set up the signal strategy.  This must outlive the client object.
+  signal_strategy_.reset(new remoting::XmppSignalStrategy(
+      net::ClientSocketFactory::GetDefaultFactory(),
+      request_context_getter,
+      xmpp_server_config));
+
+  protocol::NetworkSettings network_settings(
+      protocol::NetworkSettings::NAT_TRAVERSAL_FULL);
+
+  scoped_ptr<protocol::ChromiumPortAllocator> port_allocator(
+      protocol::ChromiumPortAllocator::Create(
+          request_context_getter,
+          network_settings));

[Sergey Ulanov, 2015/03/09 06:44:40]

nit: these arguments can fit on one line. Please use clang-format.

[joedow, 2015/03/09 21:09:25]

Done.

+
+  scoped_ptr<protocol::TransportFactory> transport_factory(
+      new protocol::LibjingleTransportFactory(
+          signal_strategy_.get(),
+          port_allocator.Pass(),
+          network_settings));
+
+  scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>
+      token_fetcher(new remoting::TokenFetcherProxy(
+          base::Bind(&ChromotingInstance::FetchThirdPartyToken,
+                     remote_host_info.authorization_code,
+                     remote_host_info.shared_secret),
+          "FAKE_HOST_PUBLIC_KEY"));
+
+  fetch_secret_callback_ = base::Bind(
+      &ChromotingInstance::FetchSecretFromString,
+      remote_host_info.shared_secret);
+
+  std::vector<protocol::AuthenticationMethod> auth_methods;
+  auth_methods.push_back(protocol::AuthenticationMethod::ThirdParty());
+
+  scoped_ptr<protocol::Authenticator> authenticator(
+      new protocol::NegotiatingClientAuthenticator(
+          std::string(),  // const std::string& client_pairing_id

[Sergey Ulanov, 2015/03/09 06:44:41]

don't need to specify type in the comment.

[joedow, 2015/03/09 21:09:25]

Done.

+          std::string(),  // const std::string& shared_secret
+          std::string(),  // const std::string& authentication_tag
+          fetch_secret_callback_,
+          token_fetcher.Pass(),
+          auth_methods));
+
+  chromoting_client_->Start(
+      signal_strategy_.get(),
+      authenticator.Pass(),
+      transport_factory.Pass(),
+      remote_host_info.host_jid,
+      kRemotingCapabilities);
+}
+
+void ChromotingInstance::EndConnection() {
+  // Clearing out the client will close the connection.
+  chromoting_client_.reset();
+
+  // The signal strategy object must outlive the client so destroy it next.
+  signal_strategy_.reset();
+
+  // The connection state should been updated when the chromoting client was
+  // destroyed however we will verify and update the connection state if not.
+  if (connection_to_host_state_ != protocol::ConnectionToHost::CLOSED &&
+      connection_to_host_state_ != protocol::ConnectionToHost::FAILED &&
+      connection_error_code_ == protocol::OK) {
+    OnConnectionState(protocol::ConnectionToHost::CLOSED, protocol::OK);
+  }
+}
+
+void ChromotingInstance::AddRemoteConnectionObserver(
+    RemoteConnectionObserver* observer) {
+  DCHECK(observer);
+  DCHECK(!connection_observers_.HasObserver(observer));

[Sergey Ulanov, 2015/03/09 06:44:40]

Don't need this. ObserverList checks for it.

I would the DCHECK that observer != null. It's not very likely for it to be
useful, but feel free to keep it if you disagree.

[joedow, 2015/03/09 21:09:25]

Done.

+
+  connection_observers_.AddObserver(observer);
+}
+
+void ChromotingInstance::RemoveRemoteConnectionObserver(
+    RemoteConnectionObserver* observer) {
+  DCHECK(observer);
+  DCHECK(connection_observers_.HasObserver(observer));

[Sergey Ulanov, 2015/03/09 06:44:40]

Don't need this DCHECK.

[joedow, 2015/03/09 21:09:25]

Done.

+
+  connection_observers_.RemoveObserver(observer);
+}
+
+void ChromotingInstance::SetConnectionToHostForTests(
+    scoped_ptr<protocol::ConnectionToHost> connection_to_host) {
+  test_connection_to_host_ = connection_to_host.Pass();
+}
+
+void ChromotingInstance::OnConnectionState(
+    protocol::ConnectionToHost::State state,
+    protocol::ErrorCode error_code) {
+  DVLOG(1) << "ChromotingInstance::OnConnectionState() Called";

[Sergey Ulanov, 2015/03/09 06:44:40]

does this need to be DVLOG, given that this is test code? Use VLOG()?
can this be a single message: 
  VLOG(1) << "ChromotingInstance::OnConnectionState(" <<
              ConnectionStateToFriendlyString(state) << ", "
              ProtocolErrorToFriendlyString(error_code) << ") called";

Same for other log statements

[joedow, 2015/03/09 21:09:25]

I've been using DVLOG in my other test objects.  I'd prefer to keep these DVLOG
for the time being and then update all of the test code in a single change to
keep logging consistent.

+  DVLOG(2) << "--State: " << ConnectionStateToFriendlyString(state);
+  DVLOG(2) << "--ErrorCode: " << ProtocolErrorToFriendlyString(error_code);
+
+  connection_error_code_ = error_code;
+  connection_to_host_state_ = state;
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    ConnectionStateChanged(state, error_code));
+
+  if (connection_error_code_ != protocol::OK ||
+      connection_to_host_state_ == protocol::ConnectionToHost::CLOSED) {
+    FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                      connection_observers_,
+                      DisconnectedFromRemoteHost());
+  }
+}
+
+void ChromotingInstance::OnConnectionReady(bool ready) {
+  DVLOG(1) << "ChromotingInstance::OnConnectionReady() Called";
+  DVLOG(2) << "--ready:" << ready;
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    ConnectionReady(ready));
+
+  if (ready) {
+    FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                      connection_observers_,
+                      ConnectedToRemoteHost());
+  }
+}
+
+void ChromotingInstance::OnRouteChanged(
+    const std::string& channel_name,
+    const protocol::TransportRoute& route) {
+  DVLOG(1) << "ChromotingInstance::OnRouteChanged() Called";
+  DVLOG(2) << "--channel_name:" << channel_name;
+  DVLOG(2) << "--route:" << protocol::TransportRoute::GetTypeString(route.type);
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    RouteChanged(channel_name, route));
+}
+
+void ChromotingInstance::SetCapabilities(const std::string& capabilities) {
+  DVLOG(1) << "ChromotingInstance::SetCapabilities() Called";
+  DVLOG(2) << "--Capabilities: " << capabilities;
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    CapabilitiesSet(capabilities));
+}
+
+void ChromotingInstance::SetPairingResponse(
+    const protocol::PairingResponse& pairing_response) {
+  DVLOG(1) << "ChromotingInstance::SetPairingResponse() Called";
+  DVLOG(2) << "--client_id: " << pairing_response.client_id();
+  DVLOG(2) << "--shared_secret: " << pairing_response.shared_secret();
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    PairingResponseSet(pairing_response));
+}
+
+void ChromotingInstance::DeliverHostMessage(
+    const protocol::ExtensionMessage& message) {
+  DVLOG(1) << "ChromotingInstance::DeliverHostMessage() called";
+  DVLOG(2) << "--type: " << message.type();
+  DVLOG(2) << "--data: " << message.data();
+
+  FOR_EACH_OBSERVER(RemoteConnectionObserver,
+                    connection_observers_,
+                    HostMessageReceived(message));
+}
+
+protocol::ClipboardStub* ChromotingInstance::GetClipboardStub() {
+  DVLOG(1) << "ChromotingInstance::GetClipboardStub() Called";
+  return this;
+}
+
+protocol::CursorShapeStub* ChromotingInstance::GetCursorShapeStub() {
+  DVLOG(1) << "ChromotingInstance::GetCursorShapeStub() Called";
+  return this;
+}
+
+void ChromotingInstance::InjectClipboardEvent(
+    const protocol::ClipboardEvent& event) {
+  DVLOG(1) << "ChromotingInstance::InjectClipboardEvent() Called";
+}
+
+void ChromotingInstance::SetCursorShape(
+    const protocol::CursorShapeInfo& cursor_shape) {
+  DVLOG(1) << "ChromotingInstance::SetCursorShape() Called";
+}
+
+void ChromotingInstance::FetchSecretFromString(
+    const std::string& shared_secret,
+    bool pairing_supported,
+    const protocol::SecretFetchedCallback& secret_fetched_callback) {
+  DVLOG(1) << "ChromotingInstance::FetchSecretFromString Called";
+  secret_fetched_callback.Run(shared_secret);
+}
+
+void ChromotingInstance::FetchThirdPartyToken(
+    const std::string& authorization_token,
+    const std::string& shared_secret,
+    const GURL& token_url,
+    const std::string& host_public_key,
+    const std::string& scope,
+    base::WeakPtr<remoting::TokenFetcherProxy> token_fetcher_proxy) {
+  DVLOG(1) << "ChromotingInstance::FetchThirdPartyToken Called";
+  DVLOG(2) << "--token_url: " << token_url;
+  DVLOG(2) << "--host_public_key: " << host_public_key;
+  DVLOG(2) << "--scope: " << scope;
+
+  if (token_fetcher_proxy) {
+    token_fetcher_proxy->OnTokenFetched(authorization_token, shared_secret);
+    token_fetcher_proxy.reset();
+  } else {
+    LOG(ERROR) << "Invalid token fetcher proxy passed in";
+  }
+}
+
+const char* ChromotingInstance::ConnectionStateToFriendlyString(
+    protocol::ConnectionToHost::State state) {
+  switch (state) {
+    case protocol::ConnectionToHost::INITIALIZING:
+      return kConnectionStateInitializing;
+
+    case protocol::ConnectionToHost::CONNECTING:
+      return kConnectionStateConnecting;
+
+    case protocol::ConnectionToHost::AUTHENTICATED:
+      return kConnectionStateAuthenticated;
+
+    case protocol::ConnectionToHost::CONNECTED:
+      return kConnectionStateConnected;
+
+    case protocol::ConnectionToHost::CLOSED:
+      return kConnectionStateClosed;
+
+    case protocol::ConnectionToHost::FAILED:
+      return kConnectionStateFailed;
+
+    default:
+      LOG(ERROR) << "Unknown connection state: '" << state << "'";
+      return kConnectionStateUnknown;
+  }
+}
+
+const char* ChromotingInstance::ProtocolErrorToFriendlyString(
+    protocol::ErrorCode error_code) {
+  switch (error_code) {
+    case protocol::OK:
+      return kProtocolErrorCodeOK;
+
+    case protocol::PEER_IS_OFFLINE:
+      return kProtocolErrorCodePeerIsOffline;
+
+    case protocol::SESSION_REJECTED:
+      return kProtocolErrorCodeSessionRejected;
+
+    case protocol::AUTHENTICATION_FAILED:
+      return kProtocolErrorCodeAuthFailed;
+
+    case protocol::INCOMPATIBLE_PROTOCOL:
+      return kProtocolErrorCodeIncompatibleProtocol;
+
+    case protocol::HOST_OVERLOAD:
+      return kProtocolErrorCodeHostOverload;
+
+    case protocol::CHANNEL_CONNECTION_ERROR:
+      return kProtocolErrorCodeChannelConnection;
+
+    case protocol::SIGNALING_ERROR:
+      return kProtocolErrorCodeSignalingError;
+
+    case protocol::SIGNALING_TIMEOUT:
+      return kProtocolErrorCodeSignalingTimeout;
+
+    case protocol::UNKNOWN_ERROR:
+      return kProtocolErrorCodeUnknown;
+
+    default:
+      LOG(ERROR) << "Unrecognized error code: '" << error_code << "'";
+      return kProtocolErrorCodeUnknown;
+  }
+}
+
+}  // namespace test
+}  // namespace remoting