Use rvalue reference instead of extensions::ListBuilder::Pass()

C++ 11 enables rvalue reference with std::move() so that removing legacy ListBuilder::Pass() stuff.

TEST=unit_tests
BUG=563649
TBR=thakis@chromium.org

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

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

6 files changed, 95 insertions, 38 deletions

diff --git a/extensions/common/features/base_feature_provider_unittest.cc b/extensions/common/features/base_feature_provider_unittest.cc
index dc41366..30850a3 100644
--- a/extensions/common/features/base_feature_provider_unittest.cc
+++ b/extensions/common/features/base_feature_provider_unittest.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <set>
 #include <string>
+#include <utility>
 
 #include "base/stl_util.h"
 #include "extensions/common/extension_builder.h"
@@ -99,16 +100,16 @@ TEST(BaseFeatureProviderTest, PermissionFeatureAvailability) {
 
   scoped_refptr<const Extension> app =
       ExtensionBuilder()
-          .SetManifest(DictionaryBuilder()
-                           .Set("name", "test app")
-                           .Set("version", "1")
-                           .Set("app",
-                                DictionaryBuilder().Set(
-                                    "background",
-                                    DictionaryBuilder().Set(
-                                        "scripts",
-                                        ListBuilder().Append("background.js"))))
-                           .Set("permissions", ListBuilder().Append("power")))
+          .SetManifest(
+              DictionaryBuilder()
+                  .Set("name", "test app")
+                  .Set("version", "1")
+                  .Set("app", DictionaryBuilder().Set(
+                                  "background",
+                                  DictionaryBuilder().Set(
+                                      "scripts", std::move(ListBuilder().Append(
+                                                     "background.js")))))
+                  .Set("permissions", std::move(ListBuilder().Append("power"))))
           .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 54355aa..d06a15a 100644
--- a/extensions/common/features/complex_feature_unittest.cc
+++ b/extensions/common/features/complex_feature_unittest.cc
@@ -5,6 +5,7 @@
 #include "extensions/common/features/complex_feature.h"
 
 #include <string>
+#include <utility>
 
 #include "extensions/common/features/simple_feature.h"
 #include "extensions/common/manifest.h"
@@ -23,18 +24,19 @@ TEST(ComplexFeatureTest, MultipleRulesWhitelist) {
   scoped_ptr<SimpleFeature> simple_feature(new SimpleFeature);
   scoped_ptr<base::DictionaryValue> rule(
       DictionaryBuilder()
-      .Set("whitelist", ListBuilder().Append(kIdFoo))
-      .Set("extension_types", ListBuilder()
-          .Append("extension")).Build());
+          .Set("whitelist", std::move(ListBuilder().Append(kIdFoo)))
+          .Set("extension_types", std::move(ListBuilder().Append("extension")))
+          .Build());
   simple_feature->Parse(rule.get());
   features->push_back(simple_feature.Pass());
 
   // Rule: "legacy_packaged_app", whitelist "bar".
   simple_feature.reset(new SimpleFeature);
   rule = DictionaryBuilder()
-      .Set("whitelist", ListBuilder().Append(kIdBar))
-      .Set("extension_types", ListBuilder()
-          .Append("legacy_packaged_app")).Build();
+             .Set("whitelist", std::move(ListBuilder().Append(kIdBar)))
+             .Set("extension_types",
+                  std::move(ListBuilder().Append("legacy_packaged_app")))
+             .Build();
   simple_feature->Parse(rule.get());
   features->push_back(simple_feature.Pass());
 
@@ -84,8 +86,8 @@ TEST(ComplexFeatureTest, Dependencies) {
   scoped_ptr<SimpleFeature> simple_feature(new SimpleFeature);
   scoped_ptr<base::DictionaryValue> rule =
       DictionaryBuilder()
-          .Set("dependencies",
-               ListBuilder().Append("manifest:content_security_policy"))
+          .Set("dependencies", std::move(ListBuilder().Append(
+                                   "manifest:content_security_policy")))
           .Build();
   simple_feature->Parse(rule.get());
   features->push_back(simple_feature.Pass());
@@ -93,7 +95,8 @@ TEST(ComplexFeatureTest, Dependencies) {
   // Rule which depends on an platform-app-only feature (serial).
   simple_feature.reset(new SimpleFeature);
   rule = DictionaryBuilder()
-             .Set("dependencies", ListBuilder().Append("permission:serial"))
+             .Set("dependencies",
+                  std::move(ListBuilder().Append("permission:serial")))
              .Build();
   simple_feature->Parse(rule.get());
   features->push_back(simple_feature.Pass());
diff --git a/extensions/common/test_util.cc b/extensions/common/test_util.cc
index dafbca2..9a9312a 100644
--- a/extensions/common/test_util.cc
+++ b/extensions/common/test_util.cc
@@ -4,6 +4,8 @@
 
 #include "extensions/common/test_util.h"
 
+#include <utility>
+
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/value_builder.h"
@@ -29,8 +31,8 @@ ExtensionBuilder& BuildApp(ExtensionBuilder& builder) {
                extensions::DictionaryBuilder().Set(
                    "background",
                    extensions::DictionaryBuilder().Set(
-                       "scripts",
-                       extensions::ListBuilder().Append("background.js")))));
+                       "scripts", std::move(extensions::ListBuilder().Append(
+                                      "background.js"))))));
 }
 
 scoped_refptr<Extension> CreateEmptyExtension() {
diff --git a/extensions/common/value_builder.cc b/extensions/common/value_builder.cc
index a69d533..6e73f36 100644
--- a/extensions/common/value_builder.cc
+++ b/extensions/common/value_builder.cc
@@ -5,6 +5,7 @@
 #include "extensions/common/value_builder.h"
 
 #include "base/json/json_writer.h"
+#include "base/values.h"
 
 namespace extensions {
 
@@ -55,7 +56,7 @@ DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
 }
 
 DictionaryBuilder& DictionaryBuilder::Set(const std::string& path,
-                                          ListBuilder& in_value) {
+                                          ListBuilder in_value) {
   dict_->SetWithoutPathExpansion(path, in_value.Build().Pass());
   return *this;
 }
@@ -79,6 +80,14 @@ ListBuilder::ListBuilder(const base::ListValue& init) : list_(init.DeepCopy()) {
 }
 ListBuilder::~ListBuilder() {}
 
+ListBuilder::ListBuilder(ListBuilder&& other)
+    : list_(other.Build().release()) {}
+
+ListBuilder& ListBuilder::operator=(ListBuilder&& other) {
+  list_.reset(other.Build().release());
+  return *this;
+}
+
 ListBuilder& ListBuilder::Append(int in_value) {
   list_->Append(new base::FundamentalValue(in_value));
   return *this;
@@ -100,12 +109,12 @@ ListBuilder& ListBuilder::Append(const base::string16& in_value) {
 }
 
 ListBuilder& ListBuilder::Append(DictionaryBuilder& in_value) {
-  list_->Append(in_value.Build().Pass());
+  list_->Append(in_value.Build());
   return *this;
 }
 
-ListBuilder& ListBuilder::Append(ListBuilder& in_value) {
-  list_->Append(in_value.Build().Pass());
+ListBuilder& ListBuilder::Append(ListBuilder in_value) {
+  list_->Append(in_value.Build());
   return *this;
 }
 
diff --git a/extensions/common/value_builder.h b/extensions/common/value_builder.h
index 4de94b6..cc706fa 100644
--- a/extensions/common/value_builder.h
+++ b/extensions/common/value_builder.h
@@ -16,12 +16,8 @@
 // For methods that take other built types, you can pass the builder directly
 // to the setter without calling Build():
 //
-// DictionaryBuilder().Set("key", ListBuilder()
-//                         .Append("foo").Append("bar") /* No .Build() */);
-//
-// Because of limitations in C++03, and to avoid extra copies, you can't pass a
-// just-constructed Builder into another Builder's method directly. Use the
-// Pass() method.
+// 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.
@@ -35,9 +31,15 @@
 
 #include <string>
 
+#include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
-#include "base/values.h"
+
+namespace base {
+class DictionaryValue;
+class ListValue;
+class Value;
+}
 
 namespace extensions {
 
@@ -51,6 +53,7 @@ class DictionaryBuilder {
 
   // Workaround to allow you to pass rvalue ExtensionBuilders by reference to
   // other functions.
+  // TODO(limasdf): Remove. Write move constructor instead.
   DictionaryBuilder& Pass() { return *this; }
 
   // Can only be called once, after which it's invalid to use the builder.
@@ -66,7 +69,7 @@ class DictionaryBuilder {
   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, ListBuilder in_value);
   DictionaryBuilder& Set(const std::string& path,
                          scoped_ptr<base::Value> in_value);
 
@@ -84,19 +87,19 @@ class ListBuilder {
   explicit ListBuilder(const base::ListValue& init);
   ~ListBuilder();
 
-  // Workaround to allow you to pass rvalue ExtensionBuilders by reference to
-  // other functions.
-  ListBuilder& Pass() { return *this; }
+  // 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 list_.Pass(); }
+  scoped_ptr<base::ListValue> Build() { return std::move(list_); }
 
   ListBuilder& Append(int in_value);
   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(ListBuilder in_value);
 
   // Named differently because overload resolution is too eager to
   // convert implicitly to bool.
@@ -104,6 +107,8 @@ class ListBuilder {
 
  private:
   scoped_ptr<base::ListValue> list_;
+
+  DISALLOW_COPY_AND_ASSIGN(ListBuilder);
 };
 
 }  // namespace extensions
diff --git a/extensions/common/value_builder_unittest.cc b/extensions/common/value_builder_unittest.cc
new file mode 100644
index 0000000..7c47451
--- /dev/null
+++ b/extensions/common/value_builder_unittest.cc
@@ -0,0 +1,37 @@
+// Copyright 2015 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 <utility>
+
+#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
+#include "extensions/common/value_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ValueBuilderTest = testing::Test;
+
+namespace extensions {
+
+TEST(ValueBuilderTest, Basic) {
+  ListBuilder permission_list;
+  permission_list.Append("tabs").Append("history");
+
+  scoped_ptr<base::DictionaryValue> settings(new base::DictionaryValue);
+
+  ASSERT_FALSE(settings->GetList("permissions", nullptr));
+  settings = DictionaryBuilder()
+                 .Set("permissions", std::move(permission_list))
+                 .Build();
+  base::ListValue* list_value;
+  ASSERT_TRUE(settings->GetList("permissions", &list_value));
+
+  ASSERT_EQ(2U, list_value->GetSize());
+  std::string permission;
+  ASSERT_TRUE(list_value->GetString(0, &permission));
+  ASSERT_EQ(permission, "tabs");
+  ASSERT_TRUE(list_value->GetString(1, &permission));
+  ASSERT_EQ(permission, "history");
+}
+
+}  // namespace extensions