Remove loader for catalog & some other cleanup:

- instantiate the catalog shell client directly in the shell
- remove some obsolete comments
- move userid inheritance handling into the instance

R=sky@chromium.org

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

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

11 files changed, 27 insertions, 114 deletions

diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp
index 213857b..5474664 100644
--- a/mojo/mojo_shell.gyp
+++ b/mojo/mojo_shell.gyp
@@ -13,8 +13,6 @@
       'services/catalog/catalog.h',
       'services/catalog/entry.cc',
       'services/catalog/entry.h',
-      'services/catalog/loader.cc',
-      'services/catalog/loader.h',
       'services/catalog/store.cc',
       'services/catalog/store.h',
       'shell/loader.h',
diff --git a/mojo/services/catalog/BUILD.gn b/mojo/services/catalog/BUILD.gn
index 633f6d1..f1eb63d 100644
--- a/mojo/services/catalog/BUILD.gn
+++ b/mojo/services/catalog/BUILD.gn
@@ -21,8 +21,6 @@ source_set("lib") {
     "catalog.h",
     "entry.cc",
     "entry.h",
-    "loader.cc",
-    "loader.h",
     "store.cc",
     "store.h",
   ]
diff --git a/mojo/services/catalog/loader.cc b/mojo/services/catalog/loader.cc
deleted file mode 100644
index 7f1fccb..0000000
--- a/mojo/services/catalog/loader.cc
+++ /dev/null
@@ -1,25 +0,0 @@
-// Copyright 2016 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.
-
-#include "mojo/services/catalog/loader.h"
-
-#include "mojo/services/catalog/catalog.h"
-#include "mojo/shell/public/cpp/shell_client.h"
-#include "mojo/shell/public/cpp/shell_connection.h"
-
-namespace catalog {
-
-Loader::Loader(base::TaskRunner* blocking_pool, scoped_ptr<Store> store)
-    : blocking_pool_(blocking_pool),
-      store_(std::move(store)) {}
-Loader::~Loader() {}
-
-void Loader::Load(const std::string& name,
-                  mojo::shell::mojom::ShellClientRequest request) {
-  client_.reset(new catalog::Catalog(blocking_pool_, std::move(store_)));
-  connection_.reset(new mojo::ShellConnection(client_.get(),
-                                              std::move(request)));
-}
-
-}  // namespace catalog
diff --git a/mojo/services/catalog/loader.h b/mojo/services/catalog/loader.h
deleted file mode 100644
index ad02411..0000000
--- a/mojo/services/catalog/loader.h
+++ /dev/null
@@ -1,45 +0,0 @@
-// Copyright 2016 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 MOJO_SERVICES_CATALOG_LOADER_H_
-#define MOJO_SERVICES_CATALOG_LOADER_H_
-
-#include "base/memory/scoped_ptr.h"
-#include "mojo/services/catalog/store.h"
-#include "mojo/shell/loader.h"
-
-namespace base {
-class BlockingPool;
-}
-
-namespace mojo {
-class ShellClient;
-class ShellConnection;
-}
-
-namespace catalog {
-
-class Store;
-
-class Loader : public mojo::shell::Loader {
- public:
-  Loader(base::TaskRunner* blocking_pool, scoped_ptr<Store> store);
-  ~Loader() override;
-
-  // mojo::shell::Loader:
-  void Load(const std::string& name,
-            mojo::shell::mojom::ShellClientRequest request) override;
-
- private:
-  base::TaskRunner* blocking_pool_;
-  scoped_ptr<Store> store_;
-  scoped_ptr<mojo::ShellClient> client_;
-  scoped_ptr<mojo::ShellConnection> connection_;
-
-  DISALLOW_COPY_AND_ASSIGN(Loader);
-};
-
-}  // namespace catalog
-
-#endif  // MOJO_SERVICES_CATALOG_LOADER_H_
diff --git a/mojo/shell/public/cpp/lib/connector_impl.cc b/mojo/shell/public/cpp/lib/connector_impl.cc
index 98a7ae5..7ff56d4 100644
--- a/mojo/shell/public/cpp/lib/connector_impl.cc
+++ b/mojo/shell/public/cpp/lib/connector_impl.cc
@@ -46,9 +46,6 @@ scoped_ptr<Connection> ConnectorImpl::Connect(ConnectParams* params) {
   DCHECK(params);
   // We allow all interfaces on outgoing connections since we are presumably in
   // a position to know who we're talking to.
-  // TODO(beng): We should filter outgoing interfaces also. The shell must pass
-  //             the manifest CapabilityFilter to the ShellConnection via
-  //             Initialize(), it can be used here.
   CapabilityRequest request;
   request.interfaces.insert("*");
   shell::mojom::InterfaceProviderPtr local_interfaces;
diff --git a/mojo/shell/runner/host/child_process_base.cc b/mojo/shell/runner/host/child_process_base.cc
index d4d6870..538cf7d 100644
--- a/mojo/shell/runner/host/child_process_base.cc
+++ b/mojo/shell/runner/host/child_process_base.cc
@@ -106,7 +106,6 @@ class AppContext : public edk::ProcessDelegate {
     io_runner_ = io_thread_.task_runner().get();
     CHECK(io_runner_.get());
 
-    // TODO(vtl): This should be SLAVE, not NONE.
     // This must be created before controller_thread_ since MessagePumpMojo will
     // create a message pipe which requires this code to be run first.
     edk::InitIPCSupport(this, io_runner_);
diff --git a/mojo/shell/runner/host/native_application_support.h b/mojo/shell/runner/host/native_application_support.h
index 2371f65..d7ce8a2 100644
--- a/mojo/shell/runner/host/native_application_support.h
+++ b/mojo/shell/runner/host/native_application_support.h
@@ -32,7 +32,6 @@ base::NativeLibrary LoadNativeApplication(const base::FilePath& app_path);
 // should be called on the same thread as |LoadNativeApplication()|. Returns
 // true if |MojoMain()| was called (even if it returns an error), and false
 // otherwise.
-// TODO(vtl): Maybe this should also have a |MojoResult| as an out parameter?
 bool RunNativeApplication(base::NativeLibrary app_library,
                           InterfaceRequest<mojom::ShellClient> request);
 
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index e5c46b3..ceb25e6 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -21,9 +21,9 @@
 #include "mojo/common/url_type_converters.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
-#include "mojo/services/catalog/loader.h"
+#include "mojo/services/catalog/catalog.h"
 #include "mojo/shell/connect_util.h"
-#include "mojo/shell/public/cpp/connect.h"
+#include "mojo/shell/public/cpp/connector.h"
 #include "mojo/shell/public/cpp/names.h"
 #include "mojo/shell/public/cpp/shell_connection.h"
 #include "mojo/shell/public/interfaces/connector.mojom.h"
@@ -35,6 +35,7 @@ namespace mojo {
 namespace shell {
 namespace {
 const char kCatalogName[] = "mojo:catalog";
+const char kShellName[] = "mojo:shell";
 
 void EmptyResolverCallback(const String& resolved_name,
                            const String& resolved_instance,
@@ -44,7 +45,7 @@ void EmptyResolverCallback(const String& resolved_name,
 }
 
 Identity CreateShellIdentity() {
-  return Identity("mojo:shell", mojom::kRootUserID);
+  return Identity(kShellName, mojom::kRootUserID);
 }
 
 CapabilitySpec GetPermissiveCapabilities() {
@@ -109,7 +110,7 @@ class Shell::Instance : public mojom::Connector,
       shell_client_(std::move(shell_client)),
       pid_receiver_binding_(this),
       weak_factory_(this) {
-    if (identity_.name() == "mojo:shell" ||
+    if (identity_.name() == kShellName ||
         shell_->GetLoaderForName(identity_.name())) {
       pid_ = base::Process::Current().Pid();
     }
@@ -207,6 +208,9 @@ class Shell::Instance : public mojom::Connector,
     if (!ValidateCapabilities(target, callback))
       return;
 
+    if (target.user_id() == mojom::kInheritUserID)
+      target.set_user_id(identity_.user_id());
+
     scoped_ptr<ConnectParams> params(new ConnectParams);
     params->set_source(identity_);
     params->set_target(target);
@@ -352,6 +356,7 @@ Shell::Shell(scoped_ptr<NativeRunnerFactory> native_runner_factory,
   mojom::ShellClientRequest request;
   CreateInstance(CreateShellIdentity(), GetPermissiveCapabilities(), &request);
   shell_connection_.reset(new ShellConnection(this, std::move(request)));
+  shell_connection_->WaitForInitialize();
 
   InitCatalog(std::move(catalog_store));
 }
@@ -373,17 +378,8 @@ void Shell::Connect(scoped_ptr<ConnectParams> params) {
                        TRACE_EVENT_SCOPE_THREAD, "original_name",
                        params->target().name());
   DCHECK(IsValidName(params->target().name()));
-
-  if (params->target().user_id() == mojom::kInheritUserID) {
-    Instance* source = GetExistingInstance(params->source());
-    Identity target = params->target();
-    // TODO(beng): we should CHECK source.
-    target.set_user_id(source ? source->identity().user_id()
-                              : mojom::kRootUserID);
-    params->set_target(target);
-  }
-
-  CHECK(params->target().user_id() != mojom::kInheritUserID);
+  DCHECK(base::IsValidGUID(params->target().user_id()));
+  DCHECK_NE(mojom::kInheritUserID, params->target().user_id());
 
   // Connect to an existing matching instance, if possible.
   if (ConnectToExistingInstance(&params))
@@ -442,26 +438,23 @@ bool Shell::AcceptConnection(Connection* connection) {
 // Shell, private:
 
 void Shell::InitCatalog(scoped_ptr<catalog::Store> store) {
-  scoped_ptr<Loader> loader(
-      new catalog::Loader(file_task_runner_, std::move(store)));
-  Loader* loader_raw = loader.get();
-  std::string name = kCatalogName;
-  SetLoaderForName(std::move(loader), name);
-
   mojom::ShellClientRequest request;
-  // TODO(beng): Does the catalog actually have to be run with a permissive
-  //             filter?
-  Identity identity(name, mojom::kRootUserID);
-  CreateInstance(identity, GetPermissiveCapabilities(), &request);
-  loader_raw->Load(name, std::move(request));
+  Identity identity(kCatalogName, mojom::kRootUserID);
+  CreateInstance(identity, CapabilitySpec(), &request);
 
-  ConnectToInterface(this, CreateShellIdentity(), name, &shell_resolver_);
+  catalog_shell_client_.reset(
+      new catalog::Catalog(file_task_runner_, std::move(store)));
+  catalog_connection_.reset(
+      new ShellConnection(catalog_shell_client_.get(), std::move(request)));
+  shell_connection_->connector()->ConnectToInterface(
+      kCatalogName, &shell_resolver_);
 
   // Seed the catalog with manifest info for the shell & catalog.
   if (file_task_runner_) {
-    shell_resolver_->ResolveMojoName(name, base::Bind(&EmptyResolverCallback));
-    shell_resolver_->ResolveMojoName("mojo:shell",
-                                     base::Bind(&EmptyResolverCallback));
+    shell_resolver_->ResolveMojoName(
+        kCatalogName, base::Bind(&EmptyResolverCallback));
+    shell_resolver_->ResolveMojoName(
+        kShellName, base::Bind(&EmptyResolverCallback));
   }
 }
 
@@ -560,7 +553,6 @@ mojom::ShellClientFactory* Shell::GetShellClientFactory(
     return it->second.get();
 
   mojom::ShellClientFactoryPtr factory;
-  // TODO(beng): we should forward the original source identity!
   ConnectToInterface(this, source_identity, shell_client_factory_identity,
                      &factory);
   mojom::ShellClientFactory* factory_interface = factory.get();
diff --git a/mojo/shell/shell.h b/mojo/shell/shell.h
index 2d66593..e8f18fb 100644
--- a/mojo/shell/shell.h
+++ b/mojo/shell/shell.h
@@ -34,6 +34,7 @@ class SequencedWorkerPool;
 }
 
 namespace mojo {
+class ShellClient;
 class ShellConnection;
 namespace shell {
 
@@ -191,6 +192,8 @@ class Shell : public ShellClient {
   scoped_ptr<NativeRunnerFactory> native_runner_factory_;
   std::vector<scoped_ptr<NativeRunner>> native_runners_;
   scoped_ptr<ShellConnection> shell_connection_;
+  scoped_ptr<ShellConnection> catalog_connection_;
+  scoped_ptr<ShellClient> catalog_shell_client_;
   base::WeakPtrFactory<Shell> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Shell);
diff --git a/mojo/shell/standalone/context.cc b/mojo/shell/standalone/context.cc
index 23dc96b..4e847fb 100644
--- a/mojo/shell/standalone/context.cc
+++ b/mojo/shell/standalone/context.cc
@@ -134,7 +134,6 @@ void Context::Init(scoped_ptr<InitParams> init_params) {
   blocking_pool_ =
       new base::SequencedWorkerPool(kMaxBlockingPoolThreads, "blocking_pool");
 
-  // TODO(vtl): This should be MASTER, not NONE.
   edk::InitIPCSupport(this, io_thread_->task_runner().get());
 
   scoped_ptr<NativeRunnerFactory> runner_factory;
@@ -164,7 +163,7 @@ void Context::Init(scoped_ptr<InitParams> init_params) {
 
   scoped_ptr<ConnectParams> params(new ConnectParams);
   params->set_source(CreateShellIdentity());
-  params->set_target(Identity("mojo:tracing", mojom::kInheritUserID));
+  params->set_target(Identity("mojo:tracing", mojom::kRootUserID));
   params->set_remote_interfaces(GetProxy(&tracing_remote_interfaces));
   params->set_local_interfaces(std::move(tracing_local_interfaces));
   shell_->Connect(std::move(params));
diff --git a/mojo/shell/tests/connect/connect_unittest.cc b/mojo/shell/tests/connect/connect_unittest.cc
index 0c65d9d..df6c6fa 100644
--- a/mojo/shell/tests/connect/connect_unittest.cc
+++ b/mojo/shell/tests/connect/connect_unittest.cc
@@ -287,8 +287,6 @@ TEST_F(ConnectTest, CapabilityClasses) {
 }
 
 // Tests that we can expose an interface to targets on outbound connections.
-// TODO(beng): Currently all interfaces on outbound connections are exposed.
-//             See ConnectorImpl::Connect().
 TEST_F(ConnectTest, LocalInterface) {
   // Connect to a standalone application.
   {