Change primordial pipes to ShellClient

mojo/shell/shell.cc

Index: mojo/shell/shell.cc
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index 8056a60651a531117d14f516aeadad70061bb2b5..bcdd1d1f400798c12f1619c78cb587d7b0887d07 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -100,8 +100,7 @@ class Shell::Instance : public mojom::Connector,
                         public InterfaceFactory<mojom::Shell>,
                         public mojom::Shell {
  public:
-  Instance(mojom::ShellClientPtr shell_client,
-           mojo::shell::Shell* shell,
+  Instance(mojo::shell::Shell* shell,
            const Identity& identity,
            const CapabilitySpec& capability_spec)
     : shell_(shell),
@@ -109,7 +108,6 @@ class Shell::Instance : public mojom::Connector,
       identity_(identity),
       capability_spec_(capability_spec),
       allow_any_application_(capability_spec.required.count("*") == 1),
-      shell_client_(std::move(shell_client)),
       pid_receiver_binding_(this),
       weak_factory_(this) {
     if (identity_.name() == kShellName ||
@@ -117,9 +115,6 @@ class Shell::Instance : public mojom::Connector,
       pid_ = base::Process::Current().Pid();
     }
     DCHECK_NE(mojom::kInvalidInstanceID, id_);
-
-    shell_client_.set_connection_error_handler(
-        base::Bind(&Instance::OnShellClientLost, base::Unretained(this)));
   }
 
   ~Instance() override {}
@@ -146,13 +141,8 @@ class Shell::Instance : public mojom::Connector,
     }
   }
 
-  void InitializeClient() {
-    shell_client_->Initialize(mojom::Identity::From(identity_), id_,
-                              base::Bind(&Instance::OnInitializeResponse,
-                                         base::Unretained(this)));
-  }
-
   void ConnectToClient(scoped_ptr<ConnectParams> params) {
+    CHECK(shell_client_.is_bound());
     params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED,
                                    identity_.user_id(), id_);
     uint32_t source_id = mojom::kInvalidInstanceID;
@@ -170,28 +160,38 @@ class Shell::Instance : public mojom::Connector,
         mojom::CapabilityRequest::From(spec), params->target().name());
   }
 
+  void StartWithClient(mojom::ShellClientPtr client) {
+    CHECK(!shell_client_);
+    shell_client_ = std::move(client);
+    shell_client_.set_connection_error_handler(
+        base::Bind(&Instance::OnShellClientLost, base::Unretained(this)));
+    shell_client_->Initialize(mojom::Identity::From(identity_), id_,
+                              base::Bind(&Instance::OnInitializeResponse,
+                                         base::Unretained(this)));
+  }
+
   void StartWithClientProcessConnection(
-      mojom::ShellClientRequest request,
       mojom::ClientProcessConnectionPtr client_process_connection) {
-    factory_.Bind(mojom::ShellClientFactoryPtrInfo(
-        std::move(client_process_connection->shell_client_factory), 0u));
+    mojom::ShellClientPtr client;
+    client.Bind(mojom::ShellClientPtrInfo(
+        std::move(client_process_connection->shell_client), 0));
     pid_receiver_binding_.Bind(
         std::move(client_process_connection->pid_receiver_request));
-    factory_->CreateShellClient(std::move(request), identity_.name());
+    StartWithClient(std::move(client));
   }
 
