Revert 55507 - Start chromoting host in the service process though a method call

This change exposes method calls to configure the chromoting host and allow
it to be started from a method.

This will allow us to use IPC message to start the chromoting host.

TEST=unit_tests --gtest_filter=ServiceProcess*
BUG=50243, 50244

Review URL: http://codereview.chromium.org/3084024

TBR=hclam@chromium.org
Review URL: http://codereview.chromium.org/3110004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@55525 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 85 insertions, 243 deletions

diff --git a/chrome/service/service_main.cc b/chrome/service/service_main.cc
index 2670c5c..8994c12 100644
--- a/chrome/service/service_main.cc
+++ b/chrome/service/service_main.cc
@@ -11,6 +11,22 @@
 #include "chrome/service/cloud_print/cloud_print_proxy.h"
 #include "chrome/service/service_process.h"
 
+#if defined(ENABLE_REMOTING)
+#include "remoting/host/json_host_config.h"
+#include "remoting/host/chromoting_host.h"
+#include "remoting/host/chromoting_host_context.h"
+
+// This method is called as a signal that the Chromoting Host Process is
+// shutting down because of failure or a request made by the user.
+// We'll then post a task to |message_loop| to stop the chromoting host
+// context to finish the final cleanup.
+static void OnChromotingHostShutdown(
+    MessageLoop* message_loop, remoting::ChromotingHostContext* context) {
+  message_loop->PostTask(FROM_HERE,
+      NewRunnableMethod(context, &remoting::ChromotingHostContext::Stop));
+}
+#endif
+
 // Mainline routine for running as the service process.
 int ServiceProcessMain(const MainFunctionParams& parameters) {
   MessageLoopForUI main_message_loop;
@@ -24,7 +40,7 @@ int ServiceProcessMain(const MainFunctionParams& parameters) {
 #endif   // defined(OS_WIN)
 
   ServiceProcess service_process;
-  service_process.Initialize(&main_message_loop);
+  service_process.Initialize();
 
   // Enable Cloud Print if needed.
   if (parameters.command_line_.HasSwitch(switches::kEnableCloudPrintProxy)) {
@@ -34,7 +50,47 @@ int ServiceProcessMain(const MainFunctionParams& parameters) {
     service_process.GetCloudPrintProxy()->EnableForUser(lsid);
   }
 
+#if defined(ENABLE_REMOTING)
+  // Enable Chromoting Host if needed.
+  // TODO(hclam): Merge this config file with Cloud Printing.
+  // TODO(hclam): There is only start but not stop of the chromoting host
+  // process.
+  FilePath user_data_dir;
+  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
+  FilePath chromoting_config_path =
+      user_data_dir.Append(FILE_PATH_LITERAL(".ChromotingConfig.json"));
+  scoped_refptr<remoting::JsonHostConfig> chromoting_config;
+  scoped_ptr<remoting::ChromotingHostContext> chromoting_context;
+  scoped_refptr<remoting::ChromotingHost> chromoting_host;
+  if (parameters.command_line_.HasSwitch(switches::kEnableRemoting)) {
+    chromoting_config = new remoting::JsonHostConfig(
+        chromoting_config_path,
+        service_process.file_thread()->message_loop_proxy());
+    if (!chromoting_config->Read()) {
+      LOG(ERROR) << "Failed to read chromoting config file.";
+    } else {
+      chromoting_context.reset(new remoting::ChromotingHostContext());
+
+      // Create the Chromoting Host Process with the context and config.
+      chromoting_host = service_process.CreateChromotingHost(
+          chromoting_context.get(), chromoting_config);
+
+      // And start the context and the host process.
+      chromoting_context->Start();
+
+      // When ChromotingHost is shutdown because of failure or a request that
+      // we made. ShutdownChromotingTask() is calls.
+      // ShutdownChromotingTask() will then post a task to
+      // |main_message_loop| to shutdown the chromoting context.
+      chromoting_host->Start(
+          NewRunnableFunction(&OnChromotingHostShutdown,
+                              &main_message_loop, chromoting_context.get()));
+    }
+  }
+#endif
+
   MessageLoop::current()->Run();
   service_process.Teardown();
+
   return 0;
 }
diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc
index fbd9c8d..60229d6 100644
--- a/chrome/service/service_process.cc
+++ b/chrome/service/service_process.cc
@@ -16,17 +16,16 @@
 #include "net/base/network_change_notifier.h"
 
 #if defined(ENABLE_REMOTING)
-#include "remoting/base/constants.h"
 #include "remoting/base/encoder_zlib.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
-#include "remoting/host/json_host_config.h"
+#include "remoting/host/host_config.h"
 
 #if defined(OS_WIN)
 #include "remoting/host/capturer_gdi.h"
 #include "remoting/host/event_executor_win.h"
 #elif defined(OS_LINUX)
-#include "remoting/host/capturer_fake.h"
+#include "remoting/host/capturer_linux.h"
 #include "remoting/host/event_executor_linux.h"
 #elif defined(OS_MACOSX)
 #include "remoting/host/capturer_mac.h"
@@ -36,15 +35,12 @@
 
 ServiceProcess* g_service_process = NULL;
 
-ServiceProcess::ServiceProcess()
-  : shutdown_event_(true, false),
-    main_message_loop_(NULL) {
+ServiceProcess::ServiceProcess() : shutdown_event_(true, false) {
   DCHECK(!g_service_process);
   g_service_process = this;
 }
 
-bool ServiceProcess::Initialize(MessageLoop* message_loop) {
-  main_message_loop_ = message_loop;
+bool ServiceProcess::Initialize() {
   network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
   base::Thread::Options options;
   options.message_loop_type = MessageLoop::TYPE_IO;
@@ -78,16 +74,9 @@ bool ServiceProcess::Initialize(MessageLoop* message_loop) {
 }
 
 bool ServiceProcess::Teardown() {
-  if (service_prefs_.get()) {
-    service_prefs_->WritePrefs();
-    service_prefs_.reset();
-  }
+  service_prefs_->WritePrefs();
+  service_prefs_.reset();
   cloud_print_proxy_.reset();
-
-#if defined(ENABLE_REMOTING)
-  ShutdownChromotingHost();
-#endif
-
   ipc_server_.reset();
   // Signal this event before shutting down the service process. That way all
   // background threads can cleanup.
@@ -97,7 +86,6 @@ bool ServiceProcess::Teardown() {
   // The NetworkChangeNotifier must be destroyed after all other threads that
   // might use it have been shut down.
   network_change_notifier_.reset();
-
   return true;
 }
 
@@ -110,29 +98,19 @@ CloudPrintProxy* ServiceProcess::GetCloudPrintProxy() {
 }
 
 #if defined(ENABLE_REMOTING)
-bool ServiceProcess::StartChromotingHost() {
-  // We have already started.
-  if (chromoting_context_.get())
-    return true;
-
-  // Load chromoting config from the disk.
-  LoadChromotingConfig();
-
-  // Start the chromoting context first.
-  chromoting_context_.reset(new remoting::ChromotingHostContext());
-  chromoting_context_->Start();
-
-  // Create capturer, encoder and executor. The ownership will be transfered
-  // to the chromoting host.
+remoting::ChromotingHost* ServiceProcess::CreateChromotingHost(
+    remoting::ChromotingHostContext* context,
+    remoting::MutableHostConfig* config) {
   scoped_ptr<remoting::Capturer> capturer;
   scoped_ptr<remoting::Encoder> encoder;
   scoped_ptr<remoting::EventExecutor> executor;
 
+  // Select the capturer and encoder from |config|.
 #if defined(OS_WIN)
   capturer.reset(new remoting::CapturerGdi());
   executor.reset(new remoting::EventExecutorWin());
 #elif defined(OS_LINUX)
-  capturer.reset(new remoting::CapturerFake());
+  capturer.reset(new remoting::CapturerLinux());
   executor.reset(new remoting::EventExecutorLinux());
 #elif defined(OS_MACOSX)
   capturer.reset(new remoting::CapturerMac());
@@ -140,81 +118,8 @@ bool ServiceProcess::StartChromotingHost() {
 #endif
   encoder.reset(new remoting::EncoderZlib());
 
-  // Create a chromoting host object.
-  chromoting_host_ = new remoting::ChromotingHost(chromoting_context_.get(),
-                                                  chromoting_config_,
-                                                  capturer.release(),
-                                                  encoder.release(),
-                                                  executor.release());
-
-  // Then start the chromoting host.
-  // When ChromotingHost is shutdown because of failure or a request that
-  // we made OnChromotingShutdown() is calls.
-  chromoting_host_->Start(
-      NewRunnableMethod(this, &ServiceProcess::OnChromotingHostShutdown));
-  return true;
-}
-
-bool ServiceProcess::ShutdownChromotingHost() {
-  // Chromoting host doesn't exist so return true.
-  if (!chromoting_host_)
-    return true;
-
-  // Shutdown the chromoting host asynchronously. This will signal the host to
-  // shutdown, we'll actually wait for all threads to stop when we destroy
-  // the chromoting context.
-  chromoting_host_->Shutdown();
-  chromoting_host_ = NULL;
-  return true;
-}
-
-// A util function to update the login information to host config.
-static void SaveChromotingConfigFunc(remoting::JsonHostConfig* config,
-                                     const std::string& login,
-                                     const std::string& token,
-                                     const std::string& host_id,
-                                     const std::string& host_name,
-                                     const std::string& private_key) {
-  config->SetString(remoting::kXmppLoginConfigPath, login);
-  config->SetString(remoting::kXmppAuthTokenConfigPath, token);
-  config->SetString(remoting::kHostIdConfigPath, host_id);
-  config->SetString(remoting::kHostNameConfigPath, host_name);
-  config->SetString(remoting::kPrivateKeyConfigPath, private_key);
-}
-
-void ServiceProcess::SaveChromotingConfig(const std::string& login,
-                                          const std::string& token,
-                                          const std::string& host_id,
-                                          const std::string& host_name,
-                                          const std::string& private_key) {
-  // First we need to load the config first.
-  LoadChromotingConfig();
-
-  // And then do the update.
-  chromoting_config_->Update(
-      NewRunnableFunction(&SaveChromotingConfigFunc, chromoting_config_.get(),
-                          login, token, host_id, host_name, private_key));
-}
-
-void ServiceProcess::LoadChromotingConfig() {
-  // TODO(hclam): We really should be doing this on IO thread so we are not
-  // blocked on file IOs.
-  if (chromoting_config_)
-    return;
-
-  FilePath user_data_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-  FilePath chromoting_config_path =
-      user_data_dir.Append(FILE_PATH_LITERAL(".ChromotingConfig.json"));
-  chromoting_config_ = new remoting::JsonHostConfig(
-      chromoting_config_path, file_thread_->message_loop_proxy());
-  if (!chromoting_config_->Read()) {
-    LOG(INFO) << "Failed to read chromoting config file.";
-  }
-}
-
-void ServiceProcess::OnChromotingHostShutdown() {
-  // TODO(hclam): Implement.
+  return new remoting::ChromotingHost(context, config, capturer.release(),
+                                      encoder.release(), executor.release());
 }
 #endif
 
@@ -222,7 +127,3 @@ ServiceProcess::~ServiceProcess() {
   Teardown();
   g_service_process = NULL;
 }
-
-// Disable refcounting for runnable method because it is really not needed
-// when we post tasks on the main message loop.
-DISABLE_RUNNABLE_METHOD_REFCOUNT(ServiceProcess);
diff --git a/chrome/service/service_process.h b/chrome/service/service_process.h
index 599fa00..ce476c6 100644
--- a/chrome/service/service_process.h
+++ b/chrome/service/service_process.h
@@ -6,7 +6,6 @@
 #define CHROME_SERVICE_SERVICE_PROCESS_H_
 #pragma once
 
-#include "base/gtest_prod_util.h"
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/thread.h"
@@ -22,7 +21,7 @@ class NetworkChangeNotifier;
 namespace remoting {
 class ChromotingHost;
 class ChromotingHostContext;
-class JsonHostConfig;
+class MutableHostConfig;
 }
 
 // The ServiceProcess does not inherit from ChildProcess because this
@@ -32,8 +31,7 @@ class ServiceProcess {
   ServiceProcess();
   ~ServiceProcess();
 
-  // Initialize the ServiceProcess with the message loop that it should run on.
-  bool Initialize(MessageLoop* message_loop);
+  bool Initialize();
   bool Teardown();
   // TODO(sanjeevr): Change various parts of the code such as
   // net::ProxyService::CreateSystemProxyConfigService to take in
@@ -66,62 +64,26 @@ class ServiceProcess {
   }
 
   CloudPrintProxy* GetCloudPrintProxy();
-
 #if defined(ENABLE_REMOTING)
-  // Return the reference to the chromoting host only if it has started.
-  remoting::ChromotingHost* GetChromotingHost() { return chromoting_host_; }
-
-  // Start running the chromoting host asynchronously.
-  // Return true if chromoting host has started.
-  bool StartChromotingHost();
-
-  // Shutdown chromoting host. Return true if chromoting host was shutdown.
-  // The shutdown process will happen asynchronously.
-  bool ShutdownChromotingHost();
+  remoting::ChromotingHost* CreateChromotingHost(
+      remoting::ChromotingHostContext* context,
+      remoting::MutableHostConfig* config);
 #endif
 
  private:
-#if defined(ENABLE_REMOTING)
-  FRIEND_TEST(ServiceProcessTest, RunChromoting);
-  FRIEND_TEST_ALL_PREFIXES(ServiceProcessTest, RunChromotingUntilShutdown);
-
-  // Save authenication token to the json config file.
-  void SaveChromotingConfig(
-      const std::string& login,
-      const std::string& token,
-      const std::string& host_id,
-      const std::string& host_name,
-      const std::string& private_key);
-
-  // Load settings for chromoting from json file.
-  void LoadChromotingConfig();
-
-  // This method is called when chromoting is shutting down. This is virtual
-  // for used in the test.
-  virtual void OnChromotingHostShutdown();
-#endif
-
   scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_;
   scoped_ptr<base::Thread> io_thread_;
   scoped_ptr<base::Thread> file_thread_;
   scoped_ptr<CloudPrintProxy> cloud_print_proxy_;
   scoped_ptr<JsonPrefStore> service_prefs_;
   scoped_ptr<ServiceIPCServer> ipc_server_;
-
-#if defined(ENABLE_REMOTING)
-  scoped_refptr<remoting::JsonHostConfig> chromoting_config_;
-  scoped_ptr<remoting::ChromotingHostContext> chromoting_context_;
-  scoped_refptr<remoting::ChromotingHost> chromoting_host_;
-#endif
   // An event that will be signalled when we shutdown.
   base::WaitableEvent shutdown_event_;
 
-  // The main message loop for the service process.
-  MessageLoop* main_message_loop_;
-
   DISALLOW_COPY_AND_ASSIGN(ServiceProcess);
 };
 
 extern ServiceProcess* g_service_process;
 
 #endif  // CHROME_SERVICE_SERVICE_PROCESS_H_
+
diff --git a/chrome/service/service_process_unittest.cc b/chrome/service/service_process_unittest.cc
index aeea3cb..b3ba608 100644
--- a/chrome/service/service_process_unittest.cc
+++ b/chrome/service/service_process_unittest.cc
@@ -2,76 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <vector>
-
-#include "base/base64.h"
-#include "base/crypto/rsa_private_key.h"
-#include "base/message_loop.h"
-#include "base/waitable_event.h"
-#include "chrome/service/service_process.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 TEST(ServiceProcessTest, Run) {
-  MessageLoopForUI main_message_loop;
-  ServiceProcess process;
-  EXPECT_TRUE(process.Initialize(&main_message_loop));
-  EXPECT_TRUE(process.Teardown());
-}
-
-#if defined(ENABLE_REMOTING)
-// TODO(hclam): This method should be made into a util.
-static std::string GeneratePrivateKey() {
-  scoped_ptr<base::RSAPrivateKey> key(base::RSAPrivateKey::Create(2048));
-
-  std::vector<uint8> private_key_buf;
-  key->ExportPrivateKey(&private_key_buf);
-  std::string private_key_str(private_key_buf.begin(), private_key_buf.end());
-  std::string private_key_base64;
-  base::Base64Encode(private_key_str, &private_key_base64);
-  return private_key_base64;
-}
-
-TEST(ServiceProcessTest, RunChromoting) {
-  MessageLoopForUI main_message_loop;
-  ServiceProcess process;
-  EXPECT_TRUE(process.Initialize(&main_message_loop));
-
-  // Then config the chromoting host and start it.
-  process.SaveChromotingConfig("hello", "world", "it's a", "good day",
-                               GeneratePrivateKey());
-  EXPECT_TRUE(process.StartChromotingHost());
-  EXPECT_TRUE(process.ShutdownChromotingHost());
-  EXPECT_TRUE(process.Teardown());
-}
-
-class MockServiceProcess : public ServiceProcess {
- private:
-  FRIEND_TEST_ALL_PREFIXES(ServiceProcessTest, RunChromotingUntilShutdown);
-  MOCK_METHOD0(OnChromotingHostShutdown, void());
-};
-
-ACTION_P(QuitMessageLoop, message_loop) {
-  message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
-}
-
-// This test is disabled and should be moved to an offline test suite because
-// it actually trys to make connect. It'll take very long to get a failure.
-TEST(ServiceProcessTest, DISABLED_RunChromotingUntilShutdown) {
-  MessageLoopForUI main_message_loop;
-  MockServiceProcess process;
-  EXPECT_TRUE(process.Initialize(&main_message_loop));
-
-  // Expect chromoting shutdown be called because the login token is invalid.
-  EXPECT_CALL(process, OnChromotingHostShutdown())
-      .WillOnce(QuitMessageLoop(&main_message_loop));
-
-  // Then config the chromoting host and start it.
-  process.SaveChromotingConfig("hello", "world", "it's a", "good day",
-                               GeneratePrivateKey());
-  EXPECT_TRUE(process.StartChromotingHost());
-  MessageLoop::current()->Run();
-
-  EXPECT_TRUE(process.Teardown());
+  // TODO(hclam): Implement this test.
 }
-#endif  // defined(ENABLE_REMOTING)
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index e6a78f7..b62b268a 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -65,7 +65,7 @@ void HeartbeatSender::Start() {
     return;
   }
 
-  DCHECK_EQ(INITIALIZED, state_);
+  DCHECK(state_ == INITIALIZED);
   state_ = STARTED;
 
   request_.reset(jingle_client_->CreateIqRequest());
@@ -82,9 +82,7 @@ void HeartbeatSender::Stop() {
     return;
   }
 
-  // We may call Stop() even if we have not started.
-  if (state_ != STARTED)
-    return;
+  DCHECK(state_ == STARTED);
   state_ = STOPPED;
   request_.reset(NULL);
 }
diff --git a/remoting/jingle_glue/jingle_client.cc b/remoting/jingle_glue/jingle_client.cc
index b3ea334..eb89e21 100644
--- a/remoting/jingle_glue/jingle_client.cc
+++ b/remoting/jingle_glue/jingle_client.cc
@@ -76,11 +76,6 @@ void JingleClient::DoConnect(scoped_refptr<JingleChannel> channel,
 }
 
 void JingleClient::Close() {
-  // Once we are closed we really shouldn't talk to the callback again. In the
-  // case when JingleClient outlives the owner access the callback is not safe.
-  // TODO(hclam): We need to lock to reset callback.
-  callback_ = NULL;
-
   message_loop()->PostTask(
       FROM_HERE, NewRunnableMethod(this, &JingleClient::DoClose));
 }
@@ -90,14 +85,12 @@ void JingleClient::DoClose() {
 
   // If we have not yet initialized and the client is already closed then
   // don't close again.
-  if (state_ == CLOSED)
+  if (!callback_ || state_ == CLOSED)
     return;
 
-  if (client_) {
-    client_->Disconnect();
-    // Client is deleted by TaskRunner.
-    client_ = NULL;
-  }
+  client_->Disconnect();
+  // Client is deleted by TaskRunner.
+  client_ = NULL;
   tunnel_session_client_.reset();
   port_allocator_.reset();
   session_manager_.reset();
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index b1c6e1a..51afbbf 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -172,8 +172,6 @@
       'sources': [
         'host/capturer.cc',
         'host/capturer.h',
-        'host/capturer_fake.cc',
-        'host/capturer_fake.h',
         'host/chromoting_host.cc',
         'host/chromoting_host.h',
         'host/chromoting_host_context.cc',
@@ -263,6 +261,8 @@
         '../base/base.gyp:base_i18n',
       ],
       'sources': [
+        'host/capturer_fake.cc',
+        'host/capturer_fake.h',
         'host/capturer_fake_ascii.cc',
         'host/capturer_fake_ascii.h',
         'host/simple_host_process.cc',