[Chromoting] Moving common logic responsible for launching child processes to WorkerProcessLauncher…

remoting/host/win/worker_process_launcher.h

Index: remoting/host/win/worker_process_launcher.h
diff --git a/remoting/host/win/worker_process_launcher.h b/remoting/host/win/worker_process_launcher.h
new file mode 100644
index 0000000000000000000000000000000000000000..9d332441385e5d46b36c8cc576541dcf34452ed5
--- /dev/null
+++ b/remoting/host/win/worker_process_launcher.h
@@ -0,0 +1,131 @@
+// 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.
+
+#ifndef REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_
+#define REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_
+
+#include <windows.h>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/process.h"
+#include "base/win/scoped_handle.h"
+#include "base/win/object_watcher.h"
+#include "ipc/ipc_channel.h"
+#include "remoting/base/stoppable.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+} // namespace base
+
+namespace IPC {
+class ChannelProxy;
+class Message;
+} // namespace IPC
+
+namespace remoting {
+
+// Used to launch a worker process that is controlled via an IPC channel. All

[Wez, 2012/08/08 20:13:41]

nit: Used to launch a ... -> Launches a ...

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+// interaction with the spawned process is through the IPC::Listener and Send()
+// method. In case of error the channel is closed and the worker process is
+// terminated.
+//
+// WorkerProcessLauncher object is good for one process launch attempt only.
+class WorkerProcessLauncher
+    : public Stoppable,
+      public base::win::ObjectWatcher::Delegate,
+      public IPC::Listener {
+ public:
+  class Delegate {
+   public:
+    virtual ~Delegate();
+
+    // Starts the worker process and passes |channel_name| to it. |wait_out|
+    // receives a handle that becomes signalled once the launched process has
+    // been terminated.
+    virtual bool DoLaunchProcess(const std::string& channel_name,
+                                 base::win::ScopedHandle* wait_out) = 0;
+
+    // Terminates the worker process with the given exit code.
+    virtual void DoKillProcess(DWORD exit_code) = 0;
+
+    // Notifies that a client has been connected to the channel. |peer|
+    // is a handle of the peer process that can be used to verify identity of
+    // the client.
+    virtual void OnChannelConnected(base::win::ScopedHandle peer) = 0;

[Wez, 2012/08/08 20:13:41]

The session process launcher checks the process ID, not the handle, and this
class gets the ID from the pipe, and has to try to open the handle; why not just
pass the process-ID here?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+
+    // Processes messages sent by the client.
+    virtual bool OnMessageReceived(const IPC::Message& message) = 0;
+  };
+
+  WorkerProcessLauncher(
+      const base::Closure& stopped_callback,

[Wez, 2012/08/08 20:13:41]

nit: Clarify under what circumstances stopped_callback is invoked, and
specifically whether deleting the launcher guarantees it won't still be called,
i.e. it's not posted via a task.

[alexeypa (please no reviews), 2012/08/08 21:49:58]

This is documented in Stoppable class description.

+      scoped_refptr<base::SingleThreadTaskRunner> main_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner);

[Wez, 2012/08/08 20:13:41]

nit: Clarify what the "main" TaskRunner is used for.

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+  virtual ~WorkerProcessLauncher();
+
+  // Starts the worker process. Returns true if the process was launched
+  // successfully.
+  //
+  // |delegate| can receive messages sent over the channel the worker has been
+  // started and until it was stopped by Stop() or due to an error.

[Wez, 2012/08/08 20:13:41]

Why pass Delegate in Start(), rather than to the constructor?
Why pass stopped_callback to the constructor, rather than to Start()?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Moved the delegate to the constructor because it expresses relations between
objects.

+  void Start(Delegate* delegate, const std::string& pipe_sddl);
+
+  // Sends an IPC message to the worker process. This method can be called only
+  // after successful Start() and until Stop() is called or an error occurred.
+  void Send(IPC::Message* message);
+
+  // base::win::ObjectWatcher::Delegate implementation.
+  virtual void OnObjectSignaled(HANDLE object) OVERRIDE;
+
+  // IPC::Listener implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void OnChannelConnected(int32 peer_pid) OVERRIDE;
+  virtual void OnChannelError() OVERRIDE;
+
+ protected:
+  // Stoppable implementation.
+  virtual void DoStop() OVERRIDE;
+
+ private:
+  // Creates the server end of the Chromoting IPC channel.
+  bool CreatePipeForIpcChannel(const std::string& channel_name,
+                               const std::string& pipe_sddl,
+                               base::win::ScopedHandle* pipe_out);
+
+  // Generates random channel ID.
+  std::string GenerateRandomChannelId();
+
+  Delegate* delegate_;
+
+  // The main service message loop.
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+
+  // Message loop used by the IPC channel.
+  scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
+
+  // Used to determine when the launched process terminates.
+  base::win::ObjectWatcher process_watcher_;
+
+  // |true| if |process_watcher_| is armed.
+  bool process_watcher_armed_;
+
+  // A waiting handle that remains signalled until the launched worked is
+  // terminated.

[Wez, 2012/08/08 20:13:41]

This is the opposite of the behaviour documented on the DoLaunchProcess() API?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+  base::win::ScopedHandle wait_;

[Wez, 2012/08/08 20:13:41]

nit: Choose a more helpful name, e.g. process_exit_event_?

[alexeypa (please no reviews), 2012/08/08 21:49:58]

Done.

+
+  // The IPC channel connecting to the launched process.
+  scoped_ptr<IPC::ChannelProxy> ipc_channel_;
+
+  // The server end of the pipe.
+  base::win::ScopedHandle pipe_;
+
+  DISALLOW_COPY_AND_ASSIGN(WorkerProcessLauncher);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_WIN_WORKER_PROCESS_LAUNCHER_H_