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

base/values.h

 // Copyright (c) 2012 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.
 
 // This file specifies a recursive data storage class called Value intended for
 // storing settings and other persistable data.
 //
 // A Value represents something that can be stored in JSON or passed to/from
 // JavaScript. As such, it is NOT a generalized variant type, since only the
 // types supported by JavaScript/JSON are supported.
@@ skipping 31 matching lines @@
 using BinaryValue = Value;
 
 // The Value class is the base class for Values. A Value can be instantiated
 // via the Create*Value() factory methods, or by directly creating instances of
 // the subclasses.
 //
 // See the file-level comment above for more information.
 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,
     BOOLEAN,
     INTEGER,
     DOUBLE,
     STRING,
     BINARY,
     DICTIONARY,
     LIST
     // Note: Do not add more types. See the file-level comment above for why.
   };
 
   static std::unique_ptr<Value> CreateNullValue();
 
   // For situations where you want to keep ownership of your buffer, this
   // factory method creates a new BinaryValue by copying the contents of the
   // buffer that's passed in.
   // DEPRECATED, use MakeUnique<Value>(const std::vector<char>&) instead.
   // TODO(crbug.com/646113): Delete this and migrate callsites.
   static std::unique_ptr<BinaryValue> CreateWithCopiedBuffer(const char* buffer,
                                                              size_t size);
 
   Value(const Value& that);
   Value(Value&& that);

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

Given the crash in components/browser_sync/profile_sync_service.cc I was
considering adding |noexcept| here to allow |push_back| to move during a
reallocation. I saw you explored |noexcept| in the context of |flat_map|, but
decided against it in the end. Do you think it would make sense here?

[brettw, 2017/03/17 05:39:19]

Yeah, any move that we can write noexcept for we should do since it avoid a
bunch of work. I forget the problem I had with flat map, but it ended up with
some case that was really hard to make compile. If it compiles to write noexcept
here, we should definitely do it.

This applies for all the other move operations below.

   Value();  // A null value.
   explicit Value(Type type);
   explicit Value(bool in_bool);
   explicit Value(int in_int);
   explicit Value(double in_double);
 
   // Value(const char*) and Value(const char16*) are required despite
   // Value(const std::string&) and Value(const string16&) because otherwise the
   // compiler will choose the Value(bool) constructor for these arguments.
   // Value(std::string&&) allow for efficient move construction.
   // Value(StringPiece) exists due to many callsites passing StringPieces as
   // arguments.
   explicit Value(const char* in_string);
   explicit Value(const std::string& in_string);
   explicit Value(std::string&& in_string);
   explicit Value(const char16* in_string);
   explicit Value(const string16& in_string);
   explicit Value(StringPiece in_string);
 
   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);
 
   ~Value();
 
   // Returns the name for a given |type|.
   static const char* GetTypeName(Type type);
 
   // Returns the type of the value stored by the current Value object.
   // Each type will be implemented by only one subclass of Value, so it's
@@ skipping 260 matching lines @@
   static std::unique_ptr<ListValue> From(std::unique_ptr<Value> value);
 
   ListValue();
 
   // Clears the contents of this ListValue
   void Clear();
 
   // 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.
   // Returns true if successful, or false if the index was negative or
   // the value is a null pointer.
   bool Set(size_t index, Value* in_value);
   // Preferred version of the above. TODO(estade): remove the above.
   bool Set(size_t index, std::unique_ptr<Value> in_value);
 
   // Gets the Value at the given index.  Modifies |out_value| (and returns true)
@@ skipping 33 matching lines @@
   // if not found.
   bool Remove(const Value& value, size_t* index);
 
   // Removes the element at |iter|. If |out_value| is NULL, the value will be
   // deleted, otherwise ownership of the value is passed back to the caller.
   // Returns an iterator pointing to the location of the element that
   // followed the erased element.
   iterator Erase(iterator iter, std::unique_ptr<Value>* out_value);
 
   // Appends a Value to the end of the list.
   void Append(std::unique_ptr<Value> in_value);

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

Given how cumbersome and error-prone fixing
|chrome/browser/ui/webui/certificate_viewer_webui.cc| became, I was seriously
considering returning a raw pointer to the just inserted element here. However,
I did want to minimize API changes and this change would likely only be
temporary anyway, so I didn't do it yet. What do you think?

[brettw, 2017/03/15 22:14:14]

STL doesn't return an iterator from vector::push_back and it seems like we
should stick with an API that mirrors that when possible.

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

Acknowledged.

 #if !defined(OS_LINUX)
   // Deprecated version of the above. TODO(estade): remove.
   void Append(Value* in_value);
 #endif
 
   // Convenience forms of Append.
   void AppendBoolean(bool in_value);
   void AppendInteger(int in_value);
   void AppendDouble(double in_value);
   void AppendString(StringPiece in_value);
@@ skipping 70 matching lines @@
   return out << static_cast<const Value&>(value);
 }
 
 // Stream operator so that enum class Types can be used in log statements.
 BASE_EXPORT std::ostream& operator<<(std::ostream& out,
                                      const Value::Type& type);
 
 }  // namespace base
 
 #endif  // BASE_VALUES_H_