[Extensions] Clean up and modernize ManifestTest and derivatives.

This CL cleans up quite a bit in this base class and derived tests:

1. The ManifestData class was using base::Value and asserting that a dictionary type was provided. It now uses base::Value::Dict and is type safe by design.

2. Along with using base::Value, it was using an instance with no type to signal error conditions. The new code returns absl::optional<base::Value::Dict> if an error occurs.

3. In many cases, ManifestData was constructed supplying both a dictionary for the manifest and name. In the cases where the dictionary specifies the name, there is no need to supply the name. Instead, the code now uses the constructor that looks up the name in the dictionary. This also fixes cases where the name in the dictionary and the name parameter were different.

4. Some places where base::Value was used have been converted to the more specific types of base::Value::Dict and base::Value::List.

5. Places where std::unique_ptr was used have been replaced with stack-based values.

Bug: 1187061
Change-Id: I29452e8b46d3240344a55283c0fe90c2e074d606
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4113017
Reviewed-by: Solomon Kinard <solomonkinard@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085090}

chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc

@@ -127,11 +127,11 @@ TEST_F(BrowserActionManifestTest,
           }
         }
       })");
+  ASSERT_TRUE(manifest_value.is_dict());
   std::u16string error =
       ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidIconPath, "19");
-  LoadAndExpectError(
-      ManifestData(std::move(manifest_value), "Invalid default icon"),
-      errors::kInvalidIconPath);
+  LoadAndExpectError(ManifestData(std::move(manifest_value).TakeDict()),
+                     errors::kInvalidIconPath);
 }
 
 }  // namespace

chrome/common/extensions/api/system_indicator/system_indicator_handler_unittest.cc

@@ -38,8 +38,8 @@ TEST_F(SystemIndicatorHandlerTest, BasicTests) {
              "system_indicator": { "default_icon": "icon.png" }
            })";
     scoped_refptr<const Extension> extension = LoadAndExpectSuccess(
-        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey)),
-                     "icon"));
+        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey))
+                         .TakeDict()));
     ASSERT_TRUE(extension);
     const ExtensionIconSet* icon =
         SystemIndicatorHandler::GetSystemIndicatorIcon(*extension);
@@ -69,8 +69,8 @@ TEST_F(SystemIndicatorHandlerTest, BasicTests) {
              }
            })";
     scoped_refptr<const Extension> extension = LoadAndExpectSuccess(
-        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey)),
-                     "icon"));
+        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey))
+                         .TakeDict()));
     ASSERT_TRUE(extension);
     const ExtensionIconSet* icon =
         SystemIndicatorHandler::GetSystemIndicatorIcon(*extension);
@@ -95,8 +95,8 @@ TEST_F(SystemIndicatorHandlerTest, BasicTests) {
              "system_indicator": {}
            })";
     scoped_refptr<const Extension> extension = LoadAndExpectSuccess(
-        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey)),
-                     "icon"));
+        ManifestData(base::test::ParseJson(base::StringPrintf(kManifest, kKey))
+                         .TakeDict()));
     ASSERT_TRUE(extension);
     const ExtensionIconSet* icon =
         SystemIndicatorHandler::GetSystemIndicatorIcon(*extension);

chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc

@@ -36,11 +36,12 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPermission) {
 
 TEST_F(ExtensionManifestBackgroundTest, BackgroundScripts) {
   std::string error;
-  base::Value manifest = LoadManifest("background_scripts.json", &error);
-  ASSERT_TRUE(manifest.is_dict());
+  absl::optional<base::Value::Dict> manifest =
+      LoadManifest("background_scripts.json", &error);
+  ASSERT_TRUE(manifest);
 
   scoped_refptr<Extension> extension(
-      LoadAndExpectSuccess(ManifestData(manifest.Clone(), "")));
+      LoadAndExpectSuccess(ManifestData(manifest->Clone(), "")));
   ASSERT_TRUE(extension.get());
   const std::vector<std::string>& background_scripts =
       BackgroundInfo::GetBackgroundScripts(extension.get());
@@ -53,26 +54,27 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundScripts) {
       std::string("/") + kGeneratedBackgroundPageFilename,
       BackgroundInfo::GetBackgroundURL(extension.get()).path());
 
