Move serialize/deserialize logic onto Entry.

I'm trying to shift manifest reading/deserialization out of the catalog. The idea is we'll end up with two instances of the catalog per user, linked, one with the user-specific entries, and one with the entries that came with the distribution.

I also flipped some of the CHECK()s in dictionary analysis to return errors. Eventually this should throw a more meaningful error via Shell::Connect().

R=sky@chromium.org

BUG=

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

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

9 files changed, 267 insertions, 258 deletions

diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp
index 9f4c80b..26144ab 100644
--- a/mojo/mojo_shell.gyp
+++ b/mojo/mojo_shell.gyp
@@ -7,8 +7,6 @@
     'target_name': 'mojo_shell_lib',
     'type': 'static_library',
     'sources': [
-      'services/catalog/builder.cc',
-      'services/catalog/builder.h',
       'services/catalog/catalog.cc',
       'services/catalog/catalog.h',
       'services/catalog/entry.cc',
diff --git a/mojo/services/catalog/BUILD.gn b/mojo/services/catalog/BUILD.gn
index d81fde3..efeee6b 100644
--- a/mojo/services/catalog/BUILD.gn
+++ b/mojo/services/catalog/BUILD.gn
@@ -15,8 +15,6 @@ group("catalog") {
 
 source_set("lib") {
   sources = [
-    "builder.cc",
-    "builder.h",
     "catalog.cc",
     "catalog.h",
     "entry.cc",
@@ -48,7 +46,7 @@ mojo_application_manifest("manifest") {
 source_set("unittests") {
   testonly = true
   sources = [
-    "builder_unittest.cc",
+    "entry_unittest.cc",
   ]
   deps = [
     ":lib",
diff --git a/mojo/services/catalog/builder.cc b/mojo/services/catalog/builder.cc
deleted file mode 100644
index 9d7d5b7..0000000
--- a/mojo/services/catalog/builder.cc
+++ /dev/null
@@ -1,164 +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/builder.h"
-
-#include "base/values.h"
-#include "mojo/services/catalog/store.h"
-#include "mojo/shell/public/cpp/capabilities.h"
-#include "mojo/shell/public/cpp/names.h"
-
-// TODO(beng): this code should do better error handling instead of CHECKing so
-// much.
-
-namespace catalog {
-
-mojo::CapabilitySpec BuildCapabilitiesV0(
-    const base::DictionaryValue& value) {
-  mojo::CapabilitySpec capabilities;
-  base::DictionaryValue::Iterator it(value);
-  for (; !it.IsAtEnd(); it.Advance()) {
-    const base::ListValue* values = nullptr;
-    CHECK(it.value().GetAsList(&values));
-    mojo::CapabilityRequest spec;
-    for (auto i = values->begin(); i != values->end(); ++i) {
-      mojo::Interface interface_name;
-      const base::Value* v = *i;
-      CHECK(v->GetAsString(&interface_name));
-      spec.interfaces.insert(interface_name);
-    }
-    capabilities.required[it.key()] = spec;
-  }
-  return capabilities;
-}
-
-void ReadStringSet(const base::ListValue& list_value,
-                   std::set<std::string>* string_set) {
-  DCHECK(string_set);
-  for (auto i = list_value.begin(); i != list_value.end(); ++i) {
-    std::string value;
-    const base::Value* value_value = *i;
-    CHECK(value_value->GetAsString(&value));
-    string_set->insert(value);
-  }
-}
-
-void ReadStringSetFromValue(const base::Value& value,
-                            std::set<std::string>* string_set) {
-  const base::ListValue* list_value = nullptr;
-  CHECK(value.GetAsList(&list_value));
-  ReadStringSet(*list_value, string_set);
-}
-
-void ReadStringSetFromDictionary(const base::DictionaryValue& dictionary,
-                                 const std::string& key,
-                                 std::set<std::string>* string_set) {
-  const base::ListValue* list_value = nullptr;
-  if (dictionary.HasKey(key))
-    CHECK(dictionary.GetList(key, &list_value));
-  if (list_value)
-    ReadStringSet(*list_value, string_set);
-}
-
-mojo::CapabilitySpec BuildCapabilitiesV1(
-    const base::DictionaryValue& value) {
-  mojo::CapabilitySpec capabilities;
-
-  const base::DictionaryValue* provided_value = nullptr;
-  if (value.HasKey(Store::kCapabilities_ProvidedKey)) {
-    CHECK(value.GetDictionary(Store::kCapabilities_ProvidedKey,
-                              &provided_value));
-  }
-  if (provided_value) {
-    mojo::CapabilityRequest provided;
-    base::DictionaryValue::Iterator it(*provided_value);
-    for(; !it.IsAtEnd(); it.Advance()) {
-      mojo::Interfaces interfaces;
-      ReadStringSetFromValue(it.value(), &interfaces);
-      capabilities.provided[it.key()] = interfaces;
-    }
-  }
-
-  const base::DictionaryValue* required_value = nullptr;
-  if (value.HasKey(Store::kCapabilities_RequiredKey)) {
-    CHECK(value.GetDictionary(Store::kCapabilities_RequiredKey,
-                              &required_value));
-  }
-  if (required_value) {
-    base::DictionaryValue::Iterator it(*required_value);
-    for (; !it.IsAtEnd(); it.Advance()) {
-      mojo::CapabilityRequest spec;
-      const base::DictionaryValue* entry_value = nullptr;
-      CHECK(it.value().GetAsDictionary(&entry_value));
-      ReadStringSetFromDictionary(
-          *entry_value, Store::kCapabilities_ClassesKey, &spec.classes);
-      ReadStringSetFromDictionary(
-          *entry_value, Store::kCapabilities_InterfacesKey, &spec.interfaces);
-      capabilities.required[it.key()] = spec;
-    }
-  }
-  return capabilities;
-}
-
-Entry BuildEntry(const base::DictionaryValue& value) {
-  Entry entry;
-  int manifest_version = 0;
-  if (value.HasKey(Store::kManifestVersionKey))
-    CHECK(value.GetInteger(Store::kManifestVersionKey, &manifest_version));
-  std::string name_string;
-  CHECK(value.GetString(Store::kNameKey, &name_string));
-  CHECK(mojo::IsValidName(name_string)) << "Invalid Name: " << name_string;
-  entry.name = name_string;
-  if (value.HasKey(Store::kQualifierKey)) {
-    CHECK(value.GetString(Store::kQualifierKey, &entry.qualifier));
-  } else {
-    entry.qualifier = mojo::GetNamePath(name_string);
-  }
-  CHECK(value.GetString(Store::kDisplayNameKey, &entry.display_name));
-  const base::DictionaryValue* capabilities = nullptr;
-  CHECK(value.GetDictionary(Store::kCapabilitiesKey, &capabilities));
-  if (manifest_version == 0)
-    entry.capabilities = BuildCapabilitiesV0(*capabilities);
-  else
-    entry.capabilities = BuildCapabilitiesV1(*capabilities);
-  return entry;
-}
-
-scoped_ptr<base::DictionaryValue> SerializeEntry(const Entry& entry) {
-  scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue);
-  value->SetInteger(Store::kManifestVersionKey, 1);
-  value->SetString(Store::kNameKey, entry.name);
-  value->SetString(Store::kDisplayNameKey, entry.display_name);
-  value->SetString(Store::kQualifierKey, entry.qualifier);
-  scoped_ptr<base::DictionaryValue> spec(new base::DictionaryValue);
-
-  scoped_ptr<base::DictionaryValue> provided(new base::DictionaryValue);
-  for (const auto& i : entry.capabilities.provided) {
-    scoped_ptr<base::ListValue> interfaces(new base::ListValue);
-    for (const auto& interface_name : i.second)
-      interfaces->AppendString(interface_name);
-    provided->Set(i.first, std::move(interfaces));
-  }
-  spec->Set(Store::kCapabilities_ProvidedKey, std::move(provided));
-
-  scoped_ptr<base::DictionaryValue> required(new base::DictionaryValue);
-  for (const auto& i : entry.capabilities.required) {
-    scoped_ptr<base::DictionaryValue> request(new base::DictionaryValue);
-    scoped_ptr<base::ListValue> classes(new base::ListValue);
-    for (const auto& class_name : i.second.classes)
-      classes->AppendString(class_name);
-    request->Set(Store::kCapabilities_ClassesKey, std::move(classes));
-    scoped_ptr<base::ListValue> interfaces(new base::ListValue);
-    for (const auto& interface_name : i.second.interfaces)
-      interfaces->AppendString(interface_name);
-    request->Set(Store::kCapabilities_InterfacesKey, std::move(interfaces));
-    required->Set(i.first, std::move(request));
-  }
-  spec->Set(Store::kCapabilities_RequiredKey, std::move(required));
-
-  value->Set(Store::kCapabilitiesKey, std::move(spec));
-  return value;
-}
-
-}  // namespace catalog
diff --git a/mojo/services/catalog/builder.h b/mojo/services/catalog/builder.h
deleted file mode 100644
index 71eebf1..0000000
--- a/mojo/services/catalog/builder.h
+++ /dev/null
@@ -1,22 +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_BUILDER_H_
-#define MOJO_SERVICES_CATALOG_BUILDER_H_
-
-#include "mojo/services/catalog/entry.h"
-
-namespace base {
-class DictionaryValue;
-}
-
-namespace catalog {
-
-Entry BuildEntry(const base::DictionaryValue& value);
-
-scoped_ptr<base::DictionaryValue> SerializeEntry(const Entry& entry);
-
-}  // namespace catalog
-
-#endif  // MOJO_SERVICES_CATALOG_BUILDER_H_
diff --git a/mojo/services/catalog/catalog.cc b/mojo/services/catalog/catalog.cc
index d02e1fe..0bbb9d4 100644
--- a/mojo/services/catalog/catalog.cc
+++ b/mojo/services/catalog/catalog.cc
@@ -9,7 +9,6 @@
 #include "base/strings/string_split.h"
 #include "base/task_runner_util.h"
 #include "mojo/common/url_type_converters.h"
-#include "mojo/services/catalog/builder.h"
 #include "mojo/services/catalog/entry.h"
 #include "mojo/services/catalog/store.h"
 #include "mojo/shell/public/cpp/names.h"
@@ -120,8 +119,8 @@ void Catalog::GetEntries(mojo::Array<mojo::String> names,
       continue;
     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);
+    entry_ptr->display_name = entry.display_name();
+    entries[entry.name()] = std::move(entry_ptr);
   }
   callback.Run(std::move(entries));
 }
@@ -153,7 +152,8 @@ void Catalog::CompleteResolveMojoName(
   }
 
   mojo::shell::mojom::CapabilitySpecPtr capabilities_ptr =
-      mojo::shell::mojom::CapabilitySpec::From(entry_iter->second.capabilities);
+      mojo::shell::mojom::CapabilitySpec::From(
+          entry_iter->second.capabilities());
 
   callback.Run(resolved_name, qualifier, std::move(capabilities_ptr),
                file_url.spec());
@@ -193,24 +193,29 @@ void Catalog::DeserializeCatalog() {
     const base::DictionaryValue* dictionary = nullptr;
     const base::Value* v = *it;
     CHECK(v->GetAsDictionary(&dictionary));
-    const Entry entry = BuildEntry(*dictionary);
-    catalog_[entry.name] = entry;
+    scoped_ptr<Entry> entry = Entry::Deserialize(*dictionary);
+    if (entry.get())
+      catalog_[entry->name()] = *entry;
   }
 }
 
 void Catalog::SerializeCatalog() {
   scoped_ptr<base::ListValue> catalog(new base::ListValue);
   for (const auto& entry : catalog_)
-    catalog->Append(SerializeEntry(entry.second));
+    catalog->Append(entry.second.Serialize());
   if (store_)
     store_->UpdateStore(std::move(catalog));
 }
 
-const Entry& Catalog::DeserializeApplication(
+scoped_ptr<Entry> Catalog::DeserializeApplication(
     const base::DictionaryValue* dictionary) {
-  Entry entry = BuildEntry(*dictionary);
-  if (catalog_.find(entry.name) == catalog_.end()) {
-    catalog_[entry.name] = entry;
+  scoped_ptr<Entry> entry = Entry::Deserialize(*dictionary);
+  if (!entry)
+    return entry;
+
+  // 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;
@@ -218,14 +223,16 @@ const Entry& Catalog::DeserializeApplication(
       for (size_t i = 0; i < applications->GetSize(); ++i) {
         const base::DictionaryValue* child_value = nullptr;
         applications->GetDictionary(i, &child_value);
-        const Entry& child = DeserializeApplication(child_value);
-        mojo_name_aliases_[child.name] =
-            std::make_pair(entry.name, child.qualifier);
+        scoped_ptr<Entry> child = DeserializeApplication(child_value);
+        if (child) {
+          mojo_name_aliases_[child->name()] =
+              std::make_pair(entry->name(), child->qualifier());
+        }
       }
     }
-    qualifiers_[entry.name] = entry.qualifier;
+    qualifiers_[entry->name()] = entry->qualifier();
   }
-  return catalog_[entry.name];
+  return entry;
 }
 
 GURL Catalog::GetManifestURL(const std::string& name) {
@@ -262,10 +269,10 @@ void Catalog::OnReadManifestImpl(const std::string& name,
     DeserializeApplication(dictionary);
   } else {
     Entry entry;
-    entry.name = name;
-    entry.display_name = name;
-    catalog_[entry.name] = entry;
-    qualifiers_[entry.name] = mojo::GetNamePath(name);
+    entry.set_name(name);
+    entry.set_display_name(name);
+    catalog_[entry.name()] = entry;
+    qualifiers_[entry.name()] = mojo::GetNamePath(name);
   }
   SerializeCatalog();
 
diff --git a/mojo/services/catalog/catalog.h b/mojo/services/catalog/catalog.h
index dd0e53a..e4bc6a5 100644
--- a/mojo/services/catalog/catalog.h
+++ b/mojo/services/catalog/catalog.h
@@ -76,7 +76,8 @@ class Catalog : public mojom::Resolver,
   void SerializeCatalog();
 
   // Construct a catalog entry from |dictionary|.
-  const Entry& DeserializeApplication(const base::DictionaryValue* dictionary);
+  scoped_ptr<Entry> DeserializeApplication(
+      const base::DictionaryValue* dictionary);
 
   GURL GetManifestURL(const std::string& name);
 
diff --git a/mojo/services/catalog/entry.cc b/mojo/services/catalog/entry.cc
index 9aec915..528eee4 100644
--- a/mojo/services/catalog/entry.cc
+++ b/mojo/services/catalog/entry.cc
@@ -4,16 +4,188 @@
 
 #include "mojo/services/catalog/entry.h"
 
+#include "base/values.h"
+#include "mojo/services/catalog/store.h"
+#include "mojo/shell/public/cpp/names.h"
+
 namespace catalog {
+namespace {
+
+mojo::CapabilitySpec BuildCapabilitiesV0(
+    const base::DictionaryValue& value) {
+  mojo::CapabilitySpec capabilities;
+  base::DictionaryValue::Iterator it(value);
+  for (; !it.IsAtEnd(); it.Advance()) {
+    const base::ListValue* values = nullptr;
+    CHECK(it.value().GetAsList(&values));
+    mojo::CapabilityRequest spec;
+    for (auto i = values->begin(); i != values->end(); ++i) {
+      mojo::Interface interface_name;
+      const base::Value* v = *i;
+      CHECK(v->GetAsString(&interface_name));
+      spec.interfaces.insert(interface_name);
+    }
+    capabilities.required[it.key()] = spec;
+  }
+  return capabilities;
+}
+
+void ReadStringSet(const base::ListValue& list_value,
+                   std::set<std::string>* string_set) {
+  DCHECK(string_set);
+  for (auto i = list_value.begin(); i != list_value.end(); ++i) {
+    std::string value;
+    const base::Value* value_value = *i;
+    CHECK(value_value->GetAsString(&value));
+    string_set->insert(value);
+  }
+}
+
+void ReadStringSetFromValue(const base::Value& value,
+                            std::set<std::string>* string_set) {
+  const base::ListValue* list_value = nullptr;
+  CHECK(value.GetAsList(&list_value));
+  ReadStringSet(*list_value, string_set);
+}
+
+void ReadStringSetFromDictionary(const base::DictionaryValue& dictionary,
+                                 const std::string& key,
+                                 std::set<std::string>* string_set) {
+  const base::ListValue* list_value = nullptr;
+  if (dictionary.HasKey(key))
+    CHECK(dictionary.GetList(key, &list_value));
+  if (list_value)
+    ReadStringSet(*list_value, string_set);
+}
+
+mojo::CapabilitySpec BuildCapabilitiesV1(
+    const base::DictionaryValue& value) {
+  mojo::CapabilitySpec capabilities;
+
+  const base::DictionaryValue* provided_value = nullptr;
+  if (value.HasKey(Store::kCapabilities_ProvidedKey)) {
+    CHECK(value.GetDictionary(Store::kCapabilities_ProvidedKey,
+                              &provided_value));
+  }
+  if (provided_value) {
+    mojo::CapabilityRequest provided;
+    base::DictionaryValue::Iterator it(*provided_value);
+    for(; !it.IsAtEnd(); it.Advance()) {
+      mojo::Interfaces interfaces;
+      ReadStringSetFromValue(it.value(), &interfaces);
+      capabilities.provided[it.key()] = interfaces;
+    }
+  }
+
+  const base::DictionaryValue* required_value = nullptr;
+  if (value.HasKey(Store::kCapabilities_RequiredKey)) {
+    CHECK(value.GetDictionary(Store::kCapabilities_RequiredKey,
+                              &required_value));
+  }
+  if (required_value) {
+    base::DictionaryValue::Iterator it(*required_value);
+    for (; !it.IsAtEnd(); it.Advance()) {
+      mojo::CapabilityRequest spec;
+      const base::DictionaryValue* entry_value = nullptr;
+      CHECK(it.value().GetAsDictionary(&entry_value));
+      ReadStringSetFromDictionary(
+          *entry_value, Store::kCapabilities_ClassesKey, &spec.classes);
+      ReadStringSetFromDictionary(
+          *entry_value, Store::kCapabilities_InterfacesKey, &spec.interfaces);
+      capabilities.required[it.key()] = spec;
+    }
+  }
+  return capabilities;
+}
+
+}  // namespace
 
 Entry::Entry() {}
 Entry::Entry(const Entry& other) = default;
 Entry::~Entry() {}
 
+scoped_ptr<base::DictionaryValue> Entry::Serialize() const {
+  scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue);
+  value->SetInteger(Store::kManifestVersionKey, 1);
+  value->SetString(Store::kNameKey, name_);
+  value->SetString(Store::kDisplayNameKey, display_name_);
+  value->SetString(Store::kQualifierKey, qualifier_);
+  scoped_ptr<base::DictionaryValue> spec(new base::DictionaryValue);
+
+  scoped_ptr<base::DictionaryValue> provided(new base::DictionaryValue);
+  for (const auto& i : capabilities_.provided) {
+    scoped_ptr<base::ListValue> interfaces(new base::ListValue);
+    for (const auto& interface_name : i.second)
+      interfaces->AppendString(interface_name);
+    provided->Set(i.first, std::move(interfaces));
+  }
+  spec->Set(Store::kCapabilities_ProvidedKey, std::move(provided));
+
+  scoped_ptr<base::DictionaryValue> required(new base::DictionaryValue);
+  for (const auto& i : capabilities_.required) {
+    scoped_ptr<base::DictionaryValue> request(new base::DictionaryValue);
+    scoped_ptr<base::ListValue> classes(new base::ListValue);
+    for (const auto& class_name : i.second.classes)
+      classes->AppendString(class_name);
+    request->Set(Store::kCapabilities_ClassesKey, std::move(classes));
+    scoped_ptr<base::ListValue> interfaces(new base::ListValue);
+    for (const auto& interface_name : i.second.interfaces)
+      interfaces->AppendString(interface_name);
+    request->Set(Store::kCapabilities_InterfacesKey, std::move(interfaces));
+    required->Set(i.first, std::move(request));
+  }
+  spec->Set(Store::kCapabilities_RequiredKey, std::move(required));
+
+  value->Set(Store::kCapabilitiesKey, std::move(spec));
+  return value;
+}
+
+// static
+scoped_ptr<Entry> Entry::Deserialize(const base::DictionaryValue& value) {
+  scoped_ptr<Entry> entry(new Entry);
+  int manifest_version = 0;
+  if (value.HasKey(Store::kManifestVersionKey))
+    CHECK(value.GetInteger(Store::kManifestVersionKey, &manifest_version));
+  std::string name_string;
+  if (!value.GetString(Store::kNameKey, &name_string)) {
+    LOG(ERROR) << "Entry::Deserialize: dictionary has no name key";
+    return nullptr;
+  }
+  if (!mojo::IsValidName(name_string)) {
+    LOG(WARNING) << "Entry::Deserialize: " << name_string << " is not a valid "
+                 << "Mojo name";
+    return nullptr;
+  }
+  entry->set_name(name_string);
+  if (value.HasKey(Store::kQualifierKey)) {
+    std::string qualifier;
+    CHECK(value.GetString(Store::kQualifierKey, &qualifier));
+    entry->set_qualifier(qualifier);
+  } else {
+    entry->set_qualifier(mojo::GetNamePath(name_string));
+  }
+  std::string display_name;
+  if (!value.GetString(Store::kDisplayNameKey, &display_name)) {
+    LOG(WARNING) << "Entry::Deserialize: dictionary has no display_name key";
+    return nullptr;
+  }
+  entry->set_display_name(display_name);
+  const base::DictionaryValue* capabilities = nullptr;
+  if (!value.GetDictionary(Store::kCapabilitiesKey, &capabilities)) {
+    LOG(WARNING) << "Entry::Description: dictionary has no capabilities key";
+    return nullptr;
+  }
+  if (manifest_version == 0)
+    entry->set_capabilities(BuildCapabilitiesV0(*capabilities));
+  else
+    entry->set_capabilities(BuildCapabilitiesV1(*capabilities));
+  return entry;
+}
+
 bool Entry::operator==(const Entry& other) const {
-  return other.name == name && other.qualifier == qualifier &&
-         other.display_name == display_name &&
-         other.capabilities == capabilities;
+  return other.name_ == name_ && other.qualifier_ == qualifier_ &&
+         other.display_name_ == display_name_ &&
+         other.capabilities_ == capabilities_;
 }
 
 }  // catalog
diff --git a/mojo/services/catalog/entry.h b/mojo/services/catalog/entry.h
index 4b7ffa8..ac5ace5 100644
--- a/mojo/services/catalog/entry.h
+++ b/mojo/services/catalog/entry.h
@@ -7,22 +7,45 @@
 
 #include <string>
 
+#include "base/memory/scoped_ptr.h"
 #include "mojo/shell/public/cpp/capabilities.h"
 
+namespace base {
+class DictionaryValue;
+}
+
 namespace catalog {
 
 // Static information about an application package known to the Catalog.
-struct Entry {
+class Entry {
+ public:
   Entry();
-  Entry(const Entry& other);
+  explicit Entry(const Entry& other);
   ~Entry();
 
+  scoped_ptr<base::DictionaryValue> Serialize() const;
+  static scoped_ptr<Entry> Deserialize(const base::DictionaryValue& value);
+
   bool operator==(const Entry& other) const;
 
-  std::string name;
-  std::string qualifier;
-  std::string display_name;
-  mojo::CapabilitySpec capabilities;
+  const std::string& name() const { return name_; }
+  void set_name(const std::string& name) { name_ = name; }
+  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_; }
+  void set_display_name(const std::string& display_name) {
+    display_name_ = display_name;
+  }
+  const mojo::CapabilitySpec& capabilities() const { return capabilities_; }
+  void set_capabilities(const mojo::CapabilitySpec& capabilities) {
+    capabilities_ = capabilities;
+  }
+
+ private:
+  std::string name_;
+  std::string qualifier_;
+  std::string display_name_;
+  mojo::CapabilitySpec capabilities_;
 };
 
 }  // namespace catalog
diff --git a/mojo/services/catalog/builder_unittest.cc b/mojo/services/catalog/entry_unittest.cc
index 1705be8..c59e3c2 100644
--- a/mojo/services/catalog/builder_unittest.cc
+++ b/mojo/services/catalog/entry_unittest.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "mojo/services/catalog/builder.h"
+#include "mojo/services/catalog/entry.h"
 
 #include "base/files/file_path.h"
 #include "base/json/json_file_value_serializer.h"
@@ -15,19 +15,20 @@
 
 namespace catalog {
 
-class BuilderTest : public testing::Test {
+class EntryTest : public testing::Test {
  public:
-  BuilderTest() {}
-  ~BuilderTest() override {}
+  EntryTest() {}
+  ~EntryTest() override {}
 
  protected:
-  scoped_ptr<base::Value> ReadEntry(const std::string& manifest, Entry* entry) {
-    DCHECK(entry);
+  scoped_ptr<Entry> ReadEntry(const std::string& manifest,
+                              scoped_ptr<base::Value>* out_value) {
     scoped_ptr<base::Value> value = ReadManifest(manifest);
     base::DictionaryValue* dictionary = nullptr;
     CHECK(value->GetAsDictionary(&dictionary));
-    *entry = BuildEntry(*dictionary);
-    return value;
+    if (out_value)
+      *out_value = std::move(value);
+    return Entry::Deserialize(*dictionary);
   }
 
   scoped_ptr<base::Value> ReadManifest(const std::string& manifest) {
@@ -49,55 +50,50 @@ class BuilderTest : public testing::Test {
   void SetUp() override {}
   void TearDown() override {}
 
-  DISALLOW_COPY_AND_ASSIGN(BuilderTest);
+  DISALLOW_COPY_AND_ASSIGN(EntryTest);
 };
 
-TEST_F(BuilderTest, Simple) {
-  Entry entry;
-  ReadEntry("simple", &entry);
-
-  EXPECT_EQ("mojo:foo", entry.name);
-  EXPECT_EQ(mojo::GetNamePath(entry.name), entry.qualifier);
-  EXPECT_EQ("Foo", entry.display_name);
+TEST_F(EntryTest, Simple) {
+  scoped_ptr<Entry> entry = ReadEntry("simple", nullptr);
+  EXPECT_EQ("mojo:foo", entry->name());
+  EXPECT_EQ(mojo::GetNamePath(entry->name()), entry->qualifier());
+  EXPECT_EQ("Foo", entry->display_name());
 }
 
-TEST_F(BuilderTest, Instance) {
-  Entry entry;
-  ReadEntry("instance", &entry);
-
-  EXPECT_EQ("mojo:foo", entry.name);
-  EXPECT_EQ("bar", entry.qualifier);
-  EXPECT_EQ("Foo", entry.display_name);
+TEST_F(EntryTest, Instance) {
+  scoped_ptr<Entry> entry = ReadEntry("instance", nullptr);
+  EXPECT_EQ("mojo:foo", entry->name());
+  EXPECT_EQ("bar", entry->qualifier());
+  EXPECT_EQ("Foo", entry->display_name());
 }
 
-TEST_F(BuilderTest, Capabilities) {
-  Entry entry;
-  ReadEntry("capabilities", &entry);
+TEST_F(EntryTest, Capabilities) {
+  scoped_ptr<Entry> entry = ReadEntry("capabilities", nullptr);
 
-  EXPECT_EQ("mojo:foo", entry.name);
-  EXPECT_EQ("bar", entry.qualifier);
-  EXPECT_EQ("Foo", entry.display_name);
+  EXPECT_EQ("mojo:foo", entry->name());
+  EXPECT_EQ("bar", entry->qualifier());
+  EXPECT_EQ("Foo", entry->display_name());
   mojo::CapabilitySpec spec;
   mojo::CapabilityRequest request;
   request.interfaces.insert("mojo::Bar");
   spec.required["mojo:bar"] = request;
-  EXPECT_EQ(spec, entry.capabilities);
+  EXPECT_EQ(spec, entry->capabilities());
 }
 
-TEST_F(BuilderTest, Serialization) {
-  Entry entry;
-  scoped_ptr<base::Value> value = ReadEntry("serialization", &entry);
+TEST_F(EntryTest, Serialization) {
+  scoped_ptr<base::Value> value;
+  scoped_ptr<Entry> entry = ReadEntry("serialization", &value);
 
-  scoped_ptr<base::DictionaryValue> serialized(SerializeEntry(entry));
+  scoped_ptr<base::DictionaryValue> serialized(entry->Serialize());
 
   // We can't just compare values, since during deserialization some of the
   // lists get converted to std::sets, which are sorted, so Value::Equals will
   // fail.
-  Entry reconstituted = BuildEntry(*serialized.get());
-  EXPECT_EQ(entry, reconstituted);
+  scoped_ptr<Entry> reconstituted = Entry::Deserialize(*serialized.get());
+  EXPECT_EQ(*entry, *reconstituted);
 }
 
-TEST_F(BuilderTest, Malformed) {
+TEST_F(EntryTest, Malformed) {
   scoped_ptr<base::Value> value = ReadManifest("malformed");
   EXPECT_FALSE(value.get());
 }