[Extensions] Add ExtensionBuilder::SetManifest[Key|Path]()

Add utility methods to ExtensionBuilder to allow for setting a manifest key or path without needing to go through MergeManifest(). This saves the hassle of needing to construct a dictionary with a key in order to set a manifest value. Use it in a smattering of places. Bug: 832958 Change-Id: If885273205572ba72787e21acc1a75da4f861751 Reviewed-on: https://chromium-review.googlesource.com/1048848 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Devlin <rdevlin.cronin@chromium.org> Cr-Commit-Position: refs/heads/master@{#556917}
FairyWorld · May 8, 2018 · 98cd658 · 98cd658
1 parent dcbf21b
commit 98cd658
Show file tree

Hide file tree

Showing 16 changed files with 106 additions and 78 deletions.
diff --git a/chrome/browser/extensions/api/file_system/consent_provider_unittest.cc b/chrome/browser/extensions/api/file_system/consent_provider_unittest.cc
@@ -181,10 +181,8 @@ TEST_F(FileSystemApiConsentProviderTest, ForKioskApps) {
   {
     scoped_refptr<Extension> auto_launch_kiosk_app(
         ExtensionBuilder("Test", ExtensionBuilder::Type::PLATFORM_APP)
-            .MergeManifest(DictionaryBuilder()
-                               .Set("kiosk_enabled", true)
-                               .Set("kiosk_only", true)
-                               .Build())
+            .SetManifestKey("kiosk_enabled", true)
+            .SetManifestKey("kiosk_only", true)
             .Build());
     user_manager_->AddKioskAppUser(
         AccountId::FromUserEmail(auto_launch_kiosk_app->id()));
@@ -211,10 +209,8 @@ TEST_F(FileSystemApiConsentProviderTest, ForKioskApps) {
   // receiving approval from the user.
   scoped_refptr<Extension> manual_launch_kiosk_app(
       ExtensionBuilder("Test", ExtensionBuilder::Type::PLATFORM_APP)
-          .MergeManifest(DictionaryBuilder()
-                             .Set("kiosk_enabled", true)
-                             .Set("kiosk_only", true)
-                             .Build())
+          .SetManifestKey("kiosk_enabled", true)
+          .SetManifestKey("kiosk_only", true)
           .Build());
   user_manager::User* const manual_kiosk_app_user =
       user_manager_->AddKioskAppUser(

diff --git a/chrome/browser/extensions/api/identity/identity_apitest.cc b/chrome/browser/extensions/api/identity/identity_apitest.cc
@@ -1955,14 +1955,11 @@ class GetAuthTokenFunctionPublicSessionTest : public GetAuthTokenFunctionTest {
 
   scoped_refptr<Extension> CreateTestExtension(const std::string& id) {
     return ExtensionBuilder("Test")
-        .MergeManifest(
-            DictionaryBuilder()
-                .Set("oauth2",
-                     DictionaryBuilder()
-                         .Set("client_id", "clientId")
-                         .Set("scopes", ListBuilder().Append("scope1").Build())
-                         .Build())
-                .Build())
+        .SetManifestKey(
+            "oauth2", DictionaryBuilder()
+                          .Set("client_id", "clientId")
+                          .Set("scopes", ListBuilder().Append("scope1").Build())
+                          .Build())
         .SetID(id)
         .Build();
   }

diff --git a/chrome/browser/extensions/extension_garbage_collector_chromeos_unittest.cc b/chrome/browser/extensions/extension_garbage_collector_chromeos_unittest.cc
@@ -121,8 +121,7 @@ class ExtensionGarbageCollectorChromeOSUnitTest
                                            const std::string& version,
                                            const base::FilePath& path) {
     return ExtensionBuilder("test")
-        .MergeManifest(
-            DictionaryBuilder().Set(manifest_keys::kVersion, version).Build())
+        .SetManifestKey(manifest_keys::kVersion, version)
         .SetID(id)
         .SetPath(path)
         .SetLocation(Manifest::INTERNAL)

diff --git a/chrome/browser/extensions/extension_protocols_unittest.cc b/chrome/browser/extensions/extension_protocols_unittest.cc
@@ -122,11 +122,9 @@ scoped_refptr<Extension> CreateWebStoreExtension() {
 }
 
 scoped_refptr<Extension> CreateTestResponseHeaderExtension() {
-  DictionaryBuilder web_accessible_resources;
-  web_accessible_resources.Set("web_accessible_resources",
-                               ListBuilder().Append("test.dat").Build());
   return ExtensionBuilder("An extension with web-accessible resources")
-      .MergeManifest(web_accessible_resources.Build())
+      .SetManifestKey("web_accessible_resources",
+                      ListBuilder().Append("test.dat").Build())
       .SetPath(GetTestPath("response_headers"))
       .Build();
 }

diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
@@ -7215,16 +7215,10 @@ TEST_F(ExtensionServiceTest, CannotEnableBlacklistedExtension) {
 // Test that calls to disable Shared Modules do not work.
 TEST_F(ExtensionServiceTest, CannotDisableSharedModules) {
   InitializeEmptyExtensionService();
-  std::unique_ptr<base::DictionaryValue> export_dict =
-      extensions::DictionaryBuilder()
-          .Set("resources", extensions::ListBuilder().Append("foo.js").Build())
-          .Build();
-
   scoped_refptr<Extension> extension =
       ExtensionBuilder("Shared Module")
-          .MergeManifest(extensions::DictionaryBuilder()
-                             .Set("export", std::move(export_dict))
-                             .Build())
+          .SetManifestPath({"export", "resources"},
+                           extensions::ListBuilder().Append("foo.js").Build())
           .AddFlags(Extension::FROM_WEBSTORE)
           .Build();
 

diff --git a/chrome/browser/extensions/extension_web_ui_unittest.cc b/chrome/browser/extensions/extension_web_ui_unittest.cc
@@ -162,14 +162,9 @@ TEST_F(ExtensionWebUITest, ExtensionURLOverride) {
 TEST_F(ExtensionWebUITest, TestRemovingDuplicateEntriesForHosts) {
   // Test that duplicate entries for a single extension are removed. This could
   // happen because of https://crbug.com/782959.
-  std::unique_ptr<base::DictionaryValue> manifest_overrides =
-      DictionaryBuilder().Set("newtab", "newtab.html").Build();
   scoped_refptr<const Extension> extension =
       ExtensionBuilder("extension")
-          .MergeManifest(
-              DictionaryBuilder()
-                  .Set("chrome_url_overrides", std::move(manifest_overrides))
-                  .Build())
+          .SetManifestPath({"chrome_url_overrides", "newtab"}, "newtab.html")
           .Build();
 
   const GURL newtab_url = extension->GetResourceURL("newtab.html");

diff --git a/chrome/browser/extensions/install_verifier_unittest.cc b/chrome/browser/extensions/install_verifier_unittest.cc
@@ -97,10 +97,8 @@ TEST_F(InstallVerifierTest, TestIsFromStoreAndMustRemainDisabled) {
     ExtensionBuilder extension_builder(test_case.test_name);
     extension_builder.SetLocation(test_case.location);
     if (test_case.update_url) {
-      extension_builder.MergeManifest(
-          DictionaryBuilder()
-              .Set("update_url", test_case.update_url->spec())
-              .Build());
+      extension_builder.SetManifestKey("update_url",
+                                       test_case.update_url->spec());
     }
     scoped_refptr<const Extension> extension = extension_builder.Build();
 

diff --git a/chrome/browser/extensions/permission_messages_unittest.cc b/chrome/browser/extensions/permission_messages_unittest.cc
@@ -56,12 +56,9 @@ class PermissionMessagesUnittest : public testing::Test {
       std::unique_ptr<base::ListValue> required_permissions,
       std::unique_ptr<base::ListValue> optional_permissions) {
     app_ = ExtensionBuilder("Test")
-               .MergeManifest(
-                   DictionaryBuilder()
-                       .Set("permissions", std::move(required_permissions))
-                       .Set("optional_permissions",
-                            std::move(optional_permissions))
-                       .Build())
+               .SetManifestKey("permissions", std::move(required_permissions))
+               .SetManifestKey("optional_permissions",
+                               std::move(optional_permissions))
                .SetID(crx_file::id_util::GenerateId("extension"))
                .SetLocation(Manifest::INTERNAL)
                .Build();

diff --git a/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc b/chrome/browser/ui/views/extensions/extension_uninstall_dialog_view_browsertest.cc
@@ -308,10 +308,8 @@ class ExtensionUninstallDialogViewInteractiveBrowserTest
   void ShowUi(const std::string& name) override {
     extensions::ExtensionBuilder extension_builder("ExtensionForRemoval");
     if (extension_origin_ == EXTENSION_FROM_WEBSTORE) {
-      extensions::DictionaryBuilder update_url;
-      update_url.Set("update_url",
-                     extension_urls::GetWebstoreUpdateUrl().spec());
-      extension_builder.MergeManifest(update_url.Build());
+      extension_builder.SetManifestKey(
+          "update_url", extension_urls::GetWebstoreUpdateUrl().spec());
     }
 
     extension_ = extension_builder.Build();

diff --git a/extensions/browser/updater/update_service_unittest.cc b/extensions/browser/updater/update_service_unittest.cc
@@ -395,17 +395,11 @@ TEST_F(UpdateServiceTest, UninstallPings) {
 
   // Build 3 extensions.
   scoped_refptr<Extension> extension1 =
-      ExtensionBuilder("1")
-          .MergeManifest(DictionaryBuilder().Set("version", "1.2").Build())
-          .Build();
+      ExtensionBuilder("1").SetManifestKey("version", "1.2").Build();
   scoped_refptr<Extension> extension2 =
-      ExtensionBuilder("2")
-          .MergeManifest(DictionaryBuilder().Set("version", "2.3").Build())
-          .Build();
+      ExtensionBuilder("2").SetManifestKey("version", "2.3").Build();
   scoped_refptr<Extension> extension3 =
-      ExtensionBuilder("3")
-          .MergeManifest(DictionaryBuilder().Set("version", "3.4").Build())
-          .Build();
+      ExtensionBuilder("3").SetManifestKey("version", "3.4").Build();
   EXPECT_TRUE(extension1->id() != extension2->id() &&
               extension1->id() != extension3->id() &&
               extension2->id() != extension3->id());

diff --git a/extensions/common/extension_builder.cc b/extensions/common/extension_builder.cc
@@ -19,7 +19,7 @@ struct ExtensionBuilder::ManifestData {
   std::vector<std::string> permissions;
   base::Optional<ActionType> action;
   base::Optional<BackgroundPage> background_page;
-  std::unique_ptr<base::DictionaryValue> extra;
+  base::Optional<base::Value> extra;
 
   std::unique_ptr<base::DictionaryValue> GetValue() const {
     DictionaryBuilder manifest;
@@ -78,11 +78,20 @@ struct ExtensionBuilder::ManifestData {
     }
 
     std::unique_ptr<base::DictionaryValue> result = manifest.Build();
-    if (extra)
-      result->MergeDictionary(extra.get());
+    if (extra) {
+      const base::DictionaryValue* extra_dict = nullptr;
+      extra->GetAsDictionary(&extra_dict);
+      result->MergeDictionary(extra_dict);
+    }
 
     return result;
   }
+
+  base::Value* get_extra() {
+    if (!extra)
+      extra.emplace(base::Value::Type::DICTIONARY);
+    return &extra.value();
+  }
 };
 
 ExtensionBuilder::ExtensionBuilder()
@@ -167,9 +176,9 @@ ExtensionBuilder& ExtensionBuilder::SetManifest(
 ExtensionBuilder& ExtensionBuilder::MergeManifest(
     std::unique_ptr<base::DictionaryValue> manifest) {
   if (manifest_data_) {
-    if (!manifest_data_->extra)
-      manifest_data_->extra = std::make_unique<base::DictionaryValue>();
-    manifest_data_->extra->MergeDictionary(manifest.get());
+    base::DictionaryValue* extra_dict = nullptr;
+    manifest_data_->get_extra()->GetAsDictionary(&extra_dict);
+    extra_dict->MergeDictionary(manifest.get());
   } else {
     manifest_value_->MergeDictionary(manifest.get());
   }
@@ -186,4 +195,17 @@ ExtensionBuilder& ExtensionBuilder::SetID(const std::string& id) {
   return *this;
 }
 
+void ExtensionBuilder::SetManifestKeyImpl(base::StringPiece key,
+                                          base::Value value) {
+  CHECK(manifest_data_);
+  manifest_data_->get_extra()->SetKey(key, std::move(value));
+}
+
+void ExtensionBuilder::SetManifestPathImpl(
+    std::initializer_list<base::StringPiece> path,
+    base::Value value) {
+  CHECK(manifest_data_);
+  manifest_data_->get_extra()->SetPath(path, std::move(value));
+}
+
 }  // namespace extensions
diff --git a/extensions/common/extension_builder.h b/extensions/common/extension_builder.h
@@ -5,12 +5,14 @@
 #ifndef EXTENSIONS_COMMON_EXTENSION_BUILDER_H_
 #define EXTENSIONS_COMMON_EXTENSION_BUILDER_H_
 
+#include <initializer_list>
 #include <memory>
 #include <string>
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/strings/string_piece.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/value_builder.h"
 
@@ -87,6 +89,39 @@ class ExtensionBuilder {
   // page will be set.
   ExtensionBuilder& SetBackgroundPage(BackgroundPage background_page);
 
+  // Shortcuts to setting values on the manifest dictionary without needing to
+  // go all the way through MergeManifest(). Sample usage:
+  // ExtensionBuilder("name").SetManifestKey("version", "0.2").Build();
+  // Can be used in conjuction with ListBuilder and DictionaryBuilder for more
+  // complex types.
+  template <typename T>
+  ExtensionBuilder& SetManifestKey(base::StringPiece key, T value) {
+    SetManifestKeyImpl(key, base::Value(value));
+    return *this;
+  }
+  template <typename T>
+  ExtensionBuilder& SetManifestPath(
+      std::initializer_list<base::StringPiece> path,
+      T value) {
+    SetManifestPathImpl(path, base::Value(value));
+    return *this;
+  }
+  // Specializations for unique_ptr<> to allow passing unique_ptr<base::Value>.
+  // All other types will fail to compile.
+  template <typename T>
+  ExtensionBuilder& SetManifestKey(base::StringPiece key,
+                                   std::unique_ptr<T> value) {
+    SetManifestKeyImpl(key, std::move(*value));
+    return *this;
+  }
+  template <typename T>
+  ExtensionBuilder& SetManifestPath(
+      std::initializer_list<base::StringPiece> path,
+      std::unique_ptr<T> value) {
+    SetManifestPathImpl(path, std::move(*value));
+    return *this;
+  }
+
   //////////////////////////////////////////////////////////////////////////////
   // Utility methods for use with custom manifest construction.
 
@@ -120,6 +155,10 @@ class ExtensionBuilder {
  private:
   struct ManifestData;
 
+  void SetManifestKeyImpl(base::StringPiece key, base::Value value);
+  void SetManifestPathImpl(std::initializer_list<base::StringPiece> path,
+                           base::Value value);
+
   // Information for constructing the manifest; either metadata about the
   // manifest which will be used to construct it, or the dictionary itself. Only
   // one will be present.

diff --git a/extensions/common/extension_builder_unittest.cc b/extensions/common/extension_builder_unittest.cc
@@ -167,4 +167,12 @@ TEST(ExtensionBuilderTest, MergeManifestOverridesValues) {
   }
 }
 
+TEST(ExtensionBuilderTest, SetManifestKey) {
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("foo")
+          .SetManifestKey("short_name", "short name")
+          .Build();
+  EXPECT_EQ("short name", extension->short_name());
+}
+
 }  // namespace extensions
diff --git a/extensions/renderer/runtime_hooks_delegate_unittest.cc b/extensions/renderer/runtime_hooks_delegate_unittest.cc
@@ -104,15 +104,10 @@ TEST_F(RuntimeHooksDelegateTest, RuntimeId) {
   v8::Local<v8::Context> context = MainContext();
 
   {
-    DictionaryBuilder connectable;
-    connectable.Set("matches",
-                    ListBuilder().Append("*://example.com/*").Build());
     scoped_refptr<Extension> connectable_extension =
         ExtensionBuilder("connectable")
-            .MergeManifest(
-                DictionaryBuilder()
-                    .Set("externally_connectable", connectable.Build())
-                    .Build())
+            .SetManifestPath({"externally_connectable", "matches"},
+                             ListBuilder().Append("*://example.com/*").Build())
             .Build();
     RegisterExtension(connectable_extension);
   }

diff --git a/extensions/shell/browser/api/identity/identity_api_unittest.cc b/extensions/shell/browser/api/identity/identity_api_unittest.cc
@@ -68,11 +68,9 @@ class IdentityApiTest : public ApiUnitTest {
                            .Append("https://www.googleapis.com/auth/drive")
                            .Build());
     // Create an extension with OAuth2 scopes.
-    set_extension(
-        ExtensionBuilder("Test")
-            .MergeManifest(
-                DictionaryBuilder().Set("oauth2", oauth2.Build()).Build())
-            .Build());
+    set_extension(ExtensionBuilder("Test")
+                      .SetManifestKey("oauth2", oauth2.Build())
+                      .Build());
   }
 };
 

diff --git a/extensions/shell/browser/system_logs/shell_system_logs_fetcher_unittest.cc b/extensions/shell/browser/system_logs/shell_system_logs_fetcher_unittest.cc
@@ -34,7 +34,7 @@ class ShellSystemLogsFetcherTest : public ExtensionsTest {
                                           const std::string& version,
                                           const std::string& id) {
     return ExtensionBuilder(name)
-        .MergeManifest(DictionaryBuilder().Set("version", version).Build())
+        .SetManifestKey("version", version)
         .SetID(id)
         .Build();
   }