Simplify resolve.

BUG=

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

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

14 files changed, 223 insertions, 324 deletions

diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp
index 5cd4a43..26144ab 100644
--- a/mojo/mojo_shell.gyp
+++ b/mojo/mojo_shell.gyp
@@ -13,8 +13,6 @@
       'services/catalog/entry.h',
       'services/catalog/factory.cc',
       'services/catalog/factory.h',
-      'services/catalog/reader.cc',
-      'services/catalog/reader.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 b94fe75..efeee6b 100644
--- a/mojo/services/catalog/BUILD.gn
+++ b/mojo/services/catalog/BUILD.gn
@@ -21,8 +21,6 @@ source_set("lib") {
     "entry.h",
     "factory.cc",
     "factory.h",
-    "reader.cc",
-    "reader.h",
     "store.cc",
     "store.h",
   ]
diff --git a/mojo/services/catalog/catalog.cc b/mojo/services/catalog/catalog.cc
index 66a651d..bb72857 100644
--- a/mojo/services/catalog/catalog.cc
+++ b/mojo/services/catalog/catalog.cc
@@ -5,26 +5,77 @@
 #include "mojo/services/catalog/catalog.h"
 
 #include "base/bind.h"
+#include "base/json/json_file_value_serializer.h"
 #include "base/strings/string_split.h"
 #include "base/task_runner_util.h"
 #include "mojo/common/url_type_converters.h"
 #include "mojo/services/catalog/entry.h"
 #include "mojo/services/catalog/store.h"
 #include "mojo/shell/public/cpp/names.h"
