Tidy up SVGListPropertyHelper

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

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 81 matching lines @@
 
   ConstIterator begin() const { return ConstIterator(values_.begin()); }
   ConstIterator end() const { return ConstIterator(values_.end()); }
 
   void Append(ItemPropertyType* new_item) {
     DCHECK(new_item);
     values_.push_back(new_item);
     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(); }
 
   virtual Derived* Clone() {
     Derived* svg_list = Derived::Create();
     svg_list->DeepCopy(static_cast<Derived*>(this));
     return svg_list;
   }
 
@@ skipping 22 matching lines @@
                               Derived* to_list,
                               float percentage,
                               AnimationMode);
 
   virtual ItemPropertyType* CreatePaddingItem() const {
     return ItemPropertyType::Create();
   }
 
  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;
+void SVGListPropertyHelper<Derived, ItemProperty>::Clear() {
+  // 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);
   }
-
-  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);
-  }
-
   values_.clear();
 }
 
 template <typename Derived, typename ItemProperty>
 ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::Initialize(
     ItemProperty* new_item) {
   // Spec: Clears all existing current items from the list and re-initializes
   // the list to hold the single item specified by the parameter.
   Clear();
   Append(new_item);
   return new_item;
 }
 
 template <typename Derived, typename ItemProperty>
 ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::GetItem(
     size_t index,
     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>
 ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::InsertItemBefore(
     ItemProperty* new_item,
     size_t index) {
   // Spec: If the index is greater than or equal to length, then the new item is
   // appended to the end of the list.
   if (index > values_.size())
     index = values_.size();
 
   // Spec: Inserts a new item into the list at the specified position. The index
   // of the item before which the new item is to be inserted. The first item is
   // number 0. If the index is equal to 0, then the new item is inserted at the
   // front of the list.
   values_.insert(index, new_item);
   new_item->SetOwnerList(this);
-
   return new_item;
 }
 
 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;
 }
 
 template <typename Derived, typename ItemProperty>
 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;
 }
 
 template <typename Derived, typename ItemProperty>
 ItemProperty* SVGListPropertyHelper<Derived, ItemProperty>::ReplaceItem(
     ItemProperty* new_item,
     size_t index,
     ExceptionState& exception_state) {
   if (!CheckIndexBound(index, exception_state))
     return nullptr;
 
   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.",
                        index));
     return nullptr;
   }
 
   // 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;
 }
 
 template <typename Derived, typename ItemProperty>
 bool SVGListPropertyHelper<Derived, ItemProperty>::CheckIndexBound(
     size_t index,
     ExceptionState& exception_state) {
   if (index >= values_.size()) {
     exception_state.ThrowDOMException(
         kIndexSizeError, ExceptionMessages::IndexExceedsMaximumBound(
                              "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>
 bool SVGListPropertyHelper<Derived, ItemProperty>::AdjustFromToListValues(
     Derived* from_list,
     Derived* to_list,
     float percentage,
     AnimationMode mode) {
   // If no 'to' value is given, nothing to animate.
   size_t to_list_size = to_list->length();
   if (!to_list_size)
     return false;
 
   // If the 'from' value is given and it's length doesn't match the 'to' value
   // list length, fallback to a discrete animation.
   size_t from_list_size = from_list->length();
   if (from_list_size != to_list_size && from_list_size) {
     if (percentage < 0.5) {
       if (mode != kToAnimation)
         DeepCopy(from_list);
     } 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;
 }
 
 }  // namespace blink
 
 #endif  // SVGListPropertyHelper_h