-  void StartWithFilePath(mojom::ShellClientRequest request,
-                         const base::FilePath& path) {
+  void StartWithFilePath(const base::FilePath& path) {
+    CHECK(!shell_client_);
     scoped_ptr<NativeRunner> runner =
         shell_->native_runner_factory_->Create(path);
     bool start_sandboxed = false;
-    runner->Start(path, identity_, start_sandboxed, std::move(request),
-                  base::Bind(&Instance::PIDAvailable,
-                             weak_factory_.GetWeakPtr()),
-                  base::Bind(&mojo::shell::Shell::CleanupRunner,
-                             shell_->weak_ptr_factory_.GetWeakPtr(),
-                             runner.get()));
+    mojom::ShellClientPtr client = runner->Start(
+        path, identity_, start_sandboxed,
+        base::Bind(&Instance::PIDAvailable, weak_factory_.GetWeakPtr()),
+        base::Bind(&mojo::shell::Shell::CleanupRunner,
+                   shell_->weak_ptr_factory_.GetWeakPtr(), runner.get()));
     shell_->native_runners_.push_back(std::move(runner));
+    StartWithClient(std::move(client));
   }
 
   mojom::InstanceInfoPtr CreateInstanceInfo() const {
@@ -295,9 +295,9 @@ class Shell::Instance : public mojom::Connector,
         return false;
       }
 
-      if (!(*client_process_connection)->shell_client_factory.is_valid() ||
+      if (!(*client_process_connection)->shell_client.is_valid() ||
           !(*client_process_connection)->pid_receiver_request.is_valid()) {
-        LOG(ERROR) << "Error: must supply both shell_client_factory AND "
+        LOG(ERROR) << "Error: must supply both shell_client AND "
                    << "pid_receiver_request when sending "
                    << "client_process_connection.";
         callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
@@ -389,7 +389,6 @@ 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_;
@@ -418,8 +417,9 @@ Shell::Shell(scoped_ptr<NativeRunnerFactory> native_runner_factory,
       weak_ptr_factory_(this) {
   mojom::ShellClientPtr client;
   mojom::ShellClientRequest request = GetProxy(&client);
-  CreateInstance(CreateShellIdentity(), GetPermissiveCapabilities(),
-                 std::move(client));
+  Instance* instance = CreateInstance(CreateShellIdentity(),
+                                      GetPermissiveCapabilities());
+  instance->StartWithClient(std::move(client));
   shell_connection_.reset(new ShellConnection(this, std::move(request)));
 
   if (catalog)
@@ -489,7 +489,8 @@ bool Shell::AcceptConnection(Connection* connection) {
 
 void Shell::InitCatalog(mojom::ShellClientPtr catalog) {
   Identity identity(kCatalogName, mojom::kRootUserID);
-  CreateInstance(identity, CapabilitySpec(), std::move(catalog));
+  Instance* instance = CreateInstance(identity, CapabilitySpec());
+  instance->StartWithClient(std::move(catalog));
   shell_connection_->connector()->ConnectToInterface(
       kCatalogName, &shell_resolver_);
 
@@ -571,10 +572,9 @@ bool Shell::ConnectToExistingInstance(scoped_ptr<ConnectParams>* params) {
 }
 
 Shell::Instance* Shell::CreateInstance(const Identity& target,
-                                       const CapabilitySpec& spec,
-                                       mojom::ShellClientPtr client) {
+                                       const CapabilitySpec& spec) {
   CHECK(target.user_id() != mojom::kInheritUserID);
-  Instance* instance = new Instance(std::move(client), this, target, spec);
+  Instance* instance = new Instance(this, target, spec);
   DCHECK(identity_to_instance_.find(target) ==
          identity_to_instance_.end());
   identity_to_instance_[target] = instance;
@@ -583,7 +583,6 @@ Shell::Instance* Shell::CreateInstance(const Identity& target,
       [this, &info](mojom::InstanceListener* listener) {
         listener->InstanceCreated(info.Clone());
       });
-  instance->InitializeClient();
   return instance;
 }
 
@@ -597,10 +596,10 @@ void Shell::AddInstanceListener(mojom::InstanceListenerPtr listener) {
   instance_listeners_.AddInterfacePtr(std::move(listener));
 }
 
-void Shell::CreateShellClient(const Identity& source,
-                              const Identity& shell_client_factory,
-                              const std::string& name,
-                              mojom::ShellClientRequest request) {
+void Shell::CreateShellClientWithFactory(const Identity& source,
+                                         const Identity& shell_client_factory,
+                                         const std::string& name,
+                                         mojom::ShellClientRequest request) {
   mojom::ShellClientFactory* factory =
       GetShellClientFactory(shell_client_factory, source);
   factory->CreateShellClient(std::move(request), name);
@@ -662,42 +661,40 @@ void Shell::OnGotResolvedName(scoped_ptr<ConnectParams> params,
 
   mojom::ClientProcessConnectionPtr client_process_connection =
       params->TakeClientProcessConnection();
-
-  mojom::ShellClientRequest request;
-  if (!client.is_bound())
-    request = GetProxy(&client);
-
-  Instance* instance = CreateInstance(target, capabilities, std::move(client));
-  instance->ConnectToClient(std::move(params));
-
-  // If a ShellClientPtr was provided, there's no more work to do: someone
-  // is already holding a corresponding ShellClientRequest.
-  if (!request.is_pending())
-    return;
-
-  if (client_process_connection.is_null() && LoadWithLoader(target, &request))
-    return;
-
-  CHECK(!file_url.is_null() && !capabilities_ptr.is_null());
-
-  if (target.name() != resolved_name) {
-    // In cases where a package alias is resolved, we have to use the instance
-    // from the original request rather than for the package itself, which will
-    // always be the same.
-    CreateShellClient(
-        source, Identity(resolved_name, target.user_id(), instance_name),
-        target.name(), std::move(request));
+  Instance* instance = CreateInstance(target, capabilities);
+
+  // Below are various paths through which a new Instance can be bound to a
+  // ShellClient proxy.
+  if (client.is_bound()) {

[Ken Rockot(use gerrit already), 2016/03/14 19:30:07]

Not crazy about how this logic panned out. We should be able to simplify at
least by getting rid of Loader.

+    // If a ShellClientPtr was provided, there's no more work to do: someone
+    // is already holding a corresponding ShellClientRequest.
+    instance->StartWithClient(std::move(client));
+  } else if (!client_process_connection.is_null()) {
+    // Likewise if a ClientProcessConnection was given via Connect(), it
+    // provides the ShellClient proxy to use.
+    instance->StartWithClientProcessConnection(
+        std::move(client_process_connection));
   } else {
-    if (!client_process_connection.is_null()) {
-      // The client already started a process for this instance, use it.
-      instance->StartWithClientProcessConnection(
-          std::move(request), std::move(client_process_connection));
+    // Otherwise we create a new ShellClient pipe.
+    mojom::ShellClientRequest request = GetProxy(&client);
+    if (LoadWithLoader(target, &request)) {
+      instance->StartWithClient(std::move(client));
     } else {
-      // Otherwise we make our own process.
-      instance->StartWithFilePath(std::move(request),
-                                  util::UrlToFilePath(file_url.To<GURL>()));
+      CHECK(!file_url.is_null() && !capabilities_ptr.is_null());
+
+      if (target.name() != resolved_name) {
+        instance->StartWithClient(std::move(client));
+        CreateShellClientWithFactory(
+            source, Identity(resolved_name, target.user_id(), instance_name),
+            target.name(), std::move(request));
+      } else {
+        instance->StartWithFilePath(util::UrlToFilePath(file_url.To<GURL>()));
+      }
     }
   }
+
+  // Now that the instance has a ShellClient, we can connect to it.
+  instance->ConnectToClient(std::move(params));
 }
 
 bool Shell::LoadWithLoader(const Identity& target,