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

Introducing the WorkerProcessLauncher class implementing the common logic for spawning a worker processes and establishing an IPC channel with the created worker.

[alexeypa (please no reviews), 2012-03-15 17:01:12]

PTAL

[Lambros, 2012-03-15 22:24:14]

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher_posix.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher_posix.cc:6: // running within user
sessions.
Drive-by: I don't think we need this class on non-Windows platforms, do we?  On
Linux, we have a script that self-daemonizes, and on Mac the host process will
be managed by launchd.

[alexeypa (please no reviews), 2012-03-15 22:27:12]

FYI

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher_posix.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher_posix.cc:6: // running within user
sessions.
On 2012/03/15 22:24:14, Lambros wrote:
> Drive-by: I don't think we need this class on non-Windows platforms, do we? 
On
> Linux, we have a script that self-daemonizes, and on Mac the host process will
> be managed by launchd.

Oops. That was a stale comment. I wasn't planning to use this class on Linux or
Mac at the moment but it is nice to have it as an available cross platform
facility.

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

I'm not sure I see the value of the WorkerProcessLauncher vs
WorkerProcessLauncherWin split - seems to be a very high boilerplate to code
ratio. :(

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/host_serv...
File remoting/host/host_service_win.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/host_serv...
remoting/host/host_service_win.cc:367: io_thread.message_loop_proxy().get());
With the change to pass a scoped_refptr<> to WtsSessionProcessLauncher, you
won't need get() here.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:6: // running within user sessions.
Stale comment?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:26: std::string
GenerateRandomChannelId(void* instance) {
nit: Since there's only one static function, use "static" and save 4 lines?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:43: Stop();
Stop() is virtual, so someone naively deriving from this class may be surprised
to find that their Stop() override never gets called...

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:56: // Create the IPC channel
offloading channel handle creation to the callback.
typo: I think you're missing a comma in here.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:65: // the IPC channel.
typo: ... start waiting until ... -> ... wait until ...

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:65: // the IPC channel.
So this is a blocking operation?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:103: if (listener_) {
How come you need to check |listener_| here but not for the other notifications?
 Because of the OnObjectSignalled() hook in the Windows implementation?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:38: // If anything went wrong and the
caller wants to retry, another instance of
nit: This makes it sound like the object might be re-usable if nothing went
wrong; perhaps just remove the second sentence?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:43: // is closed and the workern process
is terminated.
typo: workern

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:51: CreateChannelHandleCallback;
Why implement these as callbacks rather than as methods on an interface?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:59: // channel events are delivered to
|listener|. If an error occurs the channel
nit: Reword: "Channel events are delivered to |listener|..."

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:66: // |listener| must remain alive
until Stop() is called.
nit: You mentioned that requirement earlier. :)

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:75: // after successfull Start() and
until Stop() method is called or an error
nit: Remove "method" after "Stop()".

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:75: // after successfull Start() and
until Stop() method is called or an error
typo: successfull

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:76: // occured in the channel.
typo: occured

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:89: static
scoped_ptr<WorkerProcessLauncher> Create(
Put the Create() method earlier, with a comment explaining its use?

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/wts_sessi...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.h:52: base::MessageLoopProxy*
ipc_message_loop);
scoped_refptr<MessageLoopProxy>

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

Please take another look.

Note, I didn't rename worker_process_launcher.* to worker_process_launcher_win.*
just to make diffs look sane. Once the CL is reviewed I'll rename those two
files to reflect the actual class name.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/host_serv...
File remoting/host/host_service_win.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/host_serv...
remoting/host/host_service_win.cc:367: io_thread.message_loop_proxy().get());
On 2012/03/15 23:24:31, Wez wrote:
> With the change to pass a scoped_refptr<> to WtsSessionProcessLauncher, you
> won't need get() here.

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:6: // running within user sessions.
On 2012/03/15 23:24:31, Wez wrote:
> Stale comment?

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:26: std::string
GenerateRandomChannelId(void* instance) {
On 2012/03/15 23:24:31, Wez wrote:
> nit: Since there's only one static function, use "static" and save 4 lines?

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:43: Stop();
On 2012/03/15 23:24:31, Wez wrote:
> Stop() is virtual, so someone naively deriving from this class may be
surprised
> to find that their Stop() override never gets called...

It is not virtual any more after refactoring the class.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:56: // Create the IPC channel
offloading channel handle creation to the callback.
On 2012/03/15 23:24:31, Wez wrote:
> typo: I think you're missing a comma in here.

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

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:65: // the IPC channel.
On 2012/03/15 23:24:31, Wez wrote:
> typo: ... start waiting until ... -> ... wait until ...

Rephrased.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:65: // the IPC channel.
On 2012/03/15 23:24:31, Wez wrote:
> So this is a blocking operation?

No, why? Rephrased.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.cc:103: if (listener_) {
On 2012/03/15 23:24:31, Wez wrote:
>  Because of the OnObjectSignalled() hook

Exactly. This check is removed now.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:38: // If anything went wrong and the
caller wants to retry, another instance of
On 2012/03/15 23:24:31, Wez wrote:
> nit: This makes it sound like the object might be re-usable if nothing went
> wrong; perhaps just remove the second sentence?

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:43: // is closed and the workern process
is terminated.
On 2012/03/15 23:24:31, Wez wrote:
> typo: workern

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:51: CreateChannelHandleCallback;
On 2012/03/15 23:24:31, Wez wrote:
> Why implement these as callbacks rather than as methods on an interface?

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

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:59: // channel events are delivered to
|listener|. If an error occurs the channel
On 2012/03/15 23:24:31, Wez wrote:
> nit: Reword: "Channel events are delivered to |listener|..."

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:66: // |listener| must remain alive
until Stop() is called.
On 2012/03/15 23:24:31, Wez wrote:
> nit: You mentioned that requirement earlier. :)

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:75: // after successfull Start() and
until Stop() method is called or an error
On 2012/03/15 23:24:31, Wez wrote:
> nit: Remove "method" after "Stop()".

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:75: // after successfull Start() and
until Stop() method is called or an error
On 2012/03/15 23:24:31, Wez wrote:
> typo: successfull

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:76: // occured in the channel.
On 2012/03/15 23:24:31, Wez wrote:
> typo: occured

Done.

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:89: static
scoped_ptr<WorkerProcessLauncher> Create(
On 2012/03/15 23:24:31, Wez wrote:
> Put the Create() method earlier, with a comment explaining its use?

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

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/wts_sessi...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/wts_sessi...
remoting/host/wts_session_process_launcher_win.h:52: base::MessageLoopProxy*
ipc_message_loop);
On 2012/03/15 23:24:31, Wez wrote:
> scoped_refptr<MessageLoopProxy>

Done.

[Wez, 2012-03-27 16:45:44]

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
File remoting/host/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/1/remoting/host/worker_pr...
remoting/host/worker_process_launcher.h:51: CreateChannelHandleCallback;
On 2012/03/16 17:43:50, alexeypa wrote:
> On 2012/03/15 23:24:31, Wez wrote:
> > Why implement these as callbacks rather than as methods on an interface?
> 
> 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 

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.

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
File remoting/host/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.cc:114: base::GetCurrentProcId(), this,
Do we really want to include the in-memory address of this object in the channel
Id?

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.cc:118: base::Process&
WorkerProcessLauncherWin::get_process() {
nit: Prefer to keep methods defined in the order they appear in the header where
possible.

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
File remoting/host/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.h:38: // handle and creation of the
process.
nit: platform-specific -> application-specific?

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.h:43: // For the caller all interaction
with the spawned process is through
nit: ... is through the IPC::Listener ...

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.h:44: // IPC::Listener interface and
Send() method. In case of any error the channel
nit: In case of error ...

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.h:65: // |listener| can receive channel
events once the worked has been started and
typo: worked -> worker

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/worker...
remoting/host/worker_process_launcher.h:93: base::Process& get_process();
Does this still need to be protected, not private?

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/wts_se...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/wts_se...
remoting/host/wts_session_process_launcher_win.h:78: void LaunchProcess();
nit: Re-name this to LaunchWtsSessionProcess() to make its relevance over
LaunchWorker clearer. Replacing the two Callbacks with a single delegate
interface should also help with that.

https://chromiumcodereview.appspot.com/9705065/diff/6001/remoting/host/wts_se...
remoting/host/wts_session_process_launcher_win.h:90: // This is a place to store
pipe handle until the host process has been
nit: "Used to store the IPC pipe handle until the worked process has been
started."?