Tidy up SVGListPropertyHelper

third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h

Index: third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h
diff --git a/third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h b/third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h
index 22f2279efa4b33cb2ec7095f5c4904b5a84ad176..528b556c62ab433d9c99d95cc5f4c6aa22986f5c 100644
--- a/third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h
+++ b/third_party/WebKit/Source/core/svg/properties/SVGListPropertyHelper.h
@@ -99,7 +99,9 @@ class SVGListPropertyHelper : public SVGPropertyHelper<Derived> {
     new_item->SetOwnerList(this);
   }
 
-  bool operator==(const Derived&) const;
+  bool operator==(const Derived& other) const {
+    return values_ == other.values_;
+  }
   bool operator!=(const Derived& other) const { return !(*this == other); }
 
   bool IsEmpty() const { return !length(); }
@@ -142,38 +144,17 @@ class SVGListPropertyHelper : public SVGPropertyHelper<Derived> {
 
  private:
   inline bool CheckIndexBound(size_t, ExceptionState&);
-  size_t FindItem(ItemPropertyType*);
 
   HeapVector<Member<ItemPropertyType>> values_;
 };
 
 template <typename Derived, typename ItemProperty>
-bool SVGListPropertyHelper<Derived, ItemProperty>::operator==(
-    const Derived& other) const {
-  if (length() != other.length())
-    return false;
-
-  size_t size = length();
-  for (size_t i = 0; i < size; ++i) {
-    if (*at(i) != *other.at(i))
-      return false;
-  }
-
-  return true;
-}
-
-template <typename Derived, typename ItemProperty>
 void SVGListPropertyHelper<Derived, ItemProperty>::Clear() {
-  // detach all list items as they are no longer part of this list
-  typename HeapVector<Member<ItemPropertyType>>::const_iterator it =
-      values_.begin();
-  typename HeapVector<Member<ItemPropertyType>>::const_iterator it_end =
-      values_.end();
-  for (; it != it_end; ++it) {
-    DCHECK_EQ((*it)->OwnerList(), this);
-    (*it)->SetOwnerList(nullptr);
+  // Detach all list items as they are no longer part of this list.
+  for (auto& value : values_) {
+    DCHECK_EQ(value->OwnerList(), this);
+    value->SetOwnerList(nullptr);
   }
-
   values_.clear();
 }
 
@@ -193,10 +174,7 @@ ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::GetItem(
     ExceptionState& exception_state) {
   if (!CheckIndexBound(index, exception_state))
     return nullptr;
-
-  DCHECK_LT(index, values_.size());
-  DCHECK_EQ(values_.at(index)->OwnerList(), this);
-  return values_.at(index);
+  return at(index);
 }
 
 template <typename Derived, typename ItemProperty>
@@ -214,7 +192,6 @@ ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::InsertItemBefore(
   // front of the list.
   values_.insert(index, new_item);
   new_item->SetOwnerList(this);
-
   return new_item;
 }
 
@@ -222,16 +199,13 @@ template <typename Derived, typename ItemProperty>
 ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::RemoveItem(
     size_t index,
     ExceptionState& exception_state) {
-  if (index >= values_.size()) {
-    exception_state.ThrowDOMException(
-        kIndexSizeError, ExceptionMessages::IndexExceedsMaximumBound(
-                             "index", index, values_.size()));
+  if (!CheckIndexBound(index, exception_state))
     return nullptr;
-  }
+
   DCHECK_EQ(values_.at(index)->OwnerList(), this);
   ItemPropertyType* old_item = values_.at(index);
   values_.erase(index);
-  old_item->SetOwnerList(0);
+  old_item->SetOwnerList(nullptr);
   return old_item;
 }
 
@@ -240,7 +214,6 @@ ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::AppendItem(
     ItemProperty* new_item) {
   // Append the value and wrapper at the end of the list.
   Append(new_item);
-
   return new_item;
 }
 
@@ -255,6 +228,7 @@ ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::ReplaceItem(
   if (values_.IsEmpty()) {
     // 'newItem' already lived in our list, we removed it, and now we're empty,
     // which means there's nothing to replace.
+    // TODO(fs): This should not cause us to throw an exception.

[pdr., 2017/06/04 17:05:08]

Is this actually deadcode due to the CheckIndexBound check?

[fs, 2017/06/04 17:11:27]

Yes, it certainly looks that way. Should check that we have test coverage for
this though (before removing.)

     exception_state.ThrowDOMException(
         kIndexSizeError,
         String::Format("Failed to replace the provided item at index %zu.",
@@ -265,10 +239,9 @@ ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::ReplaceItem(
   // Update the value at the desired position 'index'.
   Member<ItemPropertyType>& position = values_[index];
   DCHECK_EQ(position->OwnerList(), this);
-  position->SetOwnerList(0);
+  position->SetOwnerList(nullptr);
   position = new_item;
   new_item->SetOwnerList(this);
-
   return new_item;
 }
 
@@ -282,26 +255,14 @@ bool SVGListPropertyHelper<Derived, ItemProperty>::CheckIndexBound(
                              "index", index, values_.size()));
     return false;
   }
-
   return true;
 }
 
 template <typename Derived, typename ItemProperty>
-size_t SVGListPropertyHelper<Derived, ItemProperty>::FindItem(
-    ItemPropertyType* item) {
-  return values_.Find(item);
-}
-
-template <typename Derived, typename ItemProperty>
 void SVGListPropertyHelper<Derived, ItemProperty>::DeepCopy(Derived* from) {
   Clear();
-  typename HeapVector<Member<ItemPropertyType>>::const_iterator it =
-      from->values_.begin();
-  typename HeapVector<Member<ItemPropertyType>>::const_iterator it_end =
-      from->values_.end();
-  for (; it != it_end; ++it) {
-    Append((*it)->Clone());
-  }
+  for (const auto& from_value : from->values_)
+    Append(from_value->Clone());
 }
 
 template <typename Derived, typename ItemProperty>
@@ -325,16 +286,12 @@ bool SVGListPropertyHelper<Derived, ItemProperty>::AdjustFromToListValues(
     } else {
       DeepCopy(to_list);
     }
-
     return false;
   }
 
   DCHECK(!from_list_size || from_list_size == to_list_size);
-  if (length() < to_list_size) {
-    size_t padding_count = to_list_size - length();
-    for (size_t i = 0; i < padding_count; ++i)
-      Append(CreatePaddingItem());
-  }
+  for (size_t i = length(); i < to_list_size; ++i)
+    Append(CreatePaddingItem());
 
   return true;
 }