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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

TBR=bajones@chromium.org, dbeam@chromium.org, stevenjb@chromium.org
BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2740143002
Cr-Commit-Position: refs/heads/master@{#463618}

base/json/json_writer.cc

@@ -128,7 +128,7 @@ bool JSONWriter::BuildJSONString(const Value& node, size_t depth) {
       bool result = node.GetAsList(&list);
       DCHECK(result);
       for (const auto& value : *list) {
-        if (omit_binary_values_ && value->GetType() == Value::Type::BINARY)
+        if (omit_binary_values_ && value.GetType() == Value::Type::BINARY)
           continue;
 
         if (first_value_has_been_output) {
@@ -137,7 +137,7 @@ bool JSONWriter::BuildJSONString(const Value& node, size_t depth) {
             json_string_->push_back(' ');
         }
 
-        if (!BuildJSONString(*value, depth))
+        if (!BuildJSONString(value, depth))
           result = false;
 
         first_value_has_been_output = true;

base/test/gtest_util.cc

@@ -85,7 +85,7 @@ bool ReadTestNamesFromFile(const FilePath& path,
   std::vector<base::TestIdentifier> result;
   for (base::ListValue::iterator i = tests->begin(); i != tests->end(); ++i) {
     base::DictionaryValue* test = nullptr;
-    if (!(*i)->GetAsDictionary(&test))
+    if (!i->GetAsDictionary(&test))
       return false;
 
     TestIdentifier test_data;

base/trace_event/trace_event_argument.cc

@@ -289,7 +289,7 @@ void TracedValue::SetBaseValueWithCopiedName(base::StringPiece name,
       value.GetAsList(&list_value);
       BeginArrayWithCopiedName(name);
       for (const auto& base_value : *list_value)
-        AppendBaseValue(*base_value);
+        AppendBaseValue(base_value);
       EndArray();
     } break;
   }
@@ -343,7 +343,7 @@ void TracedValue::AppendBaseValue(const base::Value& value) {
       value.GetAsList(&list_value);
       BeginArray();
       for (const auto& base_value : *list_value)
-        AppendBaseValue(*base_value);
+        AppendBaseValue(base_value);
       EndArray();
     } break;
   }
@@ -369,9 +369,11 @@ std::unique_ptr<base::Value> TracedValue::ToBaseValue() const {
           cur_dict = new_dict;
         } else {
           cur_list->Append(WrapUnique(new_dict));
+          // |new_dict| is invalidated at this point, so |cur_dict| needs to be
+          // reset.
+          cur_list->GetDictionary(cur_list->GetSize() - 1, &cur_dict);
           stack.push_back(cur_list);
           cur_list = nullptr;
-          cur_dict = new_dict;
         }
       } break;
 
@@ -396,7 +398,8 @@ std::unique_ptr<base::Value> TracedValue::ToBaseValue() const {
         } else {
           cur_list->Append(WrapUnique(new_list));
           stack.push_back(cur_list);
-          cur_list = new_list;
+          // |cur_list| is invalidated at this point, so it needs to be reset.
+          cur_list->GetList(cur_list->GetSize() - 1, &cur_list);
         }
       } break;
 

base/trace_event/trace_event_memory_overhead.cc

@@ -105,7 +105,7 @@ void TraceEventMemoryOverhead::AddValue(const Value& value) {
       value.GetAsList(&list_value);
       Add("ListValue", sizeof(ListValue));
       for (const auto& v : *list_value)
-        AddValue(*v);
+        AddValue(v);
     } break;
 
     default:

base/values.cc

@@ -35,7 +35,7 @@ std::unique_ptr<Value> CopyWithoutEmptyChildren(const Value& node);
 std::unique_ptr<ListValue> CopyListWithoutEmptyChildren(const ListValue& list) {
   std::unique_ptr<ListValue> copy;
   for (const auto& entry : list) {
-    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(*entry);
+    std::unique_ptr<Value> child_copy = CopyWithoutEmptyChildren(entry);
     if (child_copy) {
       if (!copy)
         copy.reset(new ListValue);
@@ -375,12 +375,7 @@ bool operator==(const Value& lhs, const Value& rhs) {
                                  std::tie(v.first, *v.second);
                         });
     case Value::Type::LIST:
-      if (lhs.list_->size() != rhs.list_->size())
-        return false;
-      return std::equal(
-          std::begin(*lhs.list_), std::end(*lhs.list_), std::begin(*rhs.list_),
-          [](const Value::ListStorage::value_type& u,
-             const Value::ListStorage::value_type& v) { return *u == *v; });
+      return *lhs.list_ == *rhs.list_;
   }
 
   NOTREACHED();
@@ -419,11 +414,7 @@ bool operator<(const Value& lhs, const Value& rhs) {
             return std::tie(u.first, *u.second) < std::tie(v.first, *v.second);
           });
     case Value::Type::LIST:
-      return std::lexicographical_compare(
-          std::begin(*lhs.list_), std::end(*lhs.list_), std::begin(*rhs.list_),
-          std::end(*rhs.list_),
-          [](const Value::ListStorage::value_type& u,
-             const Value::ListStorage::value_type& v) { return *u < *v; });
+      return *lhs.list_ < *rhs.list_;
   }
 
   NOTREACHED();
@@ -494,11 +485,10 @@ void Value::InternalCopyConstructFrom(const Value& that) {
     case Type::BINARY:
       binary_value_.Init(*that.binary_value_);
       return;
-    // DictStorage and ListStorage are move-only types due to the presence of
-    // unique_ptrs. This is why the explicit copy of every element is necessary
-    // here.
-    // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
-    // can be copied directly.
+    // DictStorage is a move-only type due to the presence of unique_ptrs. This
+    // is why the explicit copy of every element is necessary here.
+    // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+    // directly.
     case Type::DICTIONARY:
       dict_ptr_.Init(MakeUnique<DictStorage>());
       for (const auto& it : **that.dict_ptr_) {
@@ -508,10 +498,7 @@ void Value::InternalCopyConstructFrom(const Value& that) {
       }
       return;
     case Type::LIST:
-      list_.Init();
-      list_->reserve(that.list_->size());
-      for (const auto& it : *that.list_)
-        list_->push_back(MakeUnique<Value>(*it));
+      list_.Init(*that.list_);
       return;
   }
 }
@@ -561,16 +548,17 @@ void Value::InternalCopyAssignFromSameType(const Value& that) {
     case Type::BINARY:
       *binary_value_ = *that.binary_value_;
       return;
-    // DictStorage and ListStorage are move-only types due to the presence of
-    // unique_ptrs. This is why the explicit call to the copy constructor is
-    // necessary here.
-    // TODO(crbug.com/646113): Clean this up when DictStorage and ListStorage
-    // can be copied directly.
-    case Type::DICTIONARY:
-      *dict_ptr_ = std::move(*Value(that).dict_ptr_);
+    // DictStorage is a move-only type due to the presence of unique_ptrs. This
+    // is why the explicit call to the copy constructor is necessary here.
+    // TODO(crbug.com/646113): Clean this up when DictStorage can be copied
+    // directly.
+    case Type::DICTIONARY: {
+      Value copy = that;
+      *dict_ptr_ = std::move(*copy.dict_ptr_);
       return;
+    }
     case Type::LIST:
-      *list_ = std::move(*Value(that).list_);
+      *list_ = *that.list_;
       return;
   }
 }
