Moves manifest parsing to a new class, Reader.

. Consolidate value analysis in Entry
. Catalog now just manages the various data structures that allow it to implement various flavors of Resolve().

BUG=

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

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

12 files changed, 230 insertions, 130 deletions

diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp
index 26144ab..5cd4a43 100644
--- a/mojo/mojo_shell.gyp
+++ b/mojo/mojo_shell.gyp
@@ -13,6 +13,8 @@
       '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 efeee6b..b94fe75 100644
--- a/mojo/services/catalog/BUILD.gn
+++ b/mojo/services/catalog/BUILD.gn
@@ -21,6 +21,8 @@ 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 0bbb9d4..66a651d 100644
--- a/mojo/services/catalog/catalog.cc
+++ b/mojo/services/catalog/catalog.cc
@@ -5,7 +5,6 @@
 #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"
@@ -13,37 +12,19 @@
 #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 {
-
-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);
-}
-
-}  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // Catalog, public:
 
 Catalog::Catalog(base::TaskRunner* blocking_pool, scoped_ptr<Store> store)
-    : blocking_pool_(blocking_pool),
-      store_(std::move(store)),
+    : store_(std::move(store)),
       weak_factory_(this) {
-  base::FilePath shell_dir;
-  PathService::Get(base::DIR_MODULE, &shell_dir);
-
-  system_package_dir_ =
-      mojo::util::FilePathToFileURL(shell_dir).Resolve(std::string());
-  system_package_dir_ =
-      mojo::util::AddTrailingSlashIfNeeded(system_package_dir_);
-
+  PathService::Get(base::DIR_MODULE, &package_path_);
+  reader_.reset(new Reader(package_path_, blocking_pool));
   DeserializeCatalog();
 }
 
