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

remoting/host/worker_process_launcher.h

Index: remoting/host/worker_process_launcher.h
diff --git a/remoting/host/worker_process_launcher.h b/remoting/host/worker_process_launcher.h
new file mode 100644
index 0000000000000000000000000000000000000000..82c39151c85449b6fe709e4e35276a13f4dc4c44
--- /dev/null
+++ b/remoting/host/worker_process_launcher.h
@@ -0,0 +1,114 @@
+// 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_WORKER_PROCESS_LAUNCHER_H_
+#define REMOTING_HOST_WORKER_PROCESS_LAUNCHER_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "base/process.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "ipc/ipc_channel.h"
+
+namespace base {
+
+class MessageLoopProxy;
+
+} // namespace base
+
+namespace IPC {
+
+class ChannelProxy;
+class Message;
+
+} // namespace IPC
+
+namespace remoting {
+
+// Launches a worker process that is controlled via an IPC channel. The caller
+// is expected to provide the platform-specific code for allocation of a channel
+// handle and creation of the process.
+//
+// WorkerProcessLauncher objects are good for one process launch attempt only.
+// If anything went wrong and the caller wants to retry, another instance of

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

nit: This makes it sound like the object might be re-usable if nothing went
wrong; perhaps just remove the second sentence?

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

Done.

+// WorkerProcessLauncher must be created.
+//
+// For the caller all interaction with the spawned process is through
+// IPC::Listener interface and Send() method. In case of any error the channel
+// is closed and the workern process is terminated.

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

typo: workern

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

Done.

+class WorkerProcessLauncher : public IPC::Channel::Listener {
+ public:
+  virtual ~WorkerProcessLauncher();
+
+  // Takes the channel name and returns a channel handle for the server end of
+  // the channel.
+  typedef base::Callback<bool(const std::string&, IPC::ChannelHandle*)>
+      CreateChannelHandleCallback;

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

Why implement these as callbacks rather than as methods on an interface?

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

I've tried to make them pure virtual methods. It didn't work out even after
making WorkerProcessLauncher a Windows-specific class. For instance
WtsSessionProcessLauncher cannot inherit from WorkerProcessLauncher because the
former will retry on failure and the latter is good for one try only (on
purpose).

This means that WtsSessionProcessLauncher will use another private class, say
XxxLauncher implementing CreateChannelHandle() and LaunchWorker(). But then
XxxLauncher and WtsSessionProcessLauncher will need to share more state
(ipc_pipe_, host_binary_) or XxxLauncher should call back to
WtsSessionProcessLauncher. 

This all looks like too much of a hassle while callbacks are an easy way to
borrow a couple methods from an unrelated class and be done with it.

should implement a separate copy of IPC::Channel::Listener and not 

[Wez, 2012/03/27 16:45:45]

What I meant was an interface, not pure-virtual inheritance;
WorkerProcessLauncher would define alongside it a Delegate interface for
WtsSessionProcessLauncher to implement the relevant functions.  It's no
different from what you're doing with Callbacks except that you have a Delegate
interface.

+
+  // Launches a worker process passing the IPC channel name to it.
+  typedef base::Callback<bool(const std::string&, base::Process*)>
+      LaunchWorkerCallback;
+
+  // Starts the worker process. Returns true if the process was launched
+  // successfully. Once the process is launched and until Stop() is called
+  // channel events are delivered to |listener|. If an error occurs the channel

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

nit: Reword: "Channel events are delivered to |listener|..."

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

Done.

+  // is closed immediately after delivering the error notification to
+  // |listener|. |listener| must remain alive until Stop() is called or
+  // the final error notification is delivered.
+  //
+  // The |create_channel_handle| and |launch_worker| callbacks hide
+  // implementation details of non-portable code.
+  // |listener| must remain alive until Stop() is called.

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

nit: You mentioned that requirement earlier. :)

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

Done.

+  virtual bool Start(IPC::Channel::Listener* listener,
+                     const CreateChannelHandleCallback& create_channel_handle,
+                     const LaunchWorkerCallback& launch_worker);
+
+  // Stops the worker process. Stop() can be called multiple times.
+  virtual void Stop();
+
+  // Sends an IPC message to the worker process. This method can be called only
+  // after successfull Start() and until Stop() method is called or an error

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

nit: Remove "method" after "Stop()".

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

typo: successfull

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

Done.

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

Done.

+  // occured in the channel.

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

typo: occured

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

Done.

+  void Send(IPC::Message* message);
+
+  // IPC::Channel::Listener implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void OnChannelConnected(int32 peer_pid) OVERRIDE;
+  virtual void OnChannelError() OVERRIDE;
+
+#if defined(OS_POSIX)
+  virtual void OnChannelDenied() OVERRIDE;
+  virtual void OnChannelListenError() OVERRIDE;
+#endif  // OS_POSIX
+
+  static scoped_ptr<WorkerProcessLauncher> Create(

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

Put the Create() method earlier, with a comment explaining its use?

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

Create() was removed after making this class Windows-only.

+      base::MessageLoopProxy* ipc_message_loop);
+
+ protected:
+  WorkerProcessLauncher(base::MessageLoopProxy* ipc_message_loop);
+
+  base::Process& get_process();
+
+ private:
+  // The IPC channel to talk to the elevated process.
+  scoped_ptr<IPC::ChannelProxy> ipc_channel_;
+
+  // Message loop used by |ipc_channel_|.
+  scoped_refptr<base::MessageLoopProxy> ipc_message_loop_;
+
+  IPC::Channel::Listener* listener_;
+
+  // The handle of the worker process.
+  base::Process process_;
+
+  DISALLOW_COPY_AND_ASSIGN(WorkerProcessLauncher);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_WORKER_PROCESS_LAUNCHER_H_