Introducing the WorkerProcessLauncher class implementing the common logic for spawning a worker pro…

remoting/host/worker_process_launcher.cc

Index: remoting/host/worker_process_launcher.cc
diff --git a/remoting/host/worker_process_launcher.cc b/remoting/host/worker_process_launcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..378fb127abc2a19d794c8d08c289d313347ff4ba
--- /dev/null
+++ b/remoting/host/worker_process_launcher.cc
@@ -0,0 +1,133 @@
+// Copyright (c) 2012 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.
+//
+// This file implements the Windows service controlling Me2Me host processes
+// running within user sessions.

[Wez, 2012/03/15 23:24:31]

Stale comment?

[alexeypa (please no reviews), 2012/03/16 17:43:50]

Done.

+
+#include "remoting/host/worker_process_launcher.h"
+
+#include <limits>
+
+#include "base/logging.h"
+#include "base/process_util.h"
+#include "base/rand_util.h"
+#include "base/stringprintf.h"
+#include "base/threading/thread.h"
+#include "base/utf_string_conversions.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_message.h"
+#include "ipc/ipc_message_macros.h"
+
+namespace {
+
+// Generates random channel ID.
+// N.B. Stolen from src/content/common/child_process_host_impl.cc
+std::string GenerateRandomChannelId(void* instance) {

[Wez, 2012/03/15 23:24:31]

nit: Since there's only one static function, use "static" and save 4 lines?

[alexeypa (please no reviews), 2012/03/16 17:43:50]

Done.

+  return base::StringPrintf("%d.%p.%d",
+                            base::GetCurrentProcId(), instance,
+                            base::RandInt(0, std::numeric_limits<int>::max()));
+}
+
+} // namespace
+
+namespace remoting {
+
+WorkerProcessLauncher::WorkerProcessLauncher(
+    base::MessageLoopProxy* ipc_message_loop)
+    : ipc_message_loop_(ipc_message_loop),
+      listener_(NULL) {
+}
+
+WorkerProcessLauncher::~WorkerProcessLauncher() {
+  Stop();

[Wez, 2012/03/15 23:24:31]

Stop() is virtual, so someone naively deriving from this class may be surprised
to find that their Stop() override never gets called...

[alexeypa (please no reviews), 2012/03/16 17:43:50]

It is not virtual any more after refactoring the class.

+}
+
+bool WorkerProcessLauncher::Start(
+    IPC::Channel::Listener* listener,
+    const CreateChannelHandleCallback& create_channel_handle,
+    const LaunchWorkerCallback& launch_worker) {
+  DCHECK(ipc_channel_.get() == NULL);
+  DCHECK(listener_ == NULL);
+  DCHECK(process_.handle() == NULL);
+
+  std::string channel_name = GenerateRandomChannelId(this);
+
+  // Create the IPC channel offloading channel handle creation to the callback.

[Wez, 2012/03/15 23:24:31]

typo: I think you're missing a comma in here.

[alexeypa (please no reviews), 2012/03/16 17:43:50]

Commas haven't been my string side ever. I split it into two sentences. :-)

+  IPC::ChannelHandle channel_handle;
+  if (create_channel_handle.Run(channel_name, &channel_handle)) {
+    ipc_channel_.reset(new IPC::ChannelProxy(channel_handle,
+                                             IPC::Channel::MODE_SERVER,
+                                             this,
+                                             ipc_message_loop_));
+
+    // Try to launch the process and start waiting until it connects over
+    // the IPC channel.

[Wez, 2012/03/15 23:24:31]

typo: ... start waiting until ... -> ... wait until ...

[Wez, 2012/03/15 23:24:31]

So this is a blocking operation?

[alexeypa (please no reviews), 2012/03/16 17:43:50]

Rephrased.

[alexeypa (please no reviews), 2012/03/16 17:43:50]

No, why? Rephrased.

+    if (launch_worker.Run(channel_name, &process_)) {
+      listener_ = listener;
+      return true;
+    }
+
+    ipc_channel_.reset();
+  }
+
+  return false;
+}
+
+void WorkerProcessLauncher::Stop() {
+  ipc_channel_.reset();
+  listener_ = NULL;
+  process_.Terminate(0);
+  process_.Close();
+}
+
+void WorkerProcessLauncher::Send(IPC::Message* message) {
+  ipc_channel_->Send(message);
+}
+
+bool WorkerProcessLauncher::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(ipc_channel_.get() != NULL);
+  DCHECK(process_.handle() != NULL);
+
+  return listener_->OnMessageReceived(message);
+}
+
+void WorkerProcessLauncher::OnChannelConnected(int32 peer_pid) {
+  DCHECK(ipc_channel_.get() != NULL);
+  DCHECK(process_.handle() != NULL);
+
+  listener_->OnChannelConnected(peer_pid);
+}
+
+void WorkerProcessLauncher::OnChannelError() {
+  if (listener_) {

[Wez, 2012/03/15 23:24:31]

How come you need to check |listener_| here but not for the other notifications?
 Because of the OnObjectSignalled() hook in the Windows implementation?

[alexeypa (please no reviews), 2012/03/16 17:43:50]

Exactly. This check is removed now.

+    DCHECK(ipc_channel_.get() != NULL);
+
+    listener_->OnChannelError();
+    Stop();
+  }
+}
+
+#if defined(OS_POSIX)
+
+void WorkerProcessLauncher::OnChannelDenied() {
+  DCHECK(ipc_channel_.get() != NULL);
+
+  listener_->OnChannelDenied();
+  Stop();
+}
+
+void WorkerProcessLauncher::OnChannelListenError() {
+  DCHECK(ipc_channel_.get() != NULL);
+
+  listener_->OnChannelListenError();
+  Stop();
+}
+
+#endif  // OS_POSIX
+
+base::Process& WorkerProcessLauncher::get_process() {
+  return process_;
+}
+
+} // namespace remoting