-#include "mojo/util/filename_util.h"
 #include "url/gurl.h"
 #include "url/url_util.h"
 
 namespace catalog {
+namespace {
+
+base::FilePath GetManifestPath(const base::FilePath& package_dir,
+                               const std::string& name) {
+  // TODO(beng): think more about how this should be done for exe targets.
+  std::string type = mojo::GetNameType(name);
+  std::string path = mojo::GetNamePath(name);
+  if (type == mojo::kNameType_Mojo)
+    return package_dir.AppendASCII(path + "/manifest.json");
+  if (type == mojo::kNameType_Exe)
+    return package_dir.AppendASCII(path + "_manifest.json");
+  return base::FilePath();
+}
+
+base::FilePath GetPackagePath(const base::FilePath& package_dir,
+                              const std::string& name) {
+  std::string type = mojo::GetNameType(name);
+  if (type == mojo::kNameType_Mojo) {
+    // It's still a mojo: URL, use the default mapping scheme.
+    const std::string host = mojo::GetNamePath(name);
+    return package_dir.AppendASCII(host + "/" + host + ".mojo");
+  }
+  if (type == mojo::kNameType_Exe) {
+#if defined OS_WIN
+    std::string extension = ".exe";
+#else
+    std::string extension;
+#endif
+    return package_dir.AppendASCII(mojo::GetNamePath(name) + extension);
+  }
+  return base::FilePath();
+}
+
+scoped_ptr<ReadManifestResult> ReadManifest(const base::FilePath& package_dir,
+                                            const std::string& name) {
+  base::FilePath manifest_path = GetManifestPath(package_dir, name);
+  JSONFileValueDeserializer deserializer(manifest_path);
+  int error = 0;
+  std::string message;
+  // TODO(beng): probably want to do more detailed error checking. This should
+  //             be done when figuring out if to unblock connection completion.
+  scoped_ptr<ReadManifestResult> result(new ReadManifestResult);
+  result->manifest_root = deserializer.Deserialize(&error, &message);
+  result->package_dir = package_dir;
+  return result;
+}
+
+}  // namespace
+
+ReadManifestResult::ReadManifestResult() {}
+ReadManifestResult::~ReadManifestResult() {}
 
 ////////////////////////////////////////////////////////////////////////////////
 // Catalog, public:
 
-Catalog::Catalog(base::TaskRunner* blocking_pool, scoped_ptr<Store> store)
+Catalog::Catalog(scoped_ptr<Store> store, base::TaskRunner* file_task_runner)
     : store_(std::move(store)),
+      file_task_runner_(file_task_runner),
       weak_factory_(this) {
-  PathService::Get(base::DIR_MODULE, &package_path_);
-  reader_.reset(new Reader(package_path_, blocking_pool));
+  PathService::Get(base::DIR_MODULE, &system_package_dir_);
   DeserializeCatalog();
 }
 
@@ -72,22 +123,22 @@ void Catalog::ResolveProtocolScheme(
 
 void Catalog::ResolveMojoName(const mojo::String& mojo_name,
                               const ResolveMojoNameCallback& callback) {
-  std::string resolved_name = mojo_name;
-  auto alias_iter = mojo_name_aliases_.find(resolved_name);
-  if (alias_iter != mojo_name_aliases_.end())
-    resolved_name = alias_iter->second.first;
-
-  std::string qualifier = mojo::GetNamePath(resolved_name);
-  auto qualifier_iter = qualifiers_.find(resolved_name);
-  if (qualifier_iter != qualifiers_.end())
-    qualifier = qualifier_iter->second;
-
-  if (IsNameInCatalog(resolved_name)) {
-    CompleteResolveMojoName(resolved_name, qualifier, callback);
+  std::string type = mojo::GetNameType(mojo_name);
+  if (type != "mojo" && type != "exe") {
+    scoped_ptr<Entry> entry(new Entry(mojo_name));
+    callback.Run(mojo::shell::mojom::ResolveResult::From(*entry));
+    return;
+  }
+
+  auto entry = catalog_.find(mojo_name);
+  if (entry != catalog_.end()) {
+    callback.Run(mojo::shell::mojom::ResolveResult::From(*entry->second));
   } else {
-    reader_->Read(resolved_name,
-                  base::Bind(&Catalog::OnReadEntry, weak_factory_.GetWeakPtr(),
-                             resolved_name, callback));
+    base::PostTaskAndReplyWithResult(
+        file_task_runner_, FROM_HERE,
+        base::Bind(&ReadManifest, system_package_dir_, mojo_name),
+        base::Bind(&Catalog::OnReadManifest, weak_factory_.GetWeakPtr(),
+                   mojo_name, callback));
   }
 }
 
@@ -101,7 +152,7 @@ void Catalog::GetEntries(mojo::Array<mojo::String> names,
   for (const std::string& name : names_vec) {
     if (catalog_.find(name) == catalog_.end())
       continue;
-    const Entry& entry = catalog_[name];
+    const Entry& entry = *catalog_[name];
     mojom::CatalogEntryPtr entry_ptr(mojom::CatalogEntry::New());
     entry_ptr->display_name = entry.display_name();
     entries[entry.name()] = std::move(entry_ptr);
@@ -112,43 +163,6 @@ void Catalog::GetEntries(mojo::Array<mojo::String> names,
 ////////////////////////////////////////////////////////////////////////////////
 // Catalog, private:
 
-void Catalog::CompleteResolveMojoName(
-    const std::string& resolved_name,
-    const std::string& qualifier,
-    const ResolveMojoNameCallback& callback) {
-  auto entry_iter = catalog_.find(resolved_name);
-  CHECK(entry_iter != catalog_.end());
-
-  GURL package_url = mojo::util::AddTrailingSlashIfNeeded(
-      mojo::util::FilePathToFileURL(package_path_));
-  GURL file_url;
-  std::string type = mojo::GetNameType(resolved_name);
-  if (type == "mojo") {
-    // It's still a mojo: URL, use the default mapping scheme.
-    const std::string host = mojo::GetNamePath(resolved_name);
-    file_url = package_url.Resolve(host + "/" + host + ".mojo");
-  } else if (type == "exe") {
-#if defined OS_WIN
-    std::string extension = ".exe";
-#else
-    std::string extension;
-#endif
-    file_url = package_url.Resolve(
-        mojo::GetNamePath(resolved_name) + extension);
-  }
-
-  mojo::shell::mojom::CapabilitySpecPtr capabilities_ptr =
-      mojo::shell::mojom::CapabilitySpec::From(
-          entry_iter->second.capabilities());
-
-  callback.Run(resolved_name, qualifier, std::move(capabilities_ptr),
-               file_url.spec());
-}
-
-bool Catalog::IsNameInCatalog(const std::string& name) const {
-  return catalog_.find(name) != catalog_.end();
-}
-
 void Catalog::DeserializeCatalog() {
   if (!store_)
     return;
@@ -161,59 +175,44 @@ void Catalog::DeserializeCatalog() {
     CHECK(v->GetAsDictionary(&dictionary));
     scoped_ptr<Entry> entry = Entry::Deserialize(*dictionary);
     if (entry)
-      catalog_[entry->name()] = *entry;
+      catalog_[entry->name()] = std::move(entry);
   }
 }
 
 void Catalog::SerializeCatalog() {
   scoped_ptr<base::ListValue> catalog(new base::ListValue);
   for (const auto& entry : catalog_)
-    catalog->Append(entry.second.Serialize());
+    catalog->Append(entry.second->Serialize());
   if (store_)
     store_->UpdateStore(std::move(catalog));
 }
 
 // static
-void Catalog::OnReadEntry(base::WeakPtr<Catalog> catalog,
-                          const std::string& name,
-                          const ResolveMojoNameCallback& callback,
-                          scoped_ptr<Entry> entry) {
-  if (!catalog) {
-    callback.Run(name, mojo::GetNamePath(name), nullptr, nullptr);
-    return;
-  }
-  catalog->OnReadEntryImpl(name, callback, std::move(entry));
-}
-
-void Catalog::OnReadEntryImpl(const std::string& name,
-                              const ResolveMojoNameCallback& callback,
-                              scoped_ptr<Entry> entry) {
-  // TODO(beng): evaluate the conditions under which entry is null.
-  if (!entry) {
-    entry.reset(new Entry);
-    entry->set_name(name);
-    entry->set_display_name(name);
-    entry->set_qualifier(mojo::GetNamePath(name));
+void Catalog::OnReadManifest(base::WeakPtr<Catalog> catalog,
+                             const std::string& name,
+                             const ResolveMojoNameCallback& callback,
+                             scoped_ptr<ReadManifestResult> result) {
+  scoped_ptr<Entry> entry(new Entry(name));
+  if (result->manifest_root) {
+    const base::DictionaryValue* dictionary = nullptr;
+    CHECK(result->manifest_root->GetAsDictionary(&dictionary));
+    entry = Entry::Deserialize(*dictionary);
   }
+  entry->set_path(GetPackagePath(result->package_dir, name));
 
-  if (catalog_.find(entry->name()) == catalog_.end()) {
-    catalog_[entry->name()] = *entry;
-
-    if (!entry->applications().empty()) {
-      for (const auto& child : entry->applications()) {
-        mojo_name_aliases_[child.name()] =
-            std::make_pair(entry->name(), child.qualifier());
-      }
-    }
-    qualifiers_[entry->name()] = entry->qualifier();
-  }
+  callback.Run(mojo::shell::mojom::ResolveResult::From(*entry));
+  if (catalog)
+    catalog->AddEntryToCatalog(std::move(entry));
+}
 
+void Catalog::AddEntryToCatalog(scoped_ptr<Entry> entry) {
+  DCHECK(entry);
+  if (catalog_.end() != catalog_.find(entry->name()))
+    return;
+  for (auto child : entry->applications())
+    AddEntryToCatalog(make_scoped_ptr(child));
+  catalog_[entry->name()] = std::move(entry);
   SerializeCatalog();
-
-  auto qualifier_iter = qualifiers_.find(name);
-  DCHECK(qualifier_iter != qualifiers_.end());
-  std::string qualifier = qualifier_iter->second;
-  CompleteResolveMojoName(name, qualifier, callback);
 }
 
 }  // namespace catalog
diff --git a/mojo/services/catalog/catalog.h b/mojo/services/catalog/catalog.h
index 8f949c3..8d59b45 100644
--- a/mojo/services/catalog/catalog.h
+++ b/mojo/services/catalog/catalog.h
@@ -13,7 +13,6 @@
 #include "mojo/services/catalog/entry.h"
 #include "mojo/services/catalog/public/interfaces/catalog.mojom.h"
 #include "mojo/services/catalog/public/interfaces/resolver.mojom.h"
-#include "mojo/services/catalog/reader.h"
 #include "mojo/services/catalog/store.h"
 #include "mojo/shell/public/cpp/interface_factory.h"
 #include "mojo/shell/public/interfaces/shell_resolver.mojom.h"
@@ -22,11 +21,18 @@ namespace catalog {
 
 class Store;
 
+struct ReadManifestResult {
+  ReadManifestResult();
+  ~ReadManifestResult();
+  scoped_ptr<base::Value> manifest_root;
+  base::FilePath package_dir;
+};
+
 class Catalog : public mojom::Resolver,
                 public mojo::shell::mojom::ShellResolver,
                 public mojom::Catalog {
  public:
-  Catalog(base::TaskRunner* blocking_pool, scoped_ptr<Store> store);
+  Catalog(scoped_ptr<Store> store, base::TaskRunner* file_task_runner);
   ~Catalog() override;
 
   void BindResolver(mojom::ResolverRequest request);
@@ -57,48 +63,38 @@ class Catalog : public mojom::Resolver,
   void GetEntries(mojo::Array<mojo::String> names,
                   const GetEntriesCallback& callback) override;
 
-  // Completes resolving a Mojo name from the Shell after the resolved name has
-  // been added to the catalog and the manifest read.
-  void CompleteResolveMojoName(const std::string& resolved_name,
-                               const std::string& qualifier,
-                               const ResolveMojoNameCallback& callback);
-
-  bool IsNameInCatalog(const std::string& name) const;
-
   // Populate/serialize the catalog from/to the supplied store.
   void DeserializeCatalog();
   void SerializeCatalog();
 
-  // Callback for Reader, receives an Entry constructed from the manifest for
-  // |name|.
-  static void OnReadEntry(base::WeakPtr<Catalog> catalog,
-                          const std::string& name,
-                          const ResolveMojoNameCallback& callback,
-                          scoped_ptr<Entry> entry);
-  void OnReadEntryImpl(const std::string& name,
-                       const ResolveMojoNameCallback& callback,
-                       scoped_ptr<Entry> entry);
+  // Receives the result of manifest parsing on |file_task_runner_|, may be
+  // received after the catalog object that issued the request is destroyed.
+  static void OnReadManifest(base::WeakPtr<Catalog> catalog,
+                             const std::string& name,
+                             const ResolveMojoNameCallback& callback,
+                             scoped_ptr<ReadManifestResult> result);
 
-  // Construct a catalog entry from |dictionary|.
-  scoped_ptr<Entry> DeserializeApplication(
-      const base::DictionaryValue* dictionary);
+  // Populate the catalog with data from |entry|, and pass it to the client
+  // via callback.
+  void AddEntryToCatalog(scoped_ptr<Entry> entry);
 
-  scoped_ptr<Reader> reader_;
-  base::FilePath package_path_;
+  // Directory that contains packages and executables visible to all users.
+  base::FilePath system_package_dir_;
 
-  mojo::BindingSet<mojom::Resolver> resolver_bindings_;
-  mojo::BindingSet<mojo::shell::mojom::ShellResolver> shell_resolver_bindings_;
-  mojo::BindingSet<mojom::Catalog> catalog_bindings_;
+  // TODO(beng): add user package dir.
 
+  // User-specific persistent storage of package manifests and other settings.
   scoped_ptr<Store> store_;
-  std::map<std::string, Entry> catalog_;
 
-  // Used when an app handles multiple names. Maps from app (as name) to name of
-  // app that is responsible for handling it. The value is a pair of the name of
-  // the handler along with a qualifier.
-  MojoNameAliasMap mojo_name_aliases_;
+  // Task runner for performing file operations.
+  base::TaskRunner* file_task_runner_;
+
+  mojo::BindingSet<mojom::Resolver> resolver_bindings_;
+  mojo::BindingSet<mojo::shell::mojom::ShellResolver> shell_resolver_bindings_;
+  mojo::BindingSet<mojom::Catalog> catalog_bindings_;
 
-  std::map<std::string, std::string> qualifiers_;
+  // Mojo name -> Entry storage, constructed from Store/package manifests.
+  std::map<std::string, scoped_ptr<Entry>> catalog_;
 
   base::WeakPtrFactory<Catalog> weak_factory_;
 
diff --git a/mojo/services/catalog/entry.cc b/mojo/services/catalog/entry.cc
index 20dec2c..ace280f 100644
--- a/mojo/services/catalog/entry.cc
+++ b/mojo/services/catalog/entry.cc
@@ -7,6 +7,8 @@
 #include "base/values.h"
 #include "mojo/services/catalog/store.h"
 #include "mojo/shell/public/cpp/names.h"
+#include "mojo/util/filename_util.h"
+#include "url/gurl.h"
 
 namespace catalog {
 namespace {
@@ -101,6 +103,10 @@ mojo::CapabilitySpec BuildCapabilitiesV1(
 }  // namespace
 
 Entry::Entry() {}
+Entry::Entry(const std::string& name)
+    : name_(name),
+      qualifier_(mojo::GetNamePath(name)),
+      display_name_(name) {}
 Entry::Entry(const Entry& other) = default;
 Entry::~Entry() {}
 
@@ -187,8 +193,11 @@ scoped_ptr<Entry> Entry::Deserialize(const base::DictionaryValue& value) {
       const base::DictionaryValue* application = nullptr;
       applications->GetDictionary(i, &application);
       scoped_ptr<Entry> child = Entry::Deserialize(*application);
-      if (child)
-        entry->applications_.insert(*child);
+      if (child) {
+        child->set_package(entry.get());
+        // Caller must assume ownership of these items.
+        entry->applications_.insert(child.release());
+      }
     }
   }
 
@@ -208,3 +217,22 @@ bool Entry::operator<(const Entry& other) const {
 }
 
 }  // catalog
+
+namespace mojo {
+
+// static
+shell::mojom::ResolveResultPtr
+    TypeConverter<shell::mojom::ResolveResultPtr, catalog::Entry>::Convert(
+        const catalog::Entry& input) {
+  shell::mojom::ResolveResultPtr result(shell::mojom::ResolveResult::New());
+  result->name = input.name();
+  const catalog::Entry& package = input.package() ? *input.package() : input;
+  result->resolved_name = package.name();
+  result->qualifier = input.qualifier();
+  result->capabilities =
+      shell::mojom::CapabilitySpec::From(input.capabilities());
+  result->package_url = mojo::util::FilePathToFileURL(package.path()).spec();
+  return result;
+}
+
+}  // namespace mojo
diff --git a/mojo/services/catalog/entry.h b/mojo/services/catalog/entry.h
index 4a55808..05f616d 100644
--- a/mojo/services/catalog/entry.h
+++ b/mojo/services/catalog/entry.h
@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "mojo/shell/public/cpp/capabilities.h"
 
@@ -21,10 +22,15 @@ namespace catalog {
 class Entry {
  public:
   Entry();
+  explicit Entry(const std::string& name);
   explicit Entry(const Entry& other);
   ~Entry();
 
   scoped_ptr<base::DictionaryValue> Serialize() const;
+
+  // If the constructed Entry is a package that provides other Entrys, the
+  // caller must assume ownership of the tree of Entrys by enumerating
+  // applications().
   static scoped_ptr<Entry> Deserialize(const base::DictionaryValue& value);
 
   bool operator==(const Entry& other) const;
@@ -32,6 +38,8 @@ class Entry {
 
   const std::string& name() const { return name_; }
   void set_name(const std::string& name) { name_ = name; }
+  const base::FilePath& path() const { return path_; }
+  void set_path(const base::FilePath& path) { path_ = path; }
   const std::string& qualifier() const { return qualifier_; }
   void set_qualifier(const std::string& qualifier) { qualifier_ = qualifier; }
   const std::string& display_name() const { return display_name_; }
@@ -42,16 +50,27 @@ class Entry {
   void set_capabilities(const mojo::CapabilitySpec& capabilities) {
     capabilities_ = capabilities;
   }
-  const std::set<Entry>& applications() { return applications_; }
+  const Entry* package() const { return package_; }
+  void set_package(Entry* package) { package_ = package; }
+  const std::set<Entry*>& applications() { return applications_; }
 
  private:
   std::string name_;
+  base::FilePath path_;
   std::string qualifier_;
   std::string display_name_;
   mojo::CapabilitySpec capabilities_;
-  std::set<Entry> applications_;
+  Entry* package_ = nullptr;
+  std::set<Entry*> applications_;
 };
 
 }  // namespace catalog
 
+namespace mojo {
+template <>
+struct TypeConverter<shell::mojom::ResolveResultPtr, catalog::Entry> {
+  static shell::mojom::ResolveResultPtr Convert(const catalog::Entry& input);
+};
+}
+
 #endif  // MOJO_SERVICES_CATALOG_ENTRY_H_
diff --git a/mojo/services/catalog/factory.cc b/mojo/services/catalog/factory.cc
index 7087af7..2b62299 100644
--- a/mojo/services/catalog/factory.cc
+++ b/mojo/services/catalog/factory.cc
@@ -58,7 +58,7 @@ Catalog* Factory::GetCatalogForUserId(const std::string& user_id) {
     return it->second.get();
 
   // TODO(beng): There needs to be a way to load the store from different users.
-  Catalog* instance = new Catalog(file_task_runner_, std::move(store_));
+  Catalog* instance = new Catalog(std::move(store_), file_task_runner_);
   catalogs_[user_id] = make_scoped_ptr(instance);
   return instance;
 }
diff --git a/mojo/services/catalog/reader.cc b/mojo/services/catalog/reader.cc
deleted file mode 100644
index e26b778..0000000
--- a/mojo/services/catalog/reader.cc
+++ /dev/null
@@ -1,79 +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/reader.h"
-
-#include "base/json/json_file_value_serializer.h"
-#include "base/location.h"
-#include "base/task_runner_util.h"
-#include "mojo/services/catalog/entry.h"
-#include "mojo/shell/public/cpp/names.h"
-
-namespace catalog {
-namespace {
-
-scoped_ptr<base::Value> ReadManifest(const base::FilePath& manifest_path) {
-  JSONFileValueDeserializer deserializer(manifest_path);
-  int error = 0;
-  std::string message;
-  // TODO(beng): probably want to do more detailed error checking. This should
-  //             be done when figuring out if to unblock connection completion.
-  return deserializer.Deserialize(&error, &message);
-}
-
-void OnReadManifest(base::WeakPtr<Reader> reader,
-                    const std::string& name,
-                    const Reader::ReadManifestCallback& callback,
-                    scoped_ptr<base::Value> manifest) {
-  if (!reader) {
-    // The Reader was destroyed, we're likely in shutdown. Run the callback so
-    // we don't trigger a DCHECK.
-    callback.Run(nullptr);
-    return;
-  }
-  scoped_ptr<Entry> entry;
-  if (manifest) {
-    const base::DictionaryValue* dictionary = nullptr;
-    CHECK(manifest->GetAsDictionary(&dictionary));
-    entry = Entry::Deserialize(*dictionary);
-  }
-  callback.Run(std::move(entry));
-}
-
-}  // namespace
-
-Reader::Reader(const base::FilePath& package_path,
-               base::TaskRunner* file_task_runner)
-    : package_path_(package_path),
-      file_task_runner_(file_task_runner),
-      weak_factory_(this) {}
-Reader::~Reader() {}
-
-void Reader::Read(const std::string& name,
-                  const ReadManifestCallback& callback) {
-  base::FilePath manifest_path = GetManifestPath(name);
-  if (manifest_path.empty()) {
-    callback.Run(nullptr);
-    return;
-  }
-
-  std::string type = mojo::GetNameType(name);
-  CHECK(type == "mojo" || type == "exe");
-  base::PostTaskAndReplyWithResult(
-      file_task_runner_, FROM_HERE, base::Bind(&ReadManifest, manifest_path),
-      base::Bind(&OnReadManifest, weak_factory_.GetWeakPtr(), name, callback));
-}
-
-base::FilePath Reader::GetManifestPath(const std::string& name) const {
-  // TODO(beng): think more about how this should be done for exe targets.
-  std::string type = mojo::GetNameType(name);
-  std::string path = mojo::GetNamePath(name);
-  if (type == "mojo")
-    return package_path_.AppendASCII(path + "/manifest.json");
-  else if (type == "exe")
-    return package_path_.AppendASCII(path + "_manifest.json");
-  return base::FilePath();
-}
-
-}  // namespace catalog
diff --git a/mojo/services/catalog/reader.h b/mojo/services/catalog/reader.h
deleted file mode 100644
index d2d205f..0000000
--- a/mojo/services/catalog/reader.h
+++ /dev/null
@@ -1,54 +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_READER_H_
-#define MOJO_SERVICES_CATALOG_READER_H_
-
-#include "base/callback.h"
-#include "base/files/file_path.h"
-#include "base/macros.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/values.h"
-
-namespace base {
-class TaskRunner;
-}
-
-namespace catalog {
-
-class Entry;
-
-// Encapsulates manifest reading.
-class Reader {
- public:
-  using ReadManifestCallback =
-      base::Callback<void(scoped_ptr<Entry> entry)>;
-
-  // Loads manifests relative to |package_path|. Performs file operations on
-  // |file_task_runner|.
-  Reader(const base::FilePath& package_path,
-         base::TaskRunner* file_task_runner);
-  ~Reader();
-
-  // Attempts to load a manifest for |name|, and returns an Entry built from the
-  // metadata it contains via |callback|.
-  void Read(const std::string& name,
-            const ReadManifestCallback& callback);
-
- private:
-  // Construct a manifest path for the application named |name| within
-  // |package_path_|.
-  base::FilePath GetManifestPath(const std::string& name) const;
-
-  base::FilePath package_path_;
-  base::TaskRunner* file_task_runner_;
-  base::WeakPtrFactory<Reader> weak_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(Reader);
-};
-
-}  // namespace catalog
-
-#endif  // MOJO_SERVICES_CATALOG_READER_H_
diff --git a/mojo/shell/public/cpp/lib/names.cc b/mojo/shell/public/cpp/lib/names.cc
index e669229..971e8aa 100644
--- a/mojo/shell/public/cpp/lib/names.cc
+++ b/mojo/shell/public/cpp/lib/names.cc
@@ -9,6 +9,9 @@
 
 namespace mojo {
 
+const char kNameType_Mojo[] = "mojo";
+const char kNameType_Exe[] = "exe";
+
 bool IsValidName(const std::string& name) {
   std::vector<std::string> parts =
       base::SplitString(name, ":", base::KEEP_WHITESPACE,
diff --git a/mojo/shell/public/cpp/names.h b/mojo/shell/public/cpp/names.h
index 9fbabf2..86e8999 100644
--- a/mojo/shell/public/cpp/names.h
+++ b/mojo/shell/public/cpp/names.h
@@ -9,6 +9,9 @@
 
 namespace mojo {
 
+extern const char kNameType_Mojo[];
+extern const char kNameType_Exe[];
+
 // Mojo services and applications are identified by structured "names", of the
 // form:
 //
diff --git a/mojo/shell/public/interfaces/shell_resolver.mojom b/mojo/shell/public/interfaces/shell_resolver.mojom
index 090bf83..0c4e841 100644
--- a/mojo/shell/public/interfaces/shell_resolver.mojom
+++ b/mojo/shell/public/interfaces/shell_resolver.mojom
@@ -6,34 +6,33 @@ module mojo.shell.mojom;
 
 import "mojo/shell/public/interfaces/capabilities.mojom";
 
+// The result of a Resolve operation via ShellResolver.
+struct ResolveResult {
+  // The mojo: name that was requested to be resolved.
+  string name;
+
+  // The mojo: name of the physical package supplying the requested name. This
+  // could be the same name that was passed, or the name of a package that
+  // contains it.
+  string resolved_name;
+
+  // An additional piece of metadata that identifies what instance |name| should
+  // be run in. It's possible that |name| may provide several services that
+  // should be run as different instances.
+  string qualifier;
+
+  // The set of capabilities provided and required by |name|.
+  CapabilitySpec capabilities;
+
+  // A file URL to the package specified by |name|.
+  // TODO(beng): What if resolved_mojo_name needs to be re-resolved?
+  string package_url;
+};
+
 // Implemented exclusively for the Mojo Shell's use in resolving mojo: names
 // and reading static manifest information.
 interface ShellResolver {
-  // Resolves |mojo_name| to the following metadata:
-  //
-  // resolved_mojo_name
-  //  another mojo: name of an application implementing mojo::ShellClientFactory
-  //  that can handle connections to |mojo_name|.
-  //
-  // qualifier
-  //  an additional piece of metadata that identifies what instance
-  //  |resolved_mojo_name| should be run in. It's possible that
-  //  |resolved_mojo_name| may provide several services that should be run as
-  //  different instances.
-  //
-  // mojo_file_url
-  //  a file URL to the application specified in |resolved_mojo_name|
-  //  TODO(beng): what if |resolved_mojo_name| needs to be re-resolved??
-  // 
-  // filter
-  //  the base CapabilityFilter within which an instance of |resolved_mojo_name|
-  //  must be run for |mojo_name|.
-  //
-  // If |mojo_name| can't be resolved (i.e. not a mojo: or exe: scheme), then
-  // the callback will be run with null |mojo_file_url|, and |filter|.
-  ResolveMojoName(string mojo_name) =>
-      (string resolved_mojo_name,
-       string qualifier,
-       CapabilitySpec? capability_spec,
-       string? mojo_file_url);
+  // Resolves |mojo_name| and returns a ResolveResult containing metadata from
+  // the catalog that the Shell uses to run an instance of it.
+  ResolveMojoName(string mojo_name) => (ResolveResult result);
 };
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index 63ee453..ed3f6e4 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -41,10 +41,7 @@ const char kCapabilityClass_ClientProcess[] = "client_process";
 const char kCapabilityClass_InstanceName[] = "instance_name";
 const char kCapabilityClass_AllUsers[] = "all_users";
 
-void EmptyResolverCallback(const String& resolved_name,
-                           const String& resolved_instance,
-                           mojom::CapabilitySpecPtr capabilities,
-                           const String& file_url) {}
+void EmptyResolverCallback(mojom::ResolveResultPtr result) {}
 
 }
 
@@ -683,14 +680,11 @@ void Shell::OnShellClientFactoryLost(const Identity& which) {
 void Shell::OnGotResolvedName(mojom::ShellResolverPtr resolver,
                               scoped_ptr<ConnectParams> params,
                               mojom::ShellClientPtr client,
-                              const String& resolved_name,
-                              const String& resolved_instance,
-                              mojom::CapabilitySpecPtr capabilities_ptr,
-                              const String& file_url) {
+                              mojom::ResolveResultPtr result) {
   std::string instance_name = params->target().instance();
   if (instance_name == GetNamePath(params->target().name()) &&
-      resolved_instance != GetNamePath(resolved_name)) {
-    instance_name = resolved_instance;
+      result->qualifier != GetNamePath(result->resolved_name)) {
+    instance_name = result->qualifier;
   }
   Identity target(params->target().name(), params->target().user_id(),
                   instance_name);
@@ -706,8 +700,8 @@ void Shell::OnGotResolvedName(mojom::ShellResolverPtr resolver,
   // |capabilities_ptr| can be null when there is no manifest, e.g. for URL
   // types not resolvable by the resolver.
   CapabilitySpec capabilities = GetPermissiveCapabilities();
-  if (!capabilities_ptr.is_null())
-    capabilities = capabilities_ptr.To<CapabilitySpec>();
+  if (!result->capabilities.is_null())
+    capabilities = result->capabilities.To<CapabilitySpec>();
 
   // Clients that request "all_users" class from the shell are allowed to
   // field connection requests from any user. They also run with a synthetic
@@ -745,15 +739,17 @@ void Shell::OnGotResolvedName(mojom::ShellResolverPtr resolver,
     if (LoadWithLoader(target, &request)) {
       instance->StartWithClient(std::move(client));
     } else {
-      CHECK(!file_url.is_null() && !capabilities_ptr.is_null());
+      CHECK(!result->package_url.is_null() && !result->capabilities.is_null());
 
-      if (target.name() != resolved_name) {
+      if (target.name() != result->resolved_name) {
         instance->StartWithClient(std::move(client));
-        CreateShellClientWithFactory(
-            source, Identity(resolved_name, target.user_id(), instance_name),
-            target.name(), std::move(request));
+        Identity factory(result->resolved_name, target.user_id(),
+                         instance_name);
+        CreateShellClientWithFactory(source, factory, target.name(),
+                                     std::move(request));
       } else {
-        instance->StartWithFilePath(util::UrlToFilePath(file_url.To<GURL>()));
+        instance->StartWithFilePath(
+            util::UrlToFilePath(result->package_url.To<GURL>()));
       }
     }
   }
diff --git a/mojo/shell/shell.h b/mojo/shell/shell.h
index c060518..60c5dadb 100644
--- a/mojo/shell/shell.h
+++ b/mojo/shell/shell.h
@@ -142,18 +142,11 @@ class Shell : public ShellClient {
   // |params| are the params passed to Connect().
   // |client| if provided is a ShellClientPtr which should be used to manage the
   // new application instance. This may be null.
-  // |resolved_name| is the mojo: name identifying the physical package
-  // application.
-  // |file_url| is the resolved file:// URL of the physical package.
-  // |capabilities| is the CapabilitySpecPtr the requested application should be
-  // run with, from its manifest.
+  // |result| contains the result of the resolve operation.
   void OnGotResolvedName(mojom::ShellResolverPtr resolver,
                          scoped_ptr<ConnectParams> params,
                          mojom::ShellClientPtr client,
-                         const String& resolved_name,
-                         const String& resolved_instance,
-                         mojom::CapabilitySpecPtr capabilities,
-                         const String& file_url);
+                         mojom::ResolveResultPtr result);
 
   // Tries to load |target| with an Loader. Returns true if one was registered
   // and it was loaded, in which case |request| is taken.