@@ -1077,6 +1065,10 @@ void ListValue::Clear() {
   list_->clear();
 }
 
+void ListValue::Reserve(size_t n) {
+  list_->reserve(n);
+}
+
 bool ListValue::Set(size_t index, Value* in_value) {
   return Set(index, WrapUnique(in_value));
 }
@@ -1091,9 +1083,7 @@ bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
       Append(MakeUnique<Value>());
     Append(std::move(in_value));
   } else {
-    // TODO(dcheng): remove this DCHECK once the raw pointer version is removed?
-    DCHECK((*list_)[index] != in_value);
-    (*list_)[index] = std::move(in_value);
+    (*list_)[index] = std::move(*in_value);
   }
   return true;
 }
@@ -1103,7 +1093,7 @@ bool ListValue::Get(size_t index, const Value** out_value) const {
     return false;
 
   if (out_value)
-    *out_value = (*list_)[index].get();
+    *out_value = &(*list_)[index];
 
   return true;
 }
@@ -1213,36 +1203,35 @@ bool ListValue::Remove(size_t index, std::unique_ptr<Value>* out_value) {
     return false;
 
   if (out_value)
-    *out_value = std::move((*list_)[index]);
+    *out_value = MakeUnique<Value>(std::move((*list_)[index]));
 
   list_->erase(list_->begin() + index);
   return true;
 }
 
 bool ListValue::Remove(const Value& value, size_t* index) {
-  for (auto it = list_->begin(); it != list_->end(); ++it) {
-    if (**it == value) {
-      size_t previous_index = it - list_->begin();
-      list_->erase(it);
+  auto it = std::find(list_->begin(), list_->end(), value);
 
-      if (index)
-        *index = previous_index;
-      return true;
-    }
-  }
-  return false;
+  if (it == list_->end())
+    return false;
+
+  if (index)
+    *index = std::distance(list_->begin(), it);
+
+  list_->erase(it);
+  return true;
 }
 
 ListValue::iterator ListValue::Erase(iterator iter,
                                      std::unique_ptr<Value>* out_value) {
   if (out_value)
-    *out_value = std::move(*ListStorage::iterator(iter));
+    *out_value = MakeUnique<Value>(std::move(*iter));
 
   return list_->erase(iter);
 }
 
 void ListValue::Append(std::unique_ptr<Value> in_value) {
-  list_->push_back(std::move(in_value));
+  list_->push_back(std::move(*in_value));
 }
 
 #if !defined(OS_LINUX)