@@ -101,10 +82,13 @@ void Catalog::ResolveMojoName(const mojo::String& mojo_name,
   if (qualifier_iter != qualifiers_.end())
     qualifier = qualifier_iter->second;
 
-  if (IsNameInCatalog(resolved_name))
+  if (IsNameInCatalog(resolved_name)) {
     CompleteResolveMojoName(resolved_name, qualifier, callback);
-  else
-    AddNameToCatalog(resolved_name, callback);
+  } else {
+    reader_->Read(resolved_name,
+                  base::Bind(&Catalog::OnReadEntry, weak_factory_.GetWeakPtr(),
+                             resolved_name, callback));
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -135,19 +119,21 @@ void Catalog::CompleteResolveMojoName(
   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 = system_package_dir_.Resolve(host + "/" + host + ".mojo");
+    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 = system_package_dir_.Resolve(
+    file_url = package_url.Resolve(
         mojo::GetNamePath(resolved_name) + extension);
   }
 
@@ -163,26 +149,6 @@ bool Catalog::IsNameInCatalog(const std::string& name) const {
   return catalog_.find(name) != catalog_.end();
 }
 
-void Catalog::AddNameToCatalog(const std::string& name,
-                               const ResolveMojoNameCallback& callback) {
-  GURL manifest_url = GetManifestURL(name);
-  if (manifest_url.is_empty()) {
-    // The name is of some form that can't be resolved to a manifest (e.g. some
-    // scheme used for tests). Just pass it back to the caller so it can be
-    // loaded with a custom loader.
-    callback.Run(name, mojo::GetNamePath(name), nullptr, nullptr);
-    return;
-  }
-
-  std::string type = mojo::GetNameType(name);
-  CHECK(type == "mojo" || type == "exe");
-  base::FilePath manifest_path = mojo::util::UrlToFilePath(manifest_url);
-  base::PostTaskAndReplyWithResult(
-      blocking_pool_, FROM_HERE, base::Bind(&ReadManifest, manifest_path),
-      base::Bind(&Catalog::OnReadManifest, weak_factory_.GetWeakPtr(),
-                 name, callback));
-}
-
 void Catalog::DeserializeCatalog() {
   if (!store_)
     return;
@@ -194,7 +160,7 @@ void Catalog::DeserializeCatalog() {
     const base::Value* v = *it;
     CHECK(v->GetAsDictionary(&dictionary));
     scoped_ptr<Entry> entry = Entry::Deserialize(*dictionary);
-    if (entry.get())
+    if (entry)
       catalog_[entry->name()] = *entry;
   }
 }
@@ -207,73 +173,41 @@ void Catalog::SerializeCatalog() {
     store_->UpdateStore(std::move(catalog));
 }
 
-scoped_ptr<Entry> Catalog::DeserializeApplication(
-    const base::DictionaryValue* dictionary) {
-  scoped_ptr<Entry> entry = Entry::Deserialize(*dictionary);
-  if (!entry)
-    return entry;
+// 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));
+  }
 
-  // TODO(beng): move raw dictionary analysis into Deserialize().
   if (catalog_.find(entry->name()) == catalog_.end()) {
     catalog_[entry->name()] = *entry;
 
-    if (dictionary->HasKey("applications")) {
-      const base::ListValue* applications = nullptr;
-      dictionary->GetList("applications", &applications);
-      for (size_t i = 0; i < applications->GetSize(); ++i) {
-        const base::DictionaryValue* child_value = nullptr;
-        applications->GetDictionary(i, &child_value);
-        scoped_ptr<Entry> child = DeserializeApplication(child_value);
-        if (child) {
-          mojo_name_aliases_[child->name()] =
-              std::make_pair(entry->name(), child->qualifier());
-        }
+    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();
   }
-  return entry;
-}
 
-GURL Catalog::GetManifestURL(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")
-    return system_package_dir_.Resolve(path + "/manifest.json");
-  else if (type == "exe")
-    return system_package_dir_.Resolve(path + "_manifest.json");
-  return GURL();
-}
-
-// static
-void Catalog::OnReadManifest(base::WeakPtr<Catalog> catalog,
-                             const std::string& name,
-                             const ResolveMojoNameCallback& callback,
-                             scoped_ptr<base::Value> manifest) {
-  if (!catalog) {
-    // The Catalog was destroyed, we're likely in shutdown. Run the
-    // callback so we don't trigger a DCHECK.
-    callback.Run(name, mojo::GetNamePath(name), nullptr, nullptr);
-    return;
-  }
-  catalog->OnReadManifestImpl(name, callback, std::move(manifest));
-}
-
-void Catalog::OnReadManifestImpl(const std::string& name,
-                                 const ResolveMojoNameCallback& callback,
-                                 scoped_ptr<base::Value> manifest) {
-  if (manifest) {
-    base::DictionaryValue* dictionary = nullptr;
-    CHECK(manifest->GetAsDictionary(&dictionary));
-    DeserializeApplication(dictionary);
-  } else {
-    Entry entry;
-    entry.set_name(name);
-    entry.set_display_name(name);
-    catalog_[entry.name()] = entry;
-    qualifiers_[entry.name()] = mojo::GetNamePath(name);
-  }
   SerializeCatalog();
 
   auto qualifier_iter = qualifiers_.find(name);
diff --git a/mojo/services/catalog/catalog.h b/mojo/services/catalog/catalog.h
index e4bc6a5..8f949c3 100644
--- a/mojo/services/catalog/catalog.h
+++ b/mojo/services/catalog/catalog.h
@@ -13,10 +13,10 @@
 #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"
-#include "url/gurl.h"
 
 namespace catalog {
 
@@ -65,36 +65,26 @@ class Catalog : public mojom::Resolver,
 
   bool IsNameInCatalog(const std::string& name) const;
 
-  // Called from ResolveMojoName().
-  // Attempts to load a manifest for |name|, reads it and adds its metadata to
-  // the catalog.
-  void AddNameToCatalog(const std::string& name,
-                        const ResolveMojoNameCallback& callback);
-
   // 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);
+
   // Construct a catalog entry from |dictionary|.
   scoped_ptr<Entry> DeserializeApplication(
       const base::DictionaryValue* dictionary);
 
-  GURL GetManifestURL(const std::string& name);
-
-  // Called once the manifest has been read. |pm| may be null at this point,
-  // but |callback| must be run.
-  static void OnReadManifest(base::WeakPtr<Catalog> catalog,
-                             const std::string& name,
-                             const ResolveMojoNameCallback& callback,
-                             scoped_ptr<base::Value> manifest);
-
-  // Called once the manifest is read and |this| hasn't been deleted.
-  void OnReadManifestImpl(const std::string& name,
-                          const ResolveMojoNameCallback& callback,
-                          scoped_ptr<base::Value> manifest);
-
-  base::TaskRunner* blocking_pool_;
-  GURL system_package_dir_;
+  scoped_ptr<Reader> reader_;
+  base::FilePath package_path_;
 
   mojo::BindingSet<mojom::Resolver> resolver_bindings_;
   mojo::BindingSet<mojo::shell::mojom::ShellResolver> shell_resolver_bindings_;
diff --git a/mojo/services/catalog/entry.cc b/mojo/services/catalog/entry.cc
index 528eee4..20dec2c 100644
--- a/mojo/services/catalog/entry.cc
+++ b/mojo/services/catalog/entry.cc
@@ -179,6 +179,19 @@ scoped_ptr<Entry> Entry::Deserialize(const base::DictionaryValue& value) {
     entry->set_capabilities(BuildCapabilitiesV0(*capabilities));
   else
     entry->set_capabilities(BuildCapabilitiesV1(*capabilities));
+
+  if (value.HasKey(Store::kApplicationsKey)) {
+    const base::ListValue* applications = nullptr;
+    value.GetList(Store::kApplicationsKey, &applications);
+    for (size_t i = 0; i < applications->GetSize(); ++i) {
+      const base::DictionaryValue* application = nullptr;
+      applications->GetDictionary(i, &application);
+      scoped_ptr<Entry> child = Entry::Deserialize(*application);
+      if (child)
+        entry->applications_.insert(*child);
+    }
+  }
+
   return entry;
 }
 
@@ -188,4 +201,10 @@ bool Entry::operator==(const Entry& other) const {
          other.capabilities_ == capabilities_;
 }
 
+bool Entry::operator<(const Entry& other) const {
+  return std::tie(name_, qualifier_, display_name_, capabilities_) <
+         std::tie(other.name_, other.qualifier_, other.display_name_,
+                  other.capabilities_);
+}
+
 }  // catalog
diff --git a/mojo/services/catalog/entry.h b/mojo/services/catalog/entry.h
index ac5ace5..4a55808 100644
--- a/mojo/services/catalog/entry.h
+++ b/mojo/services/catalog/entry.h
@@ -5,6 +5,7 @@
 #ifndef MOJO_SERVICES_CATALOG_ENTRY_H_
 #define MOJO_SERVICES_CATALOG_ENTRY_H_
 
+#include <set>
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
@@ -27,6 +28,7 @@ class Entry {
   static scoped_ptr<Entry> Deserialize(const base::DictionaryValue& value);
 
   bool operator==(const Entry& other) const;
+  bool operator<(const Entry& other) const;
 
   const std::string& name() const { return name_; }
   void set_name(const std::string& name) { name_ = name; }
@@ -40,12 +42,14 @@ class Entry {
   void set_capabilities(const mojo::CapabilitySpec& capabilities) {
     capabilities_ = capabilities;
   }
+  const std::set<Entry>& applications() { return applications_; }
 
  private:
   std::string name_;
   std::string qualifier_;
   std::string display_name_;
   mojo::CapabilitySpec capabilities_;
+  std::set<Entry> applications_;
 };
 
 }  // namespace catalog
diff --git a/mojo/services/catalog/reader.cc b/mojo/services/catalog/reader.cc
new file mode 100644
index 0000000..e26b778
--- /dev/null
+++ b/mojo/services/catalog/reader.cc
@@ -0,0 +1,79 @@
+// 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
new file mode 100644
index 0000000..d2d205f
--- /dev/null
+++ b/mojo/services/catalog/reader.h
@@ -0,0 +1,54 @@
+// 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/services/catalog/store.cc b/mojo/services/catalog/store.cc
index c48aa72..6c1a6d6 100644
--- a/mojo/services/catalog/store.cc
+++ b/mojo/services/catalog/store.cc
@@ -24,5 +24,7 @@ const char Store::kCapabilities_RequiredKey[] = "required";
 const char Store::kCapabilities_ClassesKey[] = "classes";
 // static
 const char Store::kCapabilities_InterfacesKey[] = "interfaces";
+// static
+const char Store::kApplicationsKey[] = "applications";
 
 }  // namespace catalog
diff --git a/mojo/services/catalog/store.h b/mojo/services/catalog/store.h
index cb763d5..1a4087f 100644
--- a/mojo/services/catalog/store.h
+++ b/mojo/services/catalog/store.h
@@ -32,6 +32,8 @@ class Store {
   static const char kCapabilities_ClassesKey[];
   // Value is a list.
   static const char kCapabilities_InterfacesKey[];
+  // Value is a list.
+  static const char kApplicationsKey[];
 
   virtual ~Store() {}
 
diff --git a/mojo/shell/public/cpp/capabilities.h b/mojo/shell/public/cpp/capabilities.h
index 39bc387..4ec4722 100644
--- a/mojo/shell/public/cpp/capabilities.h
+++ b/mojo/shell/public/cpp/capabilities.h
@@ -25,6 +25,7 @@ struct CapabilityRequest {
   CapabilityRequest();
   ~CapabilityRequest();
   bool operator==(const CapabilityRequest& other) const;
+  bool operator<(const CapabilityRequest& other) const;
   Classes classes;
   Interfaces interfaces;
 };
@@ -33,6 +34,7 @@ struct CapabilitySpec {
   CapabilitySpec();
   ~CapabilitySpec();
   bool operator==(const CapabilitySpec& other) const;
+  bool operator<(const CapabilitySpec& other) const;
   std::map<Class, Interfaces> provided;
   std::map<Name, CapabilityRequest> required;
 };
diff --git a/mojo/shell/public/cpp/lib/capabilities.cc b/mojo/shell/public/cpp/lib/capabilities.cc
index cc3fc18..add9150 100644
--- a/mojo/shell/public/cpp/lib/capabilities.cc
+++ b/mojo/shell/public/cpp/lib/capabilities.cc
@@ -13,6 +13,11 @@ bool CapabilityRequest::operator==(const CapabilityRequest& other) const {
   return other.classes == classes && other.interfaces == interfaces;
 }
 
+bool CapabilityRequest::operator<(const CapabilityRequest& other) const {
+  return std::tie(classes, interfaces) <
+      std::tie(other.classes, other.interfaces);
+}
+
 CapabilitySpec::CapabilitySpec() {}
 CapabilitySpec::~CapabilitySpec() {}
 
@@ -20,6 +25,11 @@ bool CapabilitySpec::operator==(const CapabilitySpec& other) const {
   return other.provided == provided && other.required == required;
 }
 
+bool CapabilitySpec::operator<(const CapabilitySpec& other) const {
+  return std::tie(provided, required) <
+      std::tie(other.provided, other.required);
+}
+
 // static
 shell::mojom::CapabilitySpecPtr
 TypeConverter<shell::mojom::CapabilitySpecPtr, CapabilitySpec>::Convert(