[Extensions] Update ManifestTest to use new base::Value API

Update the ManifestTest class to use the new base::Value API and
movable types, rather than base::DictionaryValues and unique_ptrs.

Bug: None

Change-Id: Id1cd42dc1842957db5600e36e0bae75b10b71247
Reviewed-on: https://chromium-review.googlesource.com/1039780
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560296}

chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc

@@ -32,12 +32,11 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPermission) {
 
 TEST_F(ExtensionManifestBackgroundTest, BackgroundScripts) {
   std::string error;
-  std::unique_ptr<base::DictionaryValue> manifest =
-      LoadManifest("background_scripts.json", &error);
-  ASSERT_TRUE(manifest.get());
+  base::Value manifest = LoadManifest("background_scripts.json", &error);
+  ASSERT_TRUE(manifest.is_dict());
 
   scoped_refptr<Extension> extension(
-      LoadAndExpectSuccess(ManifestData(manifest.get(), "")));
+      LoadAndExpectSuccess(ManifestData(&manifest, "")));
   ASSERT_TRUE(extension.get());
   const std::vector<std::string>& background_scripts =
       BackgroundInfo::GetBackgroundScripts(extension.get());
@@ -50,8 +49,8 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundScripts) {
       std::string("/") + kGeneratedBackgroundPageFilename,
       BackgroundInfo::GetBackgroundURL(extension.get()).path());
 
-  manifest->SetString("background.page", "monkey.html");
-  LoadAndExpectError(ManifestData(manifest.get(), ""),
+  manifest.SetPath({"background", "page"}, base::Value("monkey.html"));
+  LoadAndExpectError(ManifestData(&manifest, ""),
                      errors::kInvalidBackgroundCombination);
 }
 
@@ -79,20 +78,19 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPageWebRequest) {
   ScopedCurrentChannel current_channel(version_info::Channel::DEV);
 
   std::string error;
-  std::unique_ptr<base::DictionaryValue> manifest(
-      LoadManifest("background_page.json", &error));
-  ASSERT_TRUE(manifest.get());
-  manifest->SetBoolean(keys::kBackgroundPersistent, false);
-  manifest->SetInteger(keys::kManifestVersion, 2);
+  base::Value manifest = LoadManifest("background_page.json", &error);
+  ASSERT_FALSE(manifest.is_none());
+  manifest.SetPath({"background", "persistent"}, base::Value(false));
+  manifest.SetKey(keys::kManifestVersion, base::Value(2));
   scoped_refptr<Extension> extension(
-      LoadAndExpectSuccess(ManifestData(manifest.get(), "")));
+      LoadAndExpectSuccess(ManifestData(&manifest, "")));
   ASSERT_TRUE(extension.get());
   EXPECT_TRUE(BackgroundInfo::HasLazyBackgroundPage(extension.get()));
 
-  auto permissions = std::make_unique<base::ListValue>();
-  permissions->AppendString("webRequest");
-  manifest->Set(keys::kPermissions, std::move(permissions));
-  LoadAndExpectError(ManifestData(manifest.get(), ""),
+  base::Value permissions(base::Value::Type::LIST);
+  permissions.GetList().push_back(base::Value("webRequest"));
+  manifest.SetKey(keys::kPermissions, std::move(permissions));
+  LoadAndExpectError(ManifestData(&manifest, ""),
                      errors::kWebRequestConflictsWithLazyBackground);
 }
 

chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc

@@ -117,18 +117,17 @@ TEST_F(PlatformAppsManifestTest, CertainApisRequirePlatformApps) {
   // testing. The requirements are that (1) it be a valid platform app, and (2)
   // it contain no permissions dictionary.
   std::string error;
-  std::unique_ptr<base::DictionaryValue> manifest(
-      LoadManifest("init_valid_platform_app.json", &error));
+  base::Value manifest = LoadManifest("init_valid_platform_app.json", &error);
 
   std::vector<std::unique_ptr<ManifestData>> manifests;
   // Create each manifest.
   for (const char* api_name : kPlatformAppExperimentalApis) {
-    auto permissions = std::make_unique<base::ListValue>();
-    permissions->AppendString("experimental");
-    permissions->AppendString(api_name);
-    manifest->Set("permissions", std::move(permissions));
+    base::Value permissions(base::Value::Type::LIST);
+    permissions.GetList().push_back(base::Value("experimental"));
+    permissions.GetList().push_back(base::Value(api_name));
+    manifest.SetKey("permissions", std::move(permissions));
     manifests.push_back(
-        std::make_unique<ManifestData>(manifest->CreateDeepCopy(), ""));
+        std::make_unique<ManifestData>(manifest.CreateDeepCopy(), ""));
   }
   // First try to load without any flags. This should warn for every API.
   for (const std::unique_ptr<ManifestData>& manifest : manifests) {

chrome/common/extensions/manifest_tests/extension_manifests_validapp_unittest.cc

@@ -27,10 +27,10 @@ TEST_F(ValidAppManifestTest, ValidApp) {
 
 TEST_F(ValidAppManifestTest, AllowUnrecognizedPermissions) {
   std::string error;
-  std::unique_ptr<base::DictionaryValue> manifest(
-      LoadManifest("valid_app.json", &error));
-  base::ListValue* permissions = NULL;
-  ASSERT_TRUE(manifest->GetList("permissions", &permissions));
-  permissions->AppendString("not-a-valid-permission");
+  base::Value manifest = LoadManifest("valid_app.json", &error);
+  base::Value* permissions =
+      manifest.FindKeyOfType("permissions", base::Value::Type::LIST);
+  ASSERT_TRUE(permissions);
+  permissions->GetList().emplace_back("not-a-valid-permission");
   LoadAndExpectSuccess(ManifestData(std::move(manifest), ""));
 }

extensions/common/manifest_test.cc

@@ -21,29 +21,33 @@ namespace extensions {
 namespace {
 
 // |manifest_path| is an absolute path to a manifest file.
-std::unique_ptr<base::DictionaryValue> LoadManifestFile(
-    const base::FilePath& manifest_path,
-    std::string* error) {
+base::Value LoadManifestFile(const base::FilePath& manifest_path,
+                             std::string* error) {
   base::FilePath extension_path = manifest_path.DirName();
 
   EXPECT_TRUE(base::PathExists(manifest_path)) <<
       "Couldn't find " << manifest_path.value();
 
   JSONFileValueDeserializer deserializer(manifest_path);
-  std::unique_ptr<base::DictionaryValue> manifest =
-      base::DictionaryValue::From(deserializer.Deserialize(NULL, error));
+  std::unique_ptr<base::Value> manifest =
+      deserializer.Deserialize(nullptr, error);
+
+  if (!manifest || !manifest->is_dict())
+    return base::Value();
 
   // Most unit tests don't need localization, and they'll fail if we try to
   // localize them, since their manifests don't have a default_locale key.
   // Only localize manifests that indicate they want to be localized.
   // Calling LocalizeExtension at this point mirrors file_util::LoadExtension.
-  if (manifest &&
-      manifest_path.value().find(FILE_PATH_LITERAL("localized")) !=
-      std::string::npos)
-    extension_l10n_util::LocalizeExtension(extension_path, manifest.get(),
+  if (manifest_path.value().find(FILE_PATH_LITERAL("localized")) !=
+      std::string::npos) {
+    base::DictionaryValue* manifest_dictionary = nullptr;
+    manifest->GetAsDictionary(&manifest_dictionary);
+    extension_l10n_util::LocalizeExtension(extension_path, manifest_dictionary,
                                            error);
+  }
 
-  return manifest;
+  return base::Value(std::move(*manifest));
 }
 
 }  // namespace
@@ -57,47 +61,35 @@ ManifestTest::~ManifestTest() {
 
 // Helper class that simplifies creating methods that take either a filename
 // to a manifest or the manifest itself.
-ManifestTest::ManifestData::ManifestData(const char* name)
-    : name_(name), manifest_(nullptr) {}
-
-// This does not take ownership of |manifest|.
-ManifestTest::ManifestData::ManifestData(base::DictionaryValue* manifest,
-                                          const char* name)
-    : name_(name), manifest_(manifest) {
-  CHECK(manifest_) << "Manifest NULL";
-}
+ManifestTest::ManifestData::ManifestData(base::StringPiece name)
+    : name_(name.as_string()) {}
 
-ManifestTest::ManifestData::ManifestData(
-    std::unique_ptr<base::DictionaryValue> manifest)
-    : manifest_(manifest.get()), manifest_holder_(std::move(manifest)) {
-  CHECK(manifest_) << "Manifest NULL";
+ManifestTest::ManifestData::ManifestData(base::Value manifest,
+                                         base::StringPiece name)
+    : name_(name.as_string()), manifest_(std::move(manifest)) {
+  CHECK(manifest_.is_dict()) << "Manifest must be a dictionary. " << name_;
 }
 
-ManifestTest::ManifestData::ManifestData(
-    std::unique_ptr<base::DictionaryValue> manifest,
-    const char* name)
-    : name_(name),
-      manifest_(manifest.get()),
-      manifest_holder_(std::move(manifest)) {
-  CHECK(manifest_) << "Manifest NULL";
-}
+ManifestTest::ManifestData::ManifestData(ManifestData&& other) = default;
 
-ManifestTest::ManifestData::ManifestData(const ManifestData& m) {
-  NOTREACHED();
-}
+ManifestTest::ManifestData::ManifestData(base::Value* manifest,
+                                         const char* name)
+    : ManifestData(manifest->Clone(), name) {}
+
+ManifestTest::ManifestData::ManifestData(std::unique_ptr<base::Value> manifest,
+                                         const char* name)
+    : ManifestData(base::Value(std::move(*manifest)), name) {}
 
 ManifestTest::ManifestData::~ManifestData() {
 }
 
-base::DictionaryValue* ManifestTest::ManifestData::GetManifest(
+const base::Value& ManifestTest::ManifestData::GetManifest(
     const base::FilePath& test_data_dir,
     std::string* error) const {
-  if (manifest_)
-    return manifest_;
-
-  base::FilePath manifest_path = test_data_dir.AppendASCII(name_);
-  manifest_holder_ = LoadManifestFile(manifest_path, error);
-  manifest_ = manifest_holder_.get();
+  if (manifest_.is_none()) {
+    base::FilePath manifest_path = test_data_dir.AppendASCII(name_);
+    manifest_ = LoadManifestFile(manifest_path, error);
+  }
   return manifest_;
 }
 
@@ -111,9 +103,8 @@ base::FilePath ManifestTest::GetTestDataDir() {
   return path.AppendASCII("manifest_tests");
 }
 
-std::unique_ptr<base::DictionaryValue> ManifestTest::LoadManifest(
-    char const* manifest_name,
-    std::string* error) {
+base::Value ManifestTest::LoadManifest(char const* manifest_name,
+                                       std::string* error) {
   base::FilePath manifest_path = GetTestDataDir().AppendASCII(manifest_name);
   return LoadManifestFile(manifest_path, error);
 }
@@ -124,11 +115,15 @@ scoped_refptr<Extension> ManifestTest::LoadExtension(
     extensions::Manifest::Location location,
     int flags) {
   base::FilePath test_data_dir = GetTestDataDir();
-  base::DictionaryValue* value = manifest.GetManifest(test_data_dir, error);
-  if (!value)
-    return NULL;
-  return Extension::Create(test_data_dir.DirName(), location, *value, flags,
-      GetTestExtensionID(), error);
+  const base::Value& value = manifest.GetManifest(test_data_dir, error);
+  if (value.is_none())
+    return nullptr;
+  DCHECK(value.is_dict());
+  const base::DictionaryValue* dictionary_manifest = nullptr;
+  value.GetAsDictionary(&dictionary_manifest);
+  return Extension::Create(test_data_dir.DirName(), location,
+                           *dictionary_manifest, flags, GetTestExtensionID(),
+                           error);
 }
 
 scoped_refptr<Extension> ManifestTest::LoadAndExpectSuccess(

extensions/common/manifest_test.h

@@ -34,33 +34,26 @@ class ManifestTest : public testing::Test {
   // to a manifest or the manifest itself.
   class ManifestData {
    public:
-    explicit ManifestData(const char* name);
-    ManifestData(base::DictionaryValue* manifest, const char* name);
-    explicit ManifestData(std::unique_ptr<base::DictionaryValue> manifest);
-    explicit ManifestData(std::unique_ptr<base::DictionaryValue> manifest,
+    explicit ManifestData(base::StringPiece name);
+    ManifestData(base::Value manifest, base::StringPiece name);
+
+    ManifestData(ManifestData&& other);
+
+    // DEPRECATED. Use one of the above constructors.
+    ManifestData(base::Value* manifest, const char* name);
+    explicit ManifestData(std::unique_ptr<base::Value> manifest,
                           const char* name);
-    // C++98 requires the copy constructor for a type to be visible if you
-    // take a const-ref of a temporary for that type.  Since Manifest
-    // contains a scoped_ptr, its implicit copy constructor is declared
-    // Manifest(Manifest&) according to spec 12.8.5.  This breaks the first
-    // requirement and thus you cannot use it with LoadAndExpectError() or
-    // LoadAndExpectSuccess() easily.
-    //
-    // To get around this spec pedantry, we declare the copy constructor
-    // explicitly.  It will never get invoked.
-    ManifestData(const ManifestData& m);
 
     ~ManifestData();
 
     const std::string& name() const { return name_; }
 
-    base::DictionaryValue* GetManifest(const base::FilePath& manifest_path,
-                                       std::string* error) const;
+    const base::Value& GetManifest(const base::FilePath& manifest_path,
+                                   std::string* error) const;
 
    private:
     const std::string name_;
-    mutable base::DictionaryValue* manifest_;
-    mutable std::unique_ptr<base::DictionaryValue> manifest_holder_;
+    mutable base::Value manifest_;
   };
 
   // Allows the test implementation to override a loaded test manifest's
@@ -71,8 +64,7 @@ class ManifestTest : public testing::Test {
   // extensions/test/data/manifest_tests.
   virtual base::FilePath GetTestDataDir();
 
-  std::unique_ptr<base::DictionaryValue> LoadManifest(char const* manifest_name,
-                                                      std::string* error);
+  base::Value LoadManifest(char const* manifest_name, std::string* error);
 
   scoped_refptr<extensions::Extension> LoadExtension(
       const ManifestData& manifest,