-  manifest.SetPath({"background", "page"}, base::Value("monkey.html"));
-  LoadAndExpectError(ManifestData(std::move(manifest), ""),
+  manifest->SetByDottedPath("background.page", "monkey.html");
+  LoadAndExpectError(ManifestData(std::move(*manifest), ""),
                      errors::kInvalidBackgroundCombination);
 }
 
 TEST_F(ExtensionManifestBackgroundTest, BackgroundServiceWorkerScript) {
   std::string error;
-  base::Value manifest = LoadManifest("background_script_sw.json", &error);
-  ASSERT_TRUE(manifest.is_dict());
+  absl::optional<base::Value::Dict> manifest =
+      LoadManifest("background_script_sw.json", &error);
+  ASSERT_TRUE(manifest);
 
   scoped_refptr<Extension> extension(
-      LoadAndExpectSuccess(ManifestData(manifest.Clone(), "")));
+      LoadAndExpectSuccess(ManifestData(manifest->Clone(), "")));
   ASSERT_TRUE(extension.get());
   ASSERT_TRUE(BackgroundInfo::IsServiceWorkerBased(extension.get()));
   const std::string& service_worker_script =
       BackgroundInfo::GetBackgroundServiceWorkerScript(extension.get());
   EXPECT_EQ("service_worker.js", service_worker_script);
 
-  manifest.SetPath({"background", "page"}, base::Value("monkey.html"));
-  LoadAndExpectError(ManifestData(std::move(manifest), ""),
+  manifest->SetByDottedPath("background.page", "monkey.html");
+  LoadAndExpectError(ManifestData(std::move(*manifest), ""),
                      errors::kInvalidBackgroundCombination);
 }
 
@@ -100,19 +102,20 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPageWebRequest) {
   ScopedCurrentChannel current_channel(version_info::Channel::DEV);
 
   std::string error;
-  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));
+  absl::optional<base::Value::Dict> manifest =
+      LoadManifest("background_page.json", &error);
+  ASSERT_TRUE(manifest);
+  manifest->SetByDottedPath("background.persistent", false);
+  manifest->Set(keys::kManifestVersion, 2);
   scoped_refptr<Extension> extension(
-      LoadAndExpectSuccess(ManifestData(manifest.Clone(), "")));
+      LoadAndExpectSuccess(ManifestData(manifest->Clone(), "")));
   ASSERT_TRUE(extension.get());
   EXPECT_TRUE(BackgroundInfo::HasLazyBackgroundPage(extension.get()));
 
-  base::Value permissions(base::Value::Type::LIST);
-  permissions.Append(base::Value("webRequest"));
-  manifest.SetKey(keys::kPermissions, std::move(permissions));
-  LoadAndExpectError(ManifestData(std::move(manifest), ""),
+  base::Value::List permissions;
+  permissions.Append("webRequest");
+  manifest->Set(keys::kPermissions, std::move(permissions));
+  LoadAndExpectError(ManifestData(std::move(*manifest), ""),
                      errors::kWebRequestConflictsWithLazyBackground);
 }
 
@@ -128,8 +131,8 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPageTransientBackground) {
             "background": {
               "page": "foo.html"
             }
-          })"),
-                                        "")));
+          })")
+                                            .TakeDict())));
   ASSERT_TRUE(extension.get());
   EXPECT_TRUE(BackgroundInfo::HasPersistentBackgroundPage(extension.get()));
 
@@ -145,8 +148,8 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPageTransientBackground) {
             "background": {
               "page": "foo.html"
             }
-          })"),
-                   ""),
+          })")
+                       .TakeDict()),
       errors::kTransientBackgroundConflictsWithPersistentBackground);
 }
 