@@ -1288,11 +1277,10 @@ void ListValue::AppendStrings(const std::vector<string16>& in_values) {
 
 bool ListValue::AppendIfNotPresent(std::unique_ptr<Value> in_value) {
   DCHECK(in_value);
-  for (const auto& entry : *list_) {
-    if (*entry == *in_value)
-      return false;
-  }
-  list_->push_back(std::move(in_value));
+  if (std::find(list_->begin(), list_->end(), *in_value) != list_->end())
+    return false;
+
+  list_->push_back(std::move(*in_value));
   return true;
 }
 
@@ -1301,15 +1289,12 @@ bool ListValue::Insert(size_t index, std::unique_ptr<Value> in_value) {
   if (index > list_->size())
     return false;
 
-  list_->insert(list_->begin() + index, std::move(in_value));
+  list_->insert(list_->begin() + index, std::move(*in_value));
   return true;
 }
 
 ListValue::const_iterator ListValue::Find(const Value& value) const {
-  return std::find_if(list_->begin(), list_->end(),
-                      [&value](const std::unique_ptr<Value>& entry) {
-                        return *entry == value;
-                      });
+  return std::find(list_->begin(), list_->end(), value);
 }
 
 void ListValue::Swap(ListValue* other) {

base/values.h

@@ -49,7 +49,7 @@ class Value;
 class BASE_EXPORT Value {
  public:
   using DictStorage = base::flat_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,
@@ -390,9 +390,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);
+
   // 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.

base/values_unittest.cc

@@ -437,7 +437,7 @@ TEST(ValuesTest, List) {
   base::Value not_found_value(false);
 
   ASSERT_NE(mixed_list->end(), mixed_list->Find(sought_value));
-  ASSERT_TRUE((*mixed_list->Find(sought_value))->GetAsInteger(&int_value));
+  ASSERT_TRUE((*mixed_list->Find(sought_value)).GetAsInteger(&int_value));
   ASSERT_EQ(42, int_value);
   ASSERT_EQ(mixed_list->end(), mixed_list->Find(not_found_value));
 }
@@ -540,10 +540,10 @@ TEST(ValuesTest, ListRemoval) {
   {
     ListValue list;
     auto value = MakeUnique<Value>();
-    Value* original_value = value.get();
+    Value original_value = *value;
     list.Append(std::move(value));
     size_t index = 0;
-    list.Remove(*original_value, &index);
+    list.Remove(original_value, &index);
     EXPECT_EQ(0U, index);
     EXPECT_EQ(0U, list.GetSize());
   }

cc/test/layer_tree_json_parser.cc

@@ -152,7 +152,7 @@ scoped_refptr<Layer> ParseTreeFromValue(const base::Value& val,
 
   success &= dict->GetList("Children", &list);
   for (const auto& value : *list) {
-    new_layer->AddChild(ParseTreeFromValue(*value, content_client));
+    new_layer->AddChild(ParseTreeFromValue(value, content_client));
   }
 
   if (!success)

chrome/browser/android/bookmarks/partner_bookmarks_shim.cc

@@ -253,7 +253,7 @@ void PartnerBookmarksShim::ReloadNodeMapping() {
   for (base::ListValue::const_iterator it = list->begin();
        it != list->end(); ++it) {
     const base::DictionaryValue* dict = NULL;
-    if (!*it || !(*it)->GetAsDictionary(&dict)) {
+    if (!it->GetAsDictionary(&dict)) {
       NOTREACHED();
       continue;
     }

chrome/browser/android/vr_shell/gltf_parser.cc

@@ -206,7 +206,7 @@ bool GltfParser::SetMeshes(const base::DictionaryValue& dict) {
     if (mesh_dict->GetList("primitives", &list)) {
       for (const auto& primitive_value : *list) {
         const base::DictionaryValue* primitive_dict;
-        if (!primitive_value->GetAsDictionary(&primitive_dict))
+        if (!primitive_value.GetAsDictionary(&primitive_dict))
           return false;
 
         auto primitive = ProcessPrimitive(*primitive_dict);
@@ -261,7 +261,7 @@ bool GltfParser::SetNodes(const base::DictionaryValue& dict) {
     if (node_dict->GetList("meshes", &list)) {
       std::string mesh_key;
       for (const auto& mesh_value : *list) {
-        if (!mesh_value->GetAsString(&mesh_key))
+        if (!mesh_value.GetAsString(&mesh_key))
           return false;
         auto mesh_it = mesh_ids_.find(mesh_key);
         if (mesh_it == mesh_ids_.end())
@@ -284,7 +284,7 @@ bool GltfParser::SetNodes(const base::DictionaryValue& dict) {
     if (node_dict->GetList("children", &list)) {
       std::string node_key;
       for (const auto& mesh_value : *list) {
-        if (!mesh_value->GetAsString(&node_key))
+        if (!mesh_value.GetAsString(&node_key))
           return false;
         auto node_it = nodes.find(node_key);
         if (node_it == nodes.end())
@@ -308,7 +308,7 @@ bool GltfParser::SetScenes(const base::DictionaryValue& dict) {
     if (scene_dict->GetList("nodes", &list)) {
       std::string node_key;
       for (const auto& node_value : *list) {
-        if (!node_value->GetAsString(&node_key))
+        if (!node_value.GetAsString(&node_key))
           return false;
         auto node_it = node_ids_.find(node_key);
         if (node_it == node_ids_.end())

chrome/browser/android/vr_shell/ui_scene.cc

@@ -290,7 +290,7 @@ void UiScene::HandleCommands(std::unique_ptr<base::ListValue> commands,
                              const base::TimeTicks& time) {
   for (auto& item : *commands) {
     base::DictionaryValue* dict;
-    CHECK(item->GetAsDictionary(&dict));
+    CHECK(item.GetAsDictionary(&dict));
 
     Command type;
     base::DictionaryValue* data;