Simplify ChildProcessHost.

Now that RunnerConnection implements ShellClientFactory, it's much simpler to bind the initial ShellClientRequest. Rip out all the special handling for Shell::CreateInstanceForFactory from the NativeRunners/ChildProcessHost.

R=rockot@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1769793002

Cr-Commit-Position: refs/heads/master@{#379497}

10 files changed, 41 insertions, 125 deletions

diff --git a/mojo/shell/native_runner.h b/mojo/shell/native_runner.h
index c152eb4..ce14f11 100644
--- a/mojo/shell/native_runner.h
+++ b/mojo/shell/native_runner.h
@@ -36,12 +36,6 @@ class NativeRunner {
       InterfaceRequest<mojom::ShellClient> request,
       const base::Callback<void(base::ProcessId)>& pid_available_callback,
       const base::Closure& app_completed_callback) = 0;
-
-  // Like Start(), but used to initialize the host for a child process started
-  // by someone else. Provides |request| via |factory|.
-  virtual void InitHost(mojom::ShellClientFactoryPtr factory,
-                        const String& name,
-                        mojom::ShellClientRequest request) = 0;
 };
 
 class NativeRunnerFactory {
diff --git a/mojo/shell/runner/host/child_process_host.cc b/mojo/shell/runner/host/child_process_host.cc
index bc95cb3..60a9947 100644
--- a/mojo/shell/runner/host/child_process_host.cc
+++ b/mojo/shell/runner/host/child_process_host.cc
@@ -53,20 +53,21 @@ ChildProcessHost::ChildProcessHost(base::TaskRunner* launch_process_runner,
       edk::CreateParentMessagePipe(primordial_pipe_token_), 0u));
 }
 
-ChildProcessHost::ChildProcessHost(mojom::ShellClientFactoryPtr factory)
-    : external_process_(true),
-      factory_(std::move(factory)),
-      start_child_process_event_(false, false),
-      weak_factory_(this) {}
-
 ChildProcessHost::~ChildProcessHost() {
   if (!app_path_.empty())
     CHECK(!factory_) << "Destroying ChildProcessHost before calling Join";
 }
 
