Revert of Change base::Value::ListStorage to std::vector<base::Value>…

… (patchset chromium#14 id:260001 of https://codereview.chromium.org/2740143002/ ) Reason for revert: Appears to have broken compile on ChromeOS. Needs a rebase? FAILED: obj/extensions/browser/api/networking_private/networking_private/networking_private_chromeos.o In file included from ../../extensions/browser/api/networking_private/networking_private_chromeos.cc:21: In file included from ../../chromeos/network/network_connection_handler.h:18: In file included from ../../chromeos/cert_loader.h:18: In file included from ../../net/cert/cert_database.h:13: ../../net/cert/x509_certificate.h:22:10: fatal error: 'net/net_features.h' file not found #include "net/net_features.h" ^~~~~~~~~~~~~~~~~~~~ 1 error generated. https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/32152 Original issue's description: > 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} > Committed: https://chromium.googlesource.com/chromium/src/+/ebab0defaea3aed024283a64088ebc0cd352b47f TBR=brettw@chromium.org,rdevlin.cronin@chromium.org,flackr@chromium.org,skym@chromium.org,rsesek@chromium.org,bajones@chromium.org,dbeam@chromium.org,stevenjb@chromium.org,jdoerrie@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=646113 Review-Url: https://codereview.chromium.org/2816513002 Cr-Commit-Position: refs/heads/master@{#463630}
cbchan · Apr 11, 2017 · df600cd · df600cd
1 parent 1a5a2cd
commit df600cd
Show file tree

Hide file tree

Showing 201 changed files with 562 additions and 634 deletions.
diff --git a/base/json/json_writer.cc b/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;

diff --git a/base/test/gtest_util.cc b/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;

diff --git a/base/trace_event/trace_event_argument.cc b/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,11 +369,9 @@ 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;
 
@@ -398,8 +396,7 @@ std::unique_ptr<base::Value> TracedValue::ToBaseValue() const {
         } else {
           cur_list->Append(WrapUnique(new_list));
           stack.push_back(cur_list);
-          // |cur_list| is invalidated at this point, so it needs to be reset.
-          cur_list->GetList(cur_list->GetSize() - 1, &cur_list);
+          cur_list = new_list;
         }
       } break;
 

diff --git a/base/trace_event/trace_event_memory_overhead.cc b/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:

diff --git a/base/values.cc b/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,7 +375,12 @@ bool operator==(const Value& lhs, const Value& rhs) {
                                  std::tie(v.first, *v.second);
                         });
     case Value::Type::LIST:
-      return *lhs.list_ == *rhs.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; });
   }
 
   NOTREACHED();
@@ -414,7 +419,11 @@ 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 *lhs.list_ < *rhs.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; });
   }
 
   NOTREACHED();
@@ -485,10 +494,11 @@ void Value::InternalCopyConstructFrom(const Value& that) {
     case Type::BINARY:
       binary_value_.Init(*that.binary_value_);
       return;
-    // 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.
+    // 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.
     case Type::DICTIONARY:
       dict_ptr_.Init(MakeUnique<DictStorage>());
       for (const auto& it : **that.dict_ptr_) {
@@ -498,7 +508,10 @@ void Value::InternalCopyConstructFrom(const Value& that) {
       }
       return;
     case Type::LIST:
-      list_.Init(*that.list_);
+      list_.Init();
+      list_->reserve(that.list_->size());
+      for (const auto& it : *that.list_)
+        list_->push_back(MakeUnique<Value>(*it));
       return;
   }
 }
@@ -548,17 +561,16 @@ void Value::InternalCopyAssignFromSameType(const Value& that) {
     case Type::BINARY:
       *binary_value_ = *that.binary_value_;
       return;
-    // 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_);
+    // 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_);
       return;
-    }
     case Type::LIST:
-      *list_ = *that.list_;
+      *list_ = std::move(*Value(that).list_);
       return;
   }
 }
@@ -1065,10 +1077,6 @@ 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));
 }
@@ -1083,7 +1091,9 @@ bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
       Append(MakeUnique<Value>());
     Append(std::move(in_value));
   } else {
-    (*list_)[index] = std::move(*in_value);
+    // TODO(dcheng): remove this DCHECK once the raw pointer version is removed?
+    DCHECK((*list_)[index] != in_value);
+    (*list_)[index] = std::move(in_value);
   }
   return true;
 }
@@ -1093,7 +1103,7 @@ bool ListValue::Get(size_t index, const Value** out_value) const {
     return false;
 
   if (out_value)
-    *out_value = &(*list_)[index];
+    *out_value = (*list_)[index].get();
 
   return true;
 }
@@ -1203,35 +1213,36 @@ bool ListValue::Remove(size_t index, std::unique_ptr<Value>* out_value) {
     return false;
 
   if (out_value)
-    *out_value = MakeUnique<Value>(std::move((*list_)[index]));
+    *out_value = std::move((*list_)[index]);
 
   list_->erase(list_->begin() + index);
   return true;
 }
 
 bool ListValue::Remove(const Value& value, size_t* index) {
-  auto it = std::find(list_->begin(), list_->end(), value);
-
-  if (it == list_->end())
-    return false;
-
-  if (index)
-    *index = std::distance(list_->begin(), it);
+  for (auto it = list_->begin(); it != list_->end(); ++it) {
+    if (**it == value) {
+      size_t previous_index = it - list_->begin();
+      list_->erase(it);
 
-  list_->erase(it);
-  return true;
+      if (index)
+        *index = previous_index;
+      return true;
+    }
+  }
+  return false;
 }
 
 ListValue::iterator ListValue::Erase(iterator iter,
                                      std::unique_ptr<Value>* out_value) {
   if (out_value)
-    *out_value = MakeUnique<Value>(std::move(*iter));
+    *out_value = std::move(*ListStorage::iterator(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)
@@ -1277,10 +1288,11 @@ void ListValue::AppendStrings(const std::vector<string16>& in_values) {
 
 bool ListValue::AppendIfNotPresent(std::unique_ptr<Value> in_value) {
   DCHECK(in_value);
-  if (std::find(list_->begin(), list_->end(), *in_value) != list_->end())
-    return false;
-
-  list_->push_back(std::move(*in_value));
+  for (const auto& entry : *list_) {
+    if (*entry == *in_value)
+      return false;
+  }
+  list_->push_back(std::move(in_value));
   return true;
 }
 
@@ -1289,12 +1301,15 @@ 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(list_->begin(), list_->end(), value);
+  return std::find_if(list_->begin(), list_->end(),
+                      [&value](const std::unique_ptr<Value>& entry) {
+                        return *entry == value;
+                      });
 }
 
 void ListValue::Swap(ListValue* other) {

diff --git a/base/values.h b/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<Value>;
+  using ListStorage = std::vector<std::unique_ptr<Value>>;
 
   enum class Type {
     NONE = 0,
@@ -390,15 +390,9 @@ 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.

diff --git a/base/values_unittest.cc b/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;
+    Value* original_value = value.get();
     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());
   }

diff --git a/cc/test/layer_tree_json_parser.cc b/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)

diff --git a/chrome/browser/android/bookmarks/partner_bookmarks_shim.cc b/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->GetAsDictionary(&dict)) {
+    if (!*it || !(*it)->GetAsDictionary(&dict)) {
       NOTREACHED();
       continue;
     }

diff --git a/chrome/browser/android/vr_shell/gltf_parser.cc b/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())

diff --git a/chrome/browser/android/vr_shell/ui_scene.cc b/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;