@@ -232,7 +235,7 @@ TEST_F(ExtensionManifestBackgroundTest, ManifestV3Restrictions) {
            })";
     base::Value manifest_value = base::test::ParseJson(kManifestBackgroundPage);
     LoadAndExpectError(
-        ManifestData(std::move(manifest_value), "background page"),
+        ManifestData(std::move(manifest_value).TakeDict(), "background page"),
         get_expected_error(keys::kBackgroundPage));
   }
   {
@@ -247,9 +250,9 @@ TEST_F(ExtensionManifestBackgroundTest, ManifestV3Restrictions) {
            })";
     base::Value manifest_value =
         base::test::ParseJson(kManifestBackgroundScripts);
-    LoadAndExpectError(
-        ManifestData(std::move(manifest_value), "background scripts"),
-        get_expected_error(keys::kBackgroundScripts));
+    LoadAndExpectError(ManifestData(std::move(manifest_value).TakeDict(),
+                                    "background scripts"),
+                       get_expected_error(keys::kBackgroundScripts));
   }
   {
     constexpr char kManifestBackgroundPersistent[] =
@@ -264,9 +267,9 @@ TEST_F(ExtensionManifestBackgroundTest, ManifestV3Restrictions) {
            })";
     base::Value manifest_value =
         base::test::ParseJson(kManifestBackgroundPersistent);
-    LoadAndExpectError(
-        ManifestData(std::move(manifest_value), "persistent background"),
-        get_expected_error(keys::kBackgroundPersistent));
+    LoadAndExpectError(ManifestData(std::move(manifest_value).TakeDict(),
+                                    "persistent background"),
+                       get_expected_error(keys::kBackgroundPersistent));
   }
   {
     // An extension with no background key present should still be allowed.
@@ -279,7 +282,7 @@ TEST_F(ExtensionManifestBackgroundTest, ManifestV3Restrictions) {
     base::Value manifest_value =
         base::test::ParseJson(kManifestBackgroundPersistent);
     LoadAndExpectSuccess(
-        ManifestData(std::move(manifest_value), "no background"));
+        ManifestData(std::move(manifest_value).TakeDict(), "no background"));
   }
 }
 
@@ -294,7 +297,7 @@ TEST_F(ExtensionManifestBackgroundTest, ModuleServiceWorker) {
          })";
     std::string manifest_str =
         base::StringPrintf(kManifestStub, background_value);