-void ChildProcessHost::Start(const ProcessReadyCallback& callback) {
-  DCHECK(!external_process_);
+void ChildProcessHost::Start(mojom::ShellClientRequest request,
+                             const String& name,
+                             const ProcessReadyCallback& callback,
+                             const base::Closure& quit_closure) {
   DCHECK(!child_process_.IsValid());
+  // Request is invalid in child_process_host_unittest.
+  if (request.is_pending()) {
+    factory_->CreateShellClient(std::move(request), name);
+    factory_.set_connection_error_handler(quit_closure);
+  }
   launch_process_runner_->PostTaskAndReply(
       FROM_HERE,
       base::Bind(&ChildProcessHost::DoLaunch, base::Unretained(this)),
@@ -75,7 +76,7 @@ void ChildProcessHost::Start(const ProcessReadyCallback& callback) {
 }
 
 void ChildProcessHost::Join() {
-  if (factory_ && !external_process_)
+  if (factory_)
     start_child_process_event_.Wait();
 
   factory_.reset();
@@ -91,14 +92,6 @@ void ChildProcessHost::Join() {
   }
 }
 
-void ChildProcessHost::StartChild(mojom::ShellClientRequest request,
-                                  const String& name,
-                                  const base::Closure& quit_closure) {
-  DCHECK(factory_);
-  factory_->CreateShellClient(std::move(request), name);
-  factory_.set_connection_error_handler(quit_closure);
-}
-
 void ChildProcessHost::DidStart(const ProcessReadyCallback& callback) {
   if (child_process_.IsValid()) {
     callback.Run(child_process_.Pid());
@@ -109,8 +102,6 @@ void ChildProcessHost::DidStart(const ProcessReadyCallback& callback) {
 }
 
 void ChildProcessHost::DoLaunch() {
-  DCHECK(!external_process_);
-
   const base::CommandLine* parent_command_line =
       base::CommandLine::ForCurrentProcess();
   base::FilePath target_path = parent_command_line->GetProgram();
diff --git a/mojo/shell/runner/host/child_process_host.h b/mojo/shell/runner/host/child_process_host.h
index 1ee0f05..25b59dc 100644
--- a/mojo/shell/runner/host/child_process_host.h
+++ b/mojo/shell/runner/host/child_process_host.h
@@ -55,32 +55,24 @@ class ChildProcessHost {
                    bool start_sandboxed,
                    const Identity& target,
                    const base::FilePath& app_path);
-  // Allows a ChildProcessHost to be instantiated for an existing channel
-  // created by someone else (e.g. an app that launched its own process).
-  explicit ChildProcessHost(mojom::ShellClientFactoryPtr factory);
   virtual ~ChildProcessHost();
 
   // |Start()|s the child process; calls |DidStart()| (on the thread on which
   // |Start()| was called) when the child has been started (or failed to start).
-  void Start(const ProcessReadyCallback& callback);
+  void Start(mojom::ShellClientRequest request,
+             const String& name,
+             const ProcessReadyCallback& callback,
+             const base::Closure& quit_closure);
 
   // Waits for the child process to terminate.
   void Join();
 
-  void StartChild(mojom::ShellClientRequest request,
-                  const String& name,
-                  const base::Closure& quit_closure);
-
  protected:
   void DidStart(const ProcessReadyCallback& callback);
 
  private:
   void DoLaunch();
 
-  // If |true|, the hosted process is neither launched nor owned by this
-  // ChildProcessHost.
-  bool external_process_ = false;
-
   scoped_refptr<base::TaskRunner> launch_process_runner_;
   NativeRunnerDelegate* delegate_ = nullptr;
   bool start_sandboxed_ = false;
diff --git a/mojo/shell/runner/host/child_process_host_unittest.cc b/mojo/shell/runner/host/child_process_host_unittest.cc
index 9209115..2f57bf6 100644
--- a/mojo/shell/runner/host/child_process_host_unittest.cc
+++ b/mojo/shell/runner/host/child_process_host_unittest.cc
@@ -96,7 +96,9 @@ TEST(ChildProcessHostTest, MAYBE_StartJoin) {
                                       Identity(), base::FilePath());
   base::RunLoop run_loop;
   child_process_host.Start(
-      base::Bind(&ProcessReadyCallbackAdapater, run_loop.QuitClosure()));
+      mojom::ShellClientRequest(), String(),
+      base::Bind(&ProcessReadyCallbackAdapater, run_loop.QuitClosure()),
+      base::Bind(&base::DoNothing));
   run_loop.Run();
 
   child_process_host.Join();
diff --git a/mojo/shell/runner/host/in_process_native_runner.cc b/mojo/shell/runner/host/in_process_native_runner.cc
index 3c3b1be..c006a79 100644
--- a/mojo/shell/runner/host/in_process_native_runner.cc
+++ b/mojo/shell/runner/host/in_process_native_runner.cc
@@ -60,12 +60,6 @@ void InProcessNativeRunner::Start(
   pid_available_callback.Run(base::kNullProcessId);
 }
 
-void InProcessNativeRunner::InitHost(mojom::ShellClientFactoryPtr factory,
-                                     const String& name,
-                                     mojom::ShellClientRequest request) {
-  NOTREACHED();  // Can't host another process in this runner.
-}
-
 void InProcessNativeRunner::Run() {
   DVLOG(2) << "Loading/running Mojo app in process from library: "
            << app_path_.value()
diff --git a/mojo/shell/runner/host/in_process_native_runner.h b/mojo/shell/runner/host/in_process_native_runner.h
index 34d4e95..9f63396 100644
--- a/mojo/shell/runner/host/in_process_native_runner.h
+++ b/mojo/shell/runner/host/in_process_native_runner.h
@@ -37,10 +37,6 @@ class InProcessNativeRunner : public NativeRunner,
       InterfaceRequest<mojom::ShellClient> request,
       const base::Callback<void(base::ProcessId)>& pid_available_callback,
       const base::Closure& app_completed_callback) override;
-  void InitHost(
-      mojom::ShellClientFactoryPtr factory,
-      const String& name,
-      InterfaceRequest<mojom::ShellClient> request) override;
 
  private:
   // |base::DelegateSimpleThread::Delegate| method:
diff --git a/mojo/shell/runner/host/out_of_process_native_runner.cc b/mojo/shell/runner/host/out_of_process_native_runner.cc
index b7ce0fdd2a4..619bb1d 100644
--- a/mojo/shell/runner/host/out_of_process_native_runner.cc
+++ b/mojo/shell/runner/host/out_of_process_native_runner.cc
@@ -43,20 +43,10 @@ void OutOfProcessNativeRunner::Start(
 
   child_process_host_.reset(new ChildProcessHost(
       launch_process_runner_, delegate_, start_sandboxed, target, app_path));
-  child_process_host_->Start(base::Bind(
-      &OutOfProcessNativeRunner::OnProcessLaunched, base::Unretained(this),
-      base::Passed(&request), target.name(), pid_available_callback));
-}
-
-void OutOfProcessNativeRunner::InitHost(
-    mojom::ShellClientFactoryPtr factory,
-    const String& name,
-    mojom::ShellClientRequest request) {
-  child_process_host_.reset(new ChildProcessHost(std::move(factory)));
-  child_process_host_->StartChild(
-      std::move(request), name,
-      base::Bind(&OutOfProcessNativeRunner::AppCompleted,
-                 base::Unretained(this)));
+  child_process_host_->Start(std::move(request), target.name(),
+                             pid_available_callback,
+                             base::Bind(&OutOfProcessNativeRunner::AppCompleted,
+                                        base::Unretained(this)));
 }
 
 void OutOfProcessNativeRunner::AppCompleted() {
@@ -70,19 +60,6 @@ void OutOfProcessNativeRunner::AppCompleted() {
     app_completed_callback.Run();
 }
 
-void OutOfProcessNativeRunner::OnProcessLaunched(
-    mojom::ShellClientRequest request,
-    const String& name,
-    const base::Callback<void(base::ProcessId)>& pid_available_callback,
-    base::ProcessId pid) {
-  DCHECK(child_process_host_);
-  child_process_host_->StartChild(
-      std::move(request), name,
-      base::Bind(&OutOfProcessNativeRunner::AppCompleted,
-                 base::Unretained(this)));
-  pid_available_callback.Run(pid);
-}
-
 OutOfProcessNativeRunnerFactory::OutOfProcessNativeRunnerFactory(
     base::TaskRunner* launch_process_runner,
     NativeRunnerDelegate* delegate)
diff --git a/mojo/shell/runner/host/out_of_process_native_runner.h b/mojo/shell/runner/host/out_of_process_native_runner.h
index 8d4ce10..d56fd6c 100644
--- a/mojo/shell/runner/host/out_of_process_native_runner.h
+++ b/mojo/shell/runner/host/out_of_process_native_runner.h
@@ -40,20 +40,10 @@ class OutOfProcessNativeRunner : public NativeRunner {
       mojom::ShellClientRequest request,
       const base::Callback<void(base::ProcessId)>& pid_available_callback,
       const base::Closure& app_completed_callback) override;
-  void InitHost(mojom::ShellClientFactoryPtr factory,
-                const String& name,
-                mojom::ShellClientRequest request) override;
 
  private:
   void AppCompleted();
 
-  // Callback run when the child process has launched.
-  void OnProcessLaunched(
-      mojom::ShellClientRequest request,
-      const String& name,
-      const base::Callback<void(base::ProcessId)>& pid_available_callback,
-      base::ProcessId pid);
-
   base::TaskRunner* const launch_process_runner_;
   NativeRunnerDelegate* delegate_;
 
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index ca445dd..c6e0a43 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -111,20 +111,6 @@ class Shell::Instance : public mojom::Connector,
     return runner;
   }
 
-  scoped_ptr<NativeRunner> StartWithFactory(
-      mojom::ShellClientFactoryPtr shell_client_factory,
-      const String& name,
-      mojom::ShellClientRequest request,
-      mojom::PIDReceiverRequest pid_receiver_request,
-      NativeRunnerFactory* factory) {
-    pid_receiver_binding_.Bind(std::move(pid_receiver_request));
-    scoped_ptr<NativeRunner> runner = factory->Create(base::FilePath());
-    runner_ = runner.get();
-    runner_->InitHost(std::move(shell_client_factory), name,
-                      std::move(request));
-    return runner;
-  }
-
   mojom::InstanceInfoPtr CreateInstanceInfo() const {
     mojom::InstanceInfoPtr info(mojom::InstanceInfo::New());
     info->id = id_;
@@ -209,9 +195,18 @@ class Shell::Instance : public mojom::Connector,
     std::string user_id_string = user_id;
     if (user_id_string == mojom::kInheritUserID)
       user_id_string = identity_.user_id();
-    shell_->CreateInstanceForFactory(std::move(factory), name, user_id_string,
-                                     std::move(filter),
-                                     std::move(pid_receiver));
+
+    // We don't call ConnectToClient() here since the instance was created
+    // manually by other code, not in response to a Connect() request. The newly
+    // created instance is identified by |name| and may be subsequently reached
+    // by client code using this identity.
+    Identity target_id(name, std::string(), user_id_string);
+    target_id.set_filter(filter->filter.To<CapabilityFilter>());
+    mojom::ShellClientRequest request;
+    Instance* instance = shell_->CreateInstance(target_id, &request);
+    instance->pid_receiver_binding_.Bind(std::move(pid_receiver));
+    instance->factory_ = std::move(factory);
+    instance->factory_->CreateShellClient(std::move(request), name);
     callback.Run(mojom::ConnectResult::OK);
   }
   void AddInstanceListener(mojom::InstanceListenerPtr listener) override {
@@ -243,6 +238,7 @@ class Shell::Instance : public mojom::Connector,
   Binding<mojom::PIDReceiver> pid_receiver_binding_;
   BindingSet<mojom::Connector> connectors_;
   BindingSet<mojom::Shell> shell_bindings_;
+  mojom::ShellClientFactoryPtr factory_;
   NativeRunner* runner_ = nullptr;
   base::ProcessId pid_ = base::kNullProcessId;
   base::WeakPtrFactory<Instance> weak_factory_;
@@ -441,27 +437,6 @@ Shell::Instance* Shell::CreateInstance(const Identity& target_id,
   return instance;
 }
 
-void Shell::CreateInstanceForFactory(
-    mojom::ShellClientFactoryPtr factory,
-    const std::string& name,
-    const std::string& user_id,
-    mojom::CapabilityFilterPtr filter,
-    mojom::PIDReceiverRequest pid_receiver) {
-  DCHECK(user_id != mojom::kInheritUserID);
-  // We don't call ConnectToClient() here since the instance was created
-  // manually by other code, not in response to a Connect() request. The newly
-  // created instance is identified by |name| and may be subsequently reached by
-  // client code using this identity.
-  Identity target_id(name, std::string(), mojom::kRootUserID);
-  target_id.set_filter(filter->filter.To<CapabilityFilter>());
-  mojom::ShellClientRequest request;
-  Instance* instance = CreateInstance(target_id, &request);
-  native_runners_.push_back(
-      instance->StartWithFactory(std::move(factory), name, std::move(request),
-                                 std::move(pid_receiver),
-                                 native_runner_factory_.get()));
-}
-
 void Shell::AddInstanceListener(mojom::InstanceListenerPtr listener) {
   // TODO(beng): filter instances provided by those visible to this client.
   Array<mojom::InstanceInfoPtr> instances;
diff --git a/mojo/shell/tests/lifecycle/lifecycle_unittest.cc b/mojo/shell/tests/lifecycle/lifecycle_unittest.cc
index 75e0c8e..a852f05 100644
--- a/mojo/shell/tests/lifecycle/lifecycle_unittest.cc
+++ b/mojo/shell/tests/lifecycle/lifecycle_unittest.cc
@@ -216,11 +216,13 @@ class LifecycleTest : public mojo::test::ShellTest {
     mojo::shell::mojom::ShellClientFactoryPtr factory;
     factory.Bind(mojo::InterfacePtrInfo<mojo::shell::mojom::ShellClientFactory>(
         std::move(pipe), 0u));
+    base::RunLoop loop;
     shell->CreateInstance(std::move(factory), kTestExeName,
                           mojom::kInheritUserID, std::move(filter),
                           std::move(request),
                           base::Bind(&LifecycleTest::OnConnectionCompleted,
-                                     base::Unretained(this)));
+                                     base::Unretained(this), &loop));
+    loop.Run();
 
     base::LaunchOptions options;
   #if defined(OS_WIN)
@@ -262,7 +264,10 @@ class LifecycleTest : public mojo::test::ShellTest {
     return make_scoped_ptr(state);
   }
 
-  void OnConnectionCompleted(mojom::ConnectResult result) {}
+  void OnConnectionCompleted(base::RunLoop* loop,
+                             mojom::ConnectResult result) {
+    loop->Quit();
+  }
 
   scoped_ptr<InstanceState> instances_;