Move Storage for ListValue and DictValue in Union

base/values.cc

Index: base/values.cc
diff --git a/base/values.cc b/base/values.cc
index e835e0088ec5a0191d5c44ffcee824e230c1a6d6..5c1b7606d7196258f16667c6598d305d62e57b76 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -73,17 +73,6 @@ std::unique_ptr<Value> CopyWithoutEmptyChildren(const Value& node) {
   }
 }
 
-// TODO(crbug.com/646113): Remove this once all types are implemented.
-bool IsAssignmentSafe(Value::Type lhs, Value::Type rhs) {
-  auto IsImplemented = [](Value::Type type) {
-    return type == Value::Type::NONE || type == Value::Type::BOOLEAN ||
-           type == Value::Type::INTEGER || type == Value::Type::DOUBLE ||
-           type == Value::Type::STRING || type == Value::Type::BINARY;
-  };
-
-  return lhs == rhs || (IsImplemented(lhs) && IsImplemented(rhs));
-}
-
 }  // namespace
 
 // static
@@ -129,11 +118,11 @@ Value::Value(Type type) : type_(type) {
     case Type::BINARY:
       binary_value_.Init();
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+      dict_ptr_.Init(MakeUnique<DictStorage>());
+      return;
+    case Type::LIST:
+      list_.Init();
       return;
   }
 }
