From df052b7fede354f92354bd54bc034b0fbc0d42e1 Mon Sep 17 00:00:00 2001 From: ben Date: Wed, 23 Mar 2016 09:13:27 -0700 Subject: 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} --- mojo/mojo_shell.gyp | 2 + mojo/services/catalog/BUILD.gn | 2 + mojo/services/catalog/catalog.cc | 148 +++++++++--------------------- mojo/services/catalog/catalog.h | 36 +++----- mojo/services/catalog/entry.cc | 19 ++++ mojo/services/catalog/entry.h | 4 + mojo/services/catalog/reader.cc | 79 ++++++++++++++++ mojo/services/catalog/reader.h | 54 +++++++++++ mojo/services/catalog/store.cc | 2 + mojo/services/catalog/store.h | 2 + mojo/shell/public/cpp/capabilities.h | 2 + mojo/shell/public/cpp/lib/capabilities.cc | 10 ++ 12 files changed, 230 insertions(+), 130 deletions(-) create mode 100644 mojo/services/catalog/reader.cc create mode 100644 mojo/services/catalog/reader.h (limited to 'mojo') 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 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) - : 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::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 Catalog::DeserializeApplication( - const base::DictionaryValue* dictionary) { - scoped_ptr entry = Entry::Deserialize(*dictionary); - if (!entry) - return entry; +// static +void Catalog::OnReadEntry(base::WeakPtr catalog, + const std::string& name, + const ResolveMojoNameCallback& callback, + scoped_ptr 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) { + // 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 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, - const std::string& name, - const ResolveMojoNameCallback& callback, - scoped_ptr 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 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, + const std::string& name, + const ResolveMojoNameCallback& callback, + scoped_ptr entry); + void OnReadEntryImpl(const std::string& name, + const ResolveMojoNameCallback& callback, + scoped_ptr entry); + // Construct a catalog entry from |dictionary|. scoped_ptr 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, - const std::string& name, - const ResolveMojoNameCallback& callback, - scoped_ptr manifest); - - // Called once the manifest is read and |this| hasn't been deleted. - void OnReadManifestImpl(const std::string& name, - const ResolveMojoNameCallback& callback, - scoped_ptr manifest); - - base::TaskRunner* blocking_pool_; - GURL system_package_dir_; + scoped_ptr reader_; + base::FilePath package_path_; mojo::BindingSet resolver_bindings_; mojo::BindingSet 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::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 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 #include #include "base/memory/scoped_ptr.h" @@ -27,6 +28,7 @@ class Entry { static scoped_ptr 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& applications() { return applications_; } private: std::string name_; std::string qualifier_; std::string display_name_; mojo::CapabilitySpec capabilities_; + std::set 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 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, + const std::string& name, + const Reader::ReadManifestCallback& callback, + scoped_ptr 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; + 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 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 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 provided; std::map 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::Convert( -- cgit v1.1