Make extensions::DictionaryBuilder and extensions::ListValue unmovable.

There's no reason for these classes to be movable. std::move() is just
being used as a synonym for Build().

In addition:
- Build() is fewer characters than std::move().
- clang-format works better when builder syntax is consistently used,
  which makes it easier for readers to visually match up deeply nested
  builders.
- It's surprising to see std::move() used with what looks like a
  temporary.

BUG=none

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

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

9 files changed, 75 insertions, 116 deletions

diff --git a/extensions/common/extension_builder.cc b/extensions/common/extension_builder.cc
index 9e41166..905ba6b 100644
--- a/extensions/common/extension_builder.cc
+++ b/extensions/common/extension_builder.cc
@@ -61,8 +61,9 @@ ExtensionBuilder& ExtensionBuilder::SetManifest(
   return *this;
 }
 
-ExtensionBuilder& ExtensionBuilder::MergeManifest(DictionaryBuilder& builder) {
-  manifest_->MergeDictionary(builder.Build().get());
+ExtensionBuilder& ExtensionBuilder::MergeManifest(
+    scoped_ptr<base::DictionaryValue> manifest) {
+  manifest_->MergeDictionary(manifest.get());
   return *this;
 }
 
diff --git a/extensions/common/extension_builder.h b/extensions/common/extension_builder.h
index 4734baa..5fe9801 100644
--- a/extensions/common/extension_builder.h
+++ b/extensions/common/extension_builder.h
@@ -40,13 +40,10 @@ class ExtensionBuilder {
   ExtensionBuilder& SetLocation(Manifest::Location location);
 
   ExtensionBuilder& SetManifest(scoped_ptr<base::DictionaryValue> manifest);
-  ExtensionBuilder& SetManifest(DictionaryBuilder manifest_builder) {
-    return SetManifest(manifest_builder.Build());
-  }
 
-  // Adds the keys from the DictionaryBuilder to the manifest, with new keys
-  // taking precedence.
-  ExtensionBuilder& MergeManifest(DictionaryBuilder& builder);
+  // Merge another manifest into the current manifest, with new keys taking
+  // precedence.
+  ExtensionBuilder& MergeManifest(scoped_ptr<base::DictionaryValue> manifest);
 
   ExtensionBuilder& AddFlags(int init_from_value_flags);
 
diff --git a/extensions/common/features/base_feature_provider_unittest.cc b/extensions/common/features/base_feature_provider_unittest.cc
index 43e0ce2..e601cfa5 100644
--- a/extensions/common/features/base_feature_provider_unittest.cc
+++ b/extensions/common/features/base_feature_provider_unittest.cc
@@ -46,10 +46,11 @@ TEST(BaseFeatureProviderTest, ManifestFeatureAvailability) {
 
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
-          .SetManifest(std::move(DictionaryBuilder()
-                                     .Set("name", "test extension")
-                                     .Set("version", "1")
-                                     .Set("description", "hello there")))
+          .SetManifest(DictionaryBuilder()
+                           .Set("name", "test extension")
+                           .Set("version", "1")
+                           .Set("description", "hello there")
+                           .Build())
           .Build();
   ASSERT_TRUE(extension.get());
 
@@ -100,17 +101,21 @@ TEST(BaseFeatureProviderTest, PermissionFeatureAvailability) {
 
   scoped_refptr<const Extension> app =
       ExtensionBuilder()
-          .SetManifest(std::move(
+          .SetManifest(
               DictionaryBuilder()
                   .Set("name", "test app")
                   .Set("version", "1")
-                  .Set("app", std::move(DictionaryBuilder().Set(
-                                  "background",
-                                  std::move(DictionaryBuilder().Set(
-                                      "scripts", std::move(ListBuilder().Append(
-                                                     "background.js")))))))
-                  .Set("permissions",
-                       std::move(ListBuilder().Append("power")))))
+                  .Set("app",
+                       DictionaryBuilder()
+                           .Set("background",
+                                DictionaryBuilder()
+                                    .Set("scripts", ListBuilder()
+                                                        .Append("background.js")
+                                                        .Build())
+                                    .Build())
+                           .Build())
+                  .Set("permissions", ListBuilder().Append("power").Build())
+                  .Build())
           .Build();
   ASSERT_TRUE(app.get());
   ASSERT_TRUE(app->is_platform_app());
diff --git a/extensions/common/features/complex_feature_unittest.cc b/extensions/common/features/complex_feature_unittest.cc
index 0dd7c8f..f60c900 100644
--- a/extensions/common/features/complex_feature_unittest.cc
+++ b/extensions/common/features/complex_feature_unittest.cc
@@ -24,8 +24,8 @@ TEST(ComplexFeatureTest, MultipleRulesWhitelist) {
   scoped_ptr<SimpleFeature> simple_feature(new SimpleFeature);
   scoped_ptr<base::DictionaryValue> rule(
       DictionaryBuilder()
-          .Set("whitelist", std::move(ListBuilder().Append(kIdFoo)))
-          .Set("extension_types", std::move(ListBuilder().Append("extension")))
+          .Set("whitelist", ListBuilder().Append(kIdFoo).Build())
+          .Set("extension_types", ListBuilder().Append("extension").Build())
           .Build());
   simple_feature->Parse(rule.get());
   features->push_back(std::move(simple_feature));
@@ -33,9 +33,9 @@ TEST(ComplexFeatureTest, MultipleRulesWhitelist) {
   // Rule: "legacy_packaged_app", whitelist "bar".
   simple_feature.reset(new SimpleFeature);
   rule = DictionaryBuilder()
-             .Set("whitelist", std::move(ListBuilder().Append(kIdBar)))
+             .Set("whitelist", ListBuilder().Append(kIdBar).Build())
              .Set("extension_types",
-                  std::move(ListBuilder().Append("legacy_packaged_app")))
+                  ListBuilder().Append("legacy_packaged_app").Build())
              .Build();
   simple_feature->Parse(rule.get());
   features->push_back(std::move(simple_feature));
@@ -86,8 +86,8 @@ TEST(ComplexFeatureTest, Dependencies) {
   scoped_ptr<SimpleFeature> simple_feature(new SimpleFeature);
   scoped_ptr<base::DictionaryValue> rule =
       DictionaryBuilder()
-          .Set("dependencies", std::move(ListBuilder().Append(
-                                   "manifest:content_security_policy")))
+          .Set("dependencies",
+               ListBuilder().Append("manifest:content_security_policy").Build())
           .Build();
   simple_feature->Parse(rule.get());
   features->push_back(std::move(simple_feature));
@@ -96,7 +96,7 @@ TEST(ComplexFeatureTest, Dependencies) {
   simple_feature.reset(new SimpleFeature);
   rule = DictionaryBuilder()
              .Set("dependencies",
-                  std::move(ListBuilder().Append("permission:serial")))
+                  ListBuilder().Append("permission:serial").Build())
              .Build();
   simple_feature->Parse(rule.get());
   features->push_back(std::move(simple_feature));
diff --git a/extensions/common/manifest_handler_unittest.cc b/extensions/common/manifest_handler_unittest.cc
index 3e01d49..cbccf12 100644
--- a/extensions/common/manifest_handler_unittest.cc
+++ b/extensions/common/manifest_handler_unittest.cc
@@ -186,17 +186,19 @@ TEST_F(ManifestHandlerTest, DependentHandlers) {
 
   scoped_refptr<Extension> extension =
       ExtensionBuilder()
-          .SetManifest(std::move(
-              DictionaryBuilder()
-                  .Set("name", "no name")
-                  .Set("version", "0")
-                  .Set("manifest_version", 2)
-                  .Set("a", 1)
-                  .Set("b", 2)
-                  .Set("c", std::move(
-                                DictionaryBuilder().Set("d", 3).Set("e", 4).Set(
-                                    "f", 5)))
-                  .Set("g", 6)))
+          .SetManifest(DictionaryBuilder()
+                           .Set("name", "no name")
+                           .Set("version", "0")
+                           .Set("manifest_version", 2)
+                           .Set("a", 1)
+                           .Set("b", 2)
+                           .Set("c", DictionaryBuilder()
+                                         .Set("d", 3)
+                                         .Set("e", 4)
+                                         .Set("f", 5)
+                                         .Build())
+                           .Set("g", 6)
+                           .Build())
           .Build();
 
   // A, B, C.EZ, C.D, K
@@ -248,12 +250,13 @@ TEST_F(ManifestHandlerTest, Validate) {
   ScopedTestingManifestHandlerRegistry registry;
   scoped_refptr<Extension> extension =
       ExtensionBuilder()
-          .SetManifest(std::move(DictionaryBuilder()
-                                     .Set("name", "no name")
-                                     .Set("version", "0")
-                                     .Set("manifest_version", 2)
-                                     .Set("a", 1)
-                                     .Set("b", 2)))
+          .SetManifest(DictionaryBuilder()
+                           .Set("name", "no name")
+                           .Set("version", "0")
+                           .Set("manifest_version", 2)
+                           .Set("a", 1)
+                           .Set("b", 2)
+                           .Build())
           .Build();
   EXPECT_TRUE(extension.get());
 
diff --git a/extensions/common/test_util.cc b/extensions/common/test_util.cc
index e3775e5..c065c29 100644
--- a/extensions/common/test_util.cc
+++ b/extensions/common/test_util.cc
@@ -14,39 +14,43 @@ namespace extensions {
 namespace test_util {
 
 ExtensionBuilder BuildExtension(ExtensionBuilder builder) {
-  builder.SetManifest(std::move(DictionaryBuilder()
-                                    .Set("name", "Test extension")
-                                    .Set("version", "1.0")
-                                    .Set("manifest_version", 2)));
+  builder.SetManifest(DictionaryBuilder()
+                          .Set("name", "Test extension")
+                          .Set("version", "1.0")
+                          .Set("manifest_version", 2)
+                          .Build());
   return builder;
 }
 
 ExtensionBuilder BuildApp(ExtensionBuilder builder) {
-  builder.SetManifest(std::move(
+  builder.SetManifest(
       DictionaryBuilder()
           .Set("name", "Test extension")
           .Set("version", "1.0")
           .Set("manifest_version", 2)
-          .Set("app",
-               std::move(extensions::DictionaryBuilder().Set(
-                   "background",
-                   std::move(extensions::DictionaryBuilder().Set(
-                       "scripts", std::move(extensions::ListBuilder().Append(
-                                      "background.js")))))))));
+          .Set("app", extensions::DictionaryBuilder()
+                          .Set("background",
+                               extensions::DictionaryBuilder()
+                                   .Set("scripts", extensions::ListBuilder()
+                                                       .Append("background.js")
+                                                       .Build())
+                                   .Build())
+                          .Build())
+          .Build());
   return builder;
 }
 
 scoped_refptr<Extension> CreateEmptyExtension() {
   return ExtensionBuilder()
-      .SetManifest(std::move(
-          DictionaryBuilder().Set("name", "Test").Set("version", "1.0")))
+      .SetManifest(
+          DictionaryBuilder().Set("name", "Test").Set("version", "1.0").Build())
       .Build();
 }
 
 scoped_refptr<Extension> CreateEmptyExtension(const std::string& id) {
   return ExtensionBuilder()
-      .SetManifest(std::move(
-          DictionaryBuilder().Set("name", "test").Set("version", "0.1")))
+      .SetManifest(
+          DictionaryBuilder().Set("name", "test").Set("version", "0.1").Build())
       .SetID(id)
       .Build();
 }
diff --git a/extensions/common/value_builder.cc b/extensions/common/value_builder.cc
index 987bbb4..efc2e44 100644
--- a/extensions/common/value_builder.cc
+++ b/extensions/common/value_builder.cc
@@ -20,14 +20,6 @@ DictionaryBuilder::DictionaryBuilder(const base::DictionaryValue& init)
 
 DictionaryBuilder::~DictionaryBuilder() {}
 
-DictionaryBuilder::DictionaryBuilder(DictionaryBuilder&& other)
-    : dict_(other.Build()) {}
-
-DictionaryBuilder& DictionaryBuilder::operator=(DictionaryBuilder&& other) {
-  dict_ = other.Build();
-  return *this;
-}
-
 std::string DictionaryBuilder::ToJSON() const {
   std::string json;
   base::JSONWriter::WriteWithOptions(
@@ -60,18 +52,6 @@ DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
 }
 
 DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
-                                          DictionaryBuilder in_value) {
-  dict_->SetWithoutPathExpansion(path, in_value.Build());
-  return *this;
-}
-
-DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
-                                          ListBuilder in_value) {
-  dict_->SetWithoutPathExpansion(path, in_value.Build());
-  return *this;
-}
-
-DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
                                           scoped_ptr<base::Value> in_value) {
   dict_->SetWithoutPathExpansion(path, std::move(in_value));
   return *this;
@@ -90,14 +70,6 @@ ListBuilder::ListBuilder(const base::ListValue& init) : list_(init.DeepCopy()) {
 }
 ListBuilder::~ListBuilder() {}
 
-ListBuilder::ListBuilder(ListBuilder&& other)
-    : list_(other.Build()) {}
-
-ListBuilder& ListBuilder::operator=(ListBuilder&& other) {
-  list_ = other.Build();
-  return *this;
-}
-
 ListBuilder& ListBuilder::Append(int in_value) {
   list_->Append(new base::FundamentalValue(in_value));
   return *this;
@@ -118,13 +90,8 @@ ListBuilder& ListBuilder::Append(const base::string16& in_value) {
   return *this;
 }
 
-ListBuilder& ListBuilder::Append(DictionaryBuilder in_value) {
-  list_->Append(in_value.Build());
-  return *this;
-}
-
-ListBuilder& ListBuilder::Append(ListBuilder in_value) {
-  list_->Append(in_value.Build());
+ListBuilder& ListBuilder::Append(scoped_ptr<base::Value> in_value) {
+  list_->Append(std::move(in_value));
   return *this;
 }
 
diff --git a/extensions/common/value_builder.h b/extensions/common/value_builder.h
index d746904..27c4e01 100644
--- a/extensions/common/value_builder.h
+++ b/extensions/common/value_builder.h
@@ -6,19 +6,13 @@
 // aren't specific to extensions and could move up to base/ if there's interest
 // from other sub-projects.
 //
-// The general pattern is to write:
+// The pattern is to write:
 //
 //  scoped_ptr<BuiltType> result(FooBuilder()
 //                               .Set(args)
 //                               .Set(args)
 //                               .Build());
 //
-// For methods that take other built types, you can pass the builder directly
-// to the setter without calling Build():
-//
-// DictionaryBuilder().Set("key", std::move(ListBuilder()
-//                         .Append("foo").Append("bar")) /* No .Build() */);
-//
 // The Build() method invalidates its builder, and returns ownership of the
 // built value.
 //
@@ -52,10 +46,6 @@ class DictionaryBuilder {
   explicit DictionaryBuilder(const base::DictionaryValue& init);
   ~DictionaryBuilder();
 
-  // Move constructor and operator=.
-  DictionaryBuilder(DictionaryBuilder&& other);
-  DictionaryBuilder& operator=(DictionaryBuilder&& other);
-
   // Can only be called once, after which it's invalid to use the builder.
   scoped_ptr<base::DictionaryValue> Build() { return std::move(dict_); }
 
@@ -68,8 +58,6 @@ class DictionaryBuilder {
   DictionaryBuilder& Set(const std::string& path, const std::string& in_value);
   DictionaryBuilder& Set(const std::string& path,
                          const base::string16& in_value);
-  DictionaryBuilder& Set(const std::string& path, DictionaryBuilder in_value);
-  DictionaryBuilder& Set(const std::string& path, ListBuilder in_value);
   DictionaryBuilder& Set(const std::string& path,
                          scoped_ptr<base::Value> in_value);
 
@@ -87,10 +75,6 @@ class ListBuilder {
   explicit ListBuilder(const base::ListValue& init);
   ~ListBuilder();
 
-  // Move constructor and operator=.
-  ListBuilder(ListBuilder&& other);
-  ListBuilder& operator=(ListBuilder&& other);
-
   // Can only be called once, after which it's invalid to use the builder.
   scoped_ptr<base::ListValue> Build() { return std::move(list_); }
 
@@ -98,8 +82,7 @@ class ListBuilder {
   ListBuilder& Append(double in_value);
   ListBuilder& Append(const std::string& in_value);
   ListBuilder& Append(const base::string16& in_value);
-  ListBuilder& Append(DictionaryBuilder in_value);
-  ListBuilder& Append(ListBuilder in_value);
+  ListBuilder& Append(scoped_ptr<base::Value> in_value);
 
   // Named differently because overload resolution is too eager to
   // convert implicitly to bool.
diff --git a/extensions/common/value_builder_unittest.cc b/extensions/common/value_builder_unittest.cc
index 7c47451..1f1c14e 100644
--- a/extensions/common/value_builder_unittest.cc
+++ b/extensions/common/value_builder_unittest.cc
@@ -20,9 +20,8 @@ TEST(ValueBuilderTest, Basic) {
   scoped_ptr<base::DictionaryValue> settings(new base::DictionaryValue);
 
   ASSERT_FALSE(settings->GetList("permissions", nullptr));
-  settings = DictionaryBuilder()
-                 .Set("permissions", std::move(permission_list))
-                 .Build();
+  settings =
+      DictionaryBuilder().Set("permissions", permission_list.Build()).Build();
   base::ListValue* list_value;
   ASSERT_TRUE(settings->GetList("permissions", &list_value));