@@ -185,7 +174,6 @@ Value::Value(std::vector<char>&& in_blob) : type_(Type::BINARY) {
 
 Value& Value::operator=(const Value& that) {
   if (this != &that) {
-    DCHECK(IsAssignmentSafe(type_, that.type_));
     if (type_ == that.type_) {
       InternalCopyAssignFrom(that);
     } else {
@@ -199,7 +187,6 @@ Value& Value::operator=(const Value& that) {
 
 Value& Value::operator=(Value&& that) {
   if (this != &that) {
-    DCHECK(IsAssignmentSafe(type_, that.type_));
     if (type_ == that.type_) {
       InternalMoveAssignFrom(std::move(that));
     } else {
@@ -328,19 +315,35 @@ bool Value::GetAsBinary(const BinaryValue** out_value) const {
 }
 
 bool Value::GetAsList(ListValue** out_value) {
-  return false;
+  if (out_value && is_list()) {
+    *out_value = static_cast<ListValue*>(this);
+    return true;
+  }
+  return is_list();
 }
 
 bool Value::GetAsList(const ListValue** out_value) const {
-  return false;
+  if (out_value && is_list()) {
+    *out_value = static_cast<const ListValue*>(this);
+    return true;
+  }
+  return is_list();
 }
 
 bool Value::GetAsDictionary(DictionaryValue** out_value) {
-  return false;
+  if (out_value && is_dict()) {
+    *out_value = static_cast<DictionaryValue*>(this);
+    return true;
+  }
+  return is_dict();
 }
 
 bool Value::GetAsDictionary(const DictionaryValue** out_value) const {
-  return false;
+  if (out_value && is_dict()) {
+    *out_value = static_cast<const DictionaryValue*>(this);
+    return true;
+  }
+  return is_dict();
 }
 
 Value* Value::DeepCopy() const {
@@ -367,8 +370,29 @@ Value* Value::DeepCopy() const {
     case Type::BINARY:
       return new BinaryValue(*binary_value_);
 
+    // TODO(crbug.com/646113): Clean this up when DictionaryValue and ListValue
+    // are completely inlined.
+    case Type::DICTIONARY: {
+      DictionaryValue* result = new DictionaryValue;
+
+      for (const auto& current_entry : **dict_ptr_) {
+        result->SetWithoutPathExpansion(current_entry.first,
+                                        current_entry.second->CreateDeepCopy());
+      }
+
+      return result;
+    }
+
+    case Type::LIST: {
+      ListValue* result = new ListValue;
+
+      for (const auto& entry : *list_)
+        result->Append(entry->CreateDeepCopy());
+
+      return result;
+    }
+
     default:
-      // All other types should be handled by subclasses.
       NOTREACHED();
       return nullptr;
   }
@@ -395,12 +419,37 @@ bool Value::Equals(const Value* other) const {
       return *string_value_ == *(other->string_value_);
     case Type::BINARY:
       return *binary_value_ == *(other->binary_value_);
-    default:
-      // This method should only be getting called for the above types -- all
-      // subclasses need to provide their own implementation;.
-      NOTREACHED();
-      return false;
+    // TODO(crbug.com/646113): Clean this up when DictionaryValue and ListValue
+    // are completely inlined.
+    case Type::DICTIONARY: {
+      if ((*dict_ptr_)->size() != (*other->dict_ptr_)->size())
+        return false;
+
+      return std::equal(std::begin(**dict_ptr_), std::end(**dict_ptr_),
+                        std::begin(**(other->dict_ptr_)),
+                        [](const DictStorage::value_type& lhs,
+                           const DictStorage::value_type& rhs) {
+                          if (lhs.first != rhs.first)
+                            return false;
+
+                          return lhs.second->Equals(rhs.second.get());
+                        });
+    }
+    case Type::LIST: {
+      if (list_->size() != other->list_->size())
+        return false;
+
+      return std::equal(std::begin(*list_), std::end(*list_),
+                        std::begin(*(other->list_)),
+                        [](const ListStorage::value_type& lhs,
+                           const ListStorage::value_type& rhs) {
+                          return lhs->Equals(rhs.get());
+                        });
+    }
   }
+
+  NOTREACHED();
+  return false;
 }
 
 // static
@@ -448,11 +497,13 @@ void Value::InternalCopyConstructFrom(const Value& that) {
     case Type::BINARY:
       binary_value_.Init(*that.binary_value_);
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+    case Type::LIST:
+      // Currently not implementable due to the presence of unique_ptrs in

[brettw, 2017/02/14 23:15:04]

I don't quite follow. Shouldn't this be the same as DeepCopy? With the current
state of the code, I'm also not sure what prevents us from reaching this code.
Before DictionaryValue and ListValue didn't have copy constructors so I believe
no code will call this now. But once you check this in it will be easy to
convert a DictionaryValue to a Value and then copy that.

[jdoerrie, 2017/02/15 15:29:08]

Yes, you are right. When writing this I just considered that
|list_.Init(*that.list_);| wouldn't work here. Doing a DeepCopy does work, of
course. Changed it now.

+      // DictStorage and ListStorage.
+      // TODO(crbug.com/646113): Implement this when DictionaryValues and
+      // ListValues are completely inlined.
+      NOTREACHED();
       return;
   }
 }
@@ -474,11 +525,11 @@ void Value::InternalMoveConstructFrom(Value&& that) {
     case Type::BINARY:
       binary_value_.InitFromMove(std::move(that.binary_value_));
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+      dict_ptr_.InitFromMove(std::move(that.dict_ptr_));
+      return;
+    case Type::LIST:
+      list_.InitFromMove(std::move(that.list_));
       return;
   }
 }
@@ -500,11 +551,13 @@ void Value::InternalCopyAssignFrom(const Value& that) {
     case Type::BINARY:
       *binary_value_ = *that.binary_value_;
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+    case Type::LIST:
+      // Currently not implementable due to the presence of unique_ptrs in

[brettw, 2017/02/14 23:15:05]

Same here.

[jdoerrie, 2017/02/15 15:29:08]

Acknowledged.

+      // DictStorage and ListStorage.
+      // TODO(crbug.com/646113): Implement this when DictionaryValues and
+      // ListValues are completely inlined.
+      NOTREACHED();
       return;
   }
 }
@@ -526,11 +579,11 @@ void Value::InternalMoveAssignFrom(Value&& that) {
     case Type::BINARY:
       *binary_value_ = std::move(*that.binary_value_);
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+      *dict_ptr_ = std::move(*that.dict_ptr_);
+      return;
+    case Type::LIST:
+      *list_ = std::move(*that.list_);
       return;
   }
 }
@@ -550,11 +603,11 @@ void Value::InternalCleanup() {
     case Type::BINARY:
       binary_value_.Destroy();
       return;
-
-    // TODO(crbug.com/646113): Implement these once the corresponding derived
-    // classes are removed.
-    case Type::LIST:
     case Type::DICTIONARY:
+      dict_ptr_.Destroy();
+      return;
+    case Type::LIST:
+      list_.Destroy();
       return;
   }
 }
@@ -574,31 +627,21 @@ std::unique_ptr<DictionaryValue> DictionaryValue::From(
 
 DictionaryValue::DictionaryValue() : Value(Type::DICTIONARY) {}
 
-DictionaryValue::~DictionaryValue() {
-  Clear();
-}
+DictionaryValue::DictionaryValue(DictionaryValue&& that) = default;
 
-bool DictionaryValue::GetAsDictionary(DictionaryValue** out_value) {
-  if (out_value)
-    *out_value = this;
-  return true;
-}
+DictionaryValue& DictionaryValue::operator=(DictionaryValue&& that) = default;
 
-bool DictionaryValue::GetAsDictionary(const DictionaryValue** out_value) const {
-  if (out_value)
-    *out_value = this;
-  return true;
-}
+DictionaryValue::~DictionaryValue() = default;
 
 bool DictionaryValue::HasKey(StringPiece key) const {
   DCHECK(IsStringUTF8(key));
-  auto current_entry = dictionary_.find(key.as_string());
-  DCHECK((current_entry == dictionary_.end()) || current_entry->second);
-  return current_entry != dictionary_.end();
+  auto current_entry = (*dict_ptr_)->find(key.as_string());
+  DCHECK((current_entry == (*dict_ptr_)->end()) || current_entry->second);
+  return current_entry != (*dict_ptr_)->end();
 }
 
 void DictionaryValue::Clear() {
-  dictionary_.clear();
+  (*dict_ptr_)->clear();
 }
 
 void DictionaryValue::Set(StringPiece path, std::unique_ptr<Value> in_value) {
@@ -653,7 +696,7 @@ void DictionaryValue::SetString(StringPiece path, const string16& in_value) {
 
 void DictionaryValue::SetWithoutPathExpansion(StringPiece key,
                                               std::unique_ptr<Value> in_value) {
-  dictionary_[key.as_string()] = std::move(in_value);
+  (**dict_ptr_)[key.as_string()] = std::move(in_value);
 }
 
 void DictionaryValue::SetWithoutPathExpansion(StringPiece key,
@@ -833,8 +876,8 @@ bool DictionaryValue::GetList(StringPiece path, ListValue** out_value) {
 bool DictionaryValue::GetWithoutPathExpansion(StringPiece key,
                                               const Value** out_value) const {
   DCHECK(IsStringUTF8(key));
-  auto entry_iterator = dictionary_.find(key.as_string());
-  if (entry_iterator == dictionary_.end())
+  auto entry_iterator = (*dict_ptr_)->find(key.as_string());
+  if (entry_iterator == (*dict_ptr_)->end())
     return false;
 
   if (out_value)
@@ -962,13 +1005,13 @@ bool DictionaryValue::RemoveWithoutPathExpansion(
     StringPiece key,
     std::unique_ptr<Value>* out_value) {
   DCHECK(IsStringUTF8(key));
-  auto entry_iterator = dictionary_.find(key.as_string());
-  if (entry_iterator == dictionary_.end())
+  auto entry_iterator = (*dict_ptr_)->find(key.as_string());
+  if (entry_iterator == (*dict_ptr_)->end())
     return false;
 
   if (out_value)
     *out_value = std::move(entry_iterator->second);
-  dictionary_.erase(entry_iterator);
+  (*dict_ptr_)->erase(entry_iterator);
   return true;
 }
 
@@ -1020,54 +1063,24 @@ void DictionaryValue::MergeDictionary(const DictionaryValue* dictionary) {
 }
 
 void DictionaryValue::Swap(DictionaryValue* other) {
-  dictionary_.swap(other->dictionary_);
+  dict_ptr_->swap(*(other->dict_ptr_));
 }
 
 DictionaryValue::Iterator::Iterator(const DictionaryValue& target)
-    : target_(target),
-      it_(target.dictionary_.begin()) {}
+    : target_(target), it_((*target.dict_ptr_)->begin()) {}
 
 DictionaryValue::Iterator::Iterator(const Iterator& other) = default;
 
 DictionaryValue::Iterator::~Iterator() {}
 
 DictionaryValue* DictionaryValue::DeepCopy() const {
-  DictionaryValue* result = new DictionaryValue;
-
-  for (const auto& current_entry : dictionary_) {
-    result->SetWithoutPathExpansion(current_entry.first,
-                                    current_entry.second->CreateDeepCopy());
-  }
-
-  return result;
+  return static_cast<DictionaryValue*>(Value::DeepCopy());
 }
 
 std::unique_ptr<DictionaryValue> DictionaryValue::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
 
-bool DictionaryValue::Equals(const Value* other) const {
-  if (other->GetType() != GetType())
-    return false;
-
-  const DictionaryValue* other_dict =
-      static_cast<const DictionaryValue*>(other);
-  Iterator lhs_it(*this);
-  Iterator rhs_it(*other_dict);
-  while (!lhs_it.IsAtEnd() && !rhs_it.IsAtEnd()) {
-    if (lhs_it.key() != rhs_it.key() ||
-        !lhs_it.value().Equals(&rhs_it.value())) {
-      return false;
-    }
-    lhs_it.Advance();
-    rhs_it.Advance();
-  }
-  if (!lhs_it.IsAtEnd() || !rhs_it.IsAtEnd())
-    return false;
-
-  return true;
-}
-
 ///////////////////// ListValue ////////////////////
 
 // static
@@ -1082,12 +1095,14 @@ std::unique_ptr<ListValue> ListValue::From(std::unique_ptr<Value> value) {
 
 ListValue::ListValue() : Value(Type::LIST) {}
 
-ListValue::~ListValue() {
-  Clear();
-}
+ListValue::ListValue(ListValue&& that) = default;
+
+ListValue& ListValue::operator=(ListValue&& that) = default;
+
+ListValue::~ListValue() = default;
 
 void ListValue::Clear() {
-  list_.clear();
+  list_->clear();
 }
 
 bool ListValue::Set(size_t index, Value* in_value) {
@@ -1098,25 +1113,25 @@ bool ListValue::Set(size_t index, std::unique_ptr<Value> in_value) {
   if (!in_value)
     return false;
 
-  if (index >= list_.size()) {
+  if (index >= list_->size()) {
     // Pad out any intermediate indexes with null settings
-    while (index > list_.size())
+    while (index > list_->size())
       Append(CreateNullValue());
     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);
+    DCHECK((*list_)[index] != in_value);
+    (*list_)[index] = std::move(in_value);
   }
   return true;
 }
 
 bool ListValue::Get(size_t index, const Value** out_value) const {
-  if (index >= list_.size())
+  if (index >= list_->size())
     return false;
 
   if (out_value)
-    *out_value = list_[index].get();
+    *out_value = (*list_)[index].get();
 
   return true;
 }
@@ -1223,21 +1238,21 @@ bool ListValue::GetList(size_t index, ListValue** out_value) {
 }
 
 bool ListValue::Remove(size_t index, std::unique_ptr<Value>* out_value) {
-  if (index >= list_.size())
+  if (index >= list_->size())
     return false;
 
   if (out_value)
-    *out_value = std::move(list_[index]);
+    *out_value = std::move((*list_)[index]);
 
-  list_.erase(list_.begin() + 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) {
+  for (auto it = list_->begin(); it != list_->end(); ++it) {
     if ((*it)->Equals(&value)) {
-      size_t previous_index = it - list_.begin();
-      list_.erase(it);
+      size_t previous_index = it - list_->begin();
+      list_->erase(it);
 
       if (index)
         *index = previous_index;
@@ -1250,13 +1265,13 @@ bool ListValue::Remove(const Value& value, size_t* index) {
 ListValue::iterator ListValue::Erase(iterator iter,
                                      std::unique_ptr<Value>* out_value) {
   if (out_value)
-    *out_value = std::move(*Storage::iterator(iter));
+    *out_value = std::move(*ListStorage::iterator(iter));
 
-  return list_.erase(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)
@@ -1302,79 +1317,43 @@ 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_) {
+  for (const auto& entry : *list_) {
     if (entry->Equals(in_value.get())) {
       return false;
     }
   }
-  list_.push_back(std::move(in_value));
+  list_->push_back(std::move(in_value));
   return true;
 }
 
 bool ListValue::Insert(size_t index, std::unique_ptr<Value> in_value) {
   DCHECK(in_value);
-  if (index > list_.size())
+  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(),
+  return std::find_if(list_->begin(), list_->end(),
                       [&value](const std::unique_ptr<Value>& entry) {
                         return entry->Equals(&value);
                       });
 }
 
 void ListValue::Swap(ListValue* other) {
-  list_.swap(other->list_);
-}
-
-bool ListValue::GetAsList(ListValue** out_value) {
-  if (out_value)
-    *out_value = this;
-  return true;
-}
-
-bool ListValue::GetAsList(const ListValue** out_value) const {
-  if (out_value)
-    *out_value = this;
-  return true;
+  list_->swap(*(other->list_));
 }
 
 ListValue* ListValue::DeepCopy() const {
-  ListValue* result = new ListValue;
-
-  for (const auto& entry : list_)
-    result->Append(entry->CreateDeepCopy());
-
-  return result;
+  return static_cast<ListValue*>(Value::DeepCopy());
 }
 
 std::unique_ptr<ListValue> ListValue::CreateDeepCopy() const {
   return WrapUnique(DeepCopy());
 }
 
-bool ListValue::Equals(const Value* other) const {
-  if (other->GetType() != GetType())
-    return false;
-
-  const ListValue* other_list =
-      static_cast<const ListValue*>(other);
-  Storage::const_iterator lhs_it, rhs_it;
-  for (lhs_it = begin(), rhs_it = other_list->begin();
-       lhs_it != end() && rhs_it != other_list->end();
-       ++lhs_it, ++rhs_it) {
-    if (!(*lhs_it)->Equals(rhs_it->get()))
-      return false;
-  }
-  if (lhs_it != end() || rhs_it != other_list->end())
-    return false;
-
-  return true;
-}
-
 ValueSerializer::~ValueSerializer() {
 }