-    return base::test::ParseJson(manifest_str);
+    return base::test::ParseJson(manifest_str).TakeDict();
   };
 
   {

chrome/common/extensions/manifest_tests/extension_manifests_manifest_version_unittest.cc

@@ -15,32 +15,32 @@ using extensions::Extension;
 namespace errors = extensions::manifest_errors;
 
 TEST_F(ChromeManifestTest, ManifestVersionError) {
-  base::Value mv_missing(base::Value::Type::DICTIONARY);
-  mv_missing.SetStringKey("name", "Miles");
-  mv_missing.SetStringKey("version", "0.55");
+  base::Value::Dict mv_missing;
+  mv_missing.Set("name", "Miles");
+  mv_missing.Set("version", "0.55");
 
-  base::Value mv0 = mv_missing.Clone();
-  mv0.SetIntKey("manifest_version", 0);
+  base::Value::Dict mv0 = mv_missing.Clone();
+  mv0.Set("manifest_version", 0);
 
-  base::Value mv1 = mv_missing.Clone();
-  mv1.SetIntKey("manifest_version", 1);
+  base::Value::Dict mv1 = mv_missing.Clone();
+  mv1.Set("manifest_version", 1);
 
-  base::Value mv2 = mv_missing.Clone();
-  mv2.SetIntKey("manifest_version", 2);
+  base::Value::Dict mv2 = mv_missing.Clone();
+  mv2.Set("manifest_version", 2);
 
-  base::Value mv3 = mv_missing.Clone();
-  mv3.SetIntKey("manifest_version", 3);
+  base::Value::Dict mv3 = mv_missing.Clone();
+  mv3.Set("manifest_version", 3);
 
-  base::Value mv4 = mv_missing.Clone();
-  mv4.SetIntKey("manifest_version", 4);
+  base::Value::Dict mv4 = mv_missing.Clone();
+  mv4.Set("manifest_version", 4);
 
-  base::Value mv_string = mv_missing.Clone();
-  mv_string.SetStringKey("manifest_version", "2");
+  base::Value::Dict mv_string = mv_missing.Clone();
+  mv_string.Set("manifest_version", "2");
 
   struct {
     const char* test_name;
     bool require_modern_manifest_version;
-    base::Value manifest;
+    base::Value::Dict manifest;
     std::string expected_error;
   } test_data[] = {
       {"require_modern_with_default", true, mv_missing.Clone(),

chrome/common/extensions/manifest_tests/extension_manifests_name_unittest.cc

@@ -22,7 +22,7 @@ class NameManifestTest : public ChromeManifestTest {
     base::Value manifest_value = base::test::ParseJson(
         base::StringPrintf(kManifestStub, name, manifest_version));
     CHECK(manifest_value.is_dict());
-    return ManifestData(std::move(manifest_value), "test");
+    return ManifestData(std::move(manifest_value).TakeDict());
   }
 };
 

chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc

@@ -109,9 +109,9 @@ TEST_F(PlatformAppsManifestTest, PlatformAppContentSecurityPolicy) {
 TEST_F(PlatformAppsManifestTest, CertainApisRequirePlatformApps) {
   // Put APIs here that should be restricted to platform apps, but that haven't
   // yet graduated from experimental.
-  const char* const kPlatformAppExperimentalApis[] = {
-    "dns",
-    "serial",
+  static constexpr const char* kPlatformAppExperimentalApis[] = {
+      "dns",
+      "serial",
   };
   // TODO(miket): When the first platform-app API leaves experimental, write
   // similar code that tests without the experimental flag.
@@ -120,32 +120,33 @@ 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;
-  base::Value platform_app_manifest =
+  absl::optional<base::Value::Dict> platform_app_manifest =
       LoadManifest("init_valid_platform_app.json", &error);
+  ASSERT_TRUE(platform_app_manifest);
 
-  std::vector<std::unique_ptr<ManifestData>> manifests;
+  std::vector<ManifestData> manifests;
   // Create each manifest.
   for (const char* api_name : kPlatformAppExperimentalApis) {
-    base::Value permissions(base::Value::Type::LIST);
-    permissions.Append(base::Value("experimental"));
-    permissions.Append(base::Value(api_name));
-    platform_app_manifest.SetKey("permissions", std::move(permissions));
-    manifests.push_back(
-        std::make_unique<ManifestData>(platform_app_manifest.Clone(), ""));
+    base::Value::List permissions;
+    permissions.Append("experimental");
+    permissions.Append(api_name);
+    platform_app_manifest->Set("permissions", std::move(permissions));
+    manifests.emplace_back(platform_app_manifest->Clone());
   }
   // First try to load without any flags. This should warn for every API.
-  for (const std::unique_ptr<ManifestData>& manifest : manifests) {
+  for (const auto& manifest : manifests) {
     LoadAndExpectWarning(
-        *manifest,
+        manifest,
         "'experimental' requires the 'experimental-extension-apis' "
         "command line switch to be enabled.");
   }
 
   // Now try again with the experimental flag set.
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableExperimentalExtensionApis);
-  for (const std::unique_ptr<ManifestData>& manifest : manifests)
-    LoadAndExpectSuccess(*manifest);
+  for (const auto& manifest : manifests) {
+    LoadAndExpectSuccess(manifest);
+  }
 }
 
 }  // namespace extensions

chrome/common/extensions/manifest_tests/extension_manifests_side_panel_unittest.cc

@@ -26,8 +26,8 @@ class SidePanelManifestTest : public ChromeManifestTest {
       })";
     base::Value manifest_value = base::test::ParseJson(base::StringPrintf(
         kManifestStub, manifest_version, side_panel.c_str()));
-    EXPECT_EQ(base::Value::Type::DICTIONARY, manifest_value.type());
-    return ManifestData(std::move(manifest_value), "test");
+    EXPECT_TRUE(manifest_value.is_dict());
+    return ManifestData(std::move(manifest_value).TakeDict());
   }
 };
 

chrome/common/extensions/manifest_tests/extension_manifests_validapp_unittest.cc

@@ -9,6 +9,7 @@
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/extensions/manifest_tests/chrome_manifest_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 typedef ChromeManifestTest ValidAppManifestTest;
 
@@ -27,10 +28,11 @@ TEST_F(ValidAppManifestTest, ValidApp) {
 
 TEST_F(ValidAppManifestTest, AllowUnrecognizedPermissions) {
   std::string error;
-  base::Value manifest = LoadManifest("valid_app.json", &error);
-  base::Value* permissions =
-      manifest.FindKeyOfType("permissions", base::Value::Type::LIST);
+  absl::optional<base::Value::Dict> manifest =
+      LoadManifest("valid_app.json", &error);
+  ASSERT_TRUE(manifest);
+  base::Value::List* permissions = manifest->FindList("permissions");
   ASSERT_TRUE(permissions);
   permissions->Append("not-a-valid-permission");
-  LoadAndExpectSuccess(ManifestData(std::move(manifest), ""));
+  LoadAndExpectSuccess(ManifestData(std::move(*manifest), ""));
 }

chrome/common/extensions/manifest_tests/extension_manifests_web_accessible_resources_unittest.cc

@@ -26,8 +26,8 @@ class WebAccessibleResourcesManifestTest : public ChromeManifestTest {
         })";
     base::Value manifest_value = base::test::ParseJson(base::StringPrintf(
         kManifestStub, manifest_version, web_accessible_resources.c_str()));
-    EXPECT_EQ(base::Value::Type::DICTIONARY, manifest_value.type());
-    return ManifestData(std::move(manifest_value), "test");
+    EXPECT_TRUE(manifest_value.is_dict());
+    return ManifestData(std::move(manifest_value).TakeDict());
   }
 };
 
