Change base::Value::ListStorage to std::vector<base::Value>

base/values.h

Index: base/values.h
diff --git a/base/values.h b/base/values.h
index d4b334c2a57d14ee7cc35e13b3919bc4e5072c63..0d2ac8afe88ff022f822d4831dd186ff2d1e72cc 100644
--- a/base/values.h
+++ b/base/values.h
@@ -49,7 +49,7 @@ using BinaryValue = Value;
 class BASE_EXPORT Value {
  public:
   using DictStorage = std::map<std::string, std::unique_ptr<Value>>;
-  using ListStorage = std::vector<std::unique_ptr<Value>>;
+  using ListStorage = std::vector<Value>;
 
   enum class Type {
     NONE = 0,
@@ -97,6 +97,9 @@ class BASE_EXPORT Value {
   explicit Value(const std::vector<char>& in_blob);
   explicit Value(std::vector<char>&& in_blob);
 
+  explicit Value(const ListStorage& in_list);

[Devlin, 2017/03/15 15:25:03]

Not my area, so no need to wait for my signoff, but it seems like doing this for
complex types (like List and Dictionary) could lead to some accidental expensive
copies.  Is there a reason to prefer this rather than just having the move
semantics and an explicit Copy() method?

[jdoerrie, 2017/03/16 19:01:56]

Currently this is only used within |Value::DeepCopy| where the semantics are
quite explicit. I do agree that having this public is currently not needed and
could be completely inlined. I will soon follow up with a post to the refactor
doc to start a discussion regarding the new public API of ListValue, and will
mention this point there. 

My initial motivation for including it was that it is mostly a convenience
function when callers already happen to have access to a
std::vector<base::Value> and want to turn it into a List Value. Marking it as
explicit should hopefully help to avoid accidental expensive copies. But I
certainly agree that this is better discussed in the doc.

+  explicit Value(ListStorage&& in_list);
+
   Value& operator=(const Value& that);
   Value& operator=(Value&& that);
 
@@ -377,9 +380,15 @@ class BASE_EXPORT ListValue : public Value {
   // Returns the number of Values in this list.
   size_t GetSize() const { return list_->size(); }
 
+  // Returns the capacity of storage for Values in this list.
+  size_t capacity() const { return list_->capacity(); }
+
   // Returns whether the list is empty.
   bool empty() const { return list_->empty(); }
 
+  // Reserves storage for at least |n| values.
+  void Reserve(size_t n);

[jdoerrie, 2017/03/15 10:10:41]

I tried to minimize API changes, but these were strictly necessary to not have
crashes in components/sync/driver/about_sync_util.cc.

+
   // Sets the list item at the given index to be the Value specified by
   // the value given.  If the index beyond the current end of the list, null
   // Values will be used to pad out the list.