@@ -324,8 +324,8 @@ TEST_F(WebAccessibleResourcesManifestTest,
           })";
     base::Value manifest_value = base::test::ParseJson(
         base::StringPrintf(kManifestStub, extension_id.c_str()));
-    EXPECT_EQ(base::Value::Type::DICTIONARY, manifest_value.type());
-    return ManifestData(std::move(manifest_value), "test");
+    EXPECT_TRUE(manifest_value.is_dict());
+    return ManifestData(std::move(manifest_value).TakeDict());
   };
   scoped_refptr<const Extension> extension_callee =
       LoadAndExpectSuccess(get_manifest_data());
@@ -479,8 +479,8 @@ TEST_F(WebAccessibleResourcesManifestTest, ShouldUseDynamicUrl) {
         "manifest_version": 3
     })";
   base::Value manifest_value = base::test::ParseJson(kManifestStub);
-  EXPECT_EQ(base::Value::Type::DICTIONARY, manifest_value.type());
-  auto manifest_data = ManifestData(std::move(manifest_value), "test");
+  ASSERT_TRUE(manifest_value.is_dict());
+  ManifestData manifest_data(std::move(manifest_value).TakeDict());
   scoped_refptr<Extension> extension(LoadAndExpectSuccess(manifest_data));
   EXPECT_EQ(false, WebAccessibleResourcesInfo::ShouldUseDynamicUrl(
                        extension.get(), "resource.html"));

chrome/common/extensions/manifest_tests/permissions_parser_unittest.cc

@@ -225,8 +225,8 @@ TEST_F(PermissionsParserTest, ChromeFavicon) {
                                       : manifest_keys::kPermissions;
     base::Value manifest_value = base::test::ParseJson(base::StringPrintf(
         kManifestStub, manifest_version, permissions_key, permission));
-    EXPECT_EQ(base::Value::Type::DICTIONARY, manifest_value.type());
-    return ManifestData(std::move(manifest_value), permission);
+    EXPECT_TRUE(manifest_value.is_dict());
+    return ManifestData(std::move(manifest_value).TakeDict(), permission);
   };
 
   static constexpr char kFaviconPattern[] = "chrome://favicon/*";