Rework SVGStringList to match SVGProperty object lists

This moves DOM interface operations to the tear-off class, similarly to
how SVGProperty object lists are structured.

Change SVGStringListTearOff to inherit from SVGPropertyTearOffBase
rather than SVGPropertyTearOff<T> since since this type is never
animated, and thus does not need the ability to re-target to another
object.

Bug: 1106762
Change-Id: Ic8040339e3a3cdf6a818333dd77bdf15af3829ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302755Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#800946}

third_party/blink/renderer/core/frame/dactyloscoper.cc

@@ -94,7 +94,7 @@ void Dactyloscoper::RecordDirectSurface(
 void Dactyloscoper::RecordDirectSurface(ExecutionContext* context,
                                         WebFeature feature,
                                         SVGStringListTearOff* strings) {
-  RecordDirectSurface(context, feature, strings->Target()->Values());
+  RecordDirectSurface(context, feature, strings->Values());
 }
 
 }  // namespace blink

third_party/blink/renderer/core/svg/svg_static_string_list.cc

@@ -75,10 +75,8 @@ void SVGStaticStringList::AnimationEnded() {
 }
 
 SVGStringListTearOff* SVGStaticStringList::TearOff() {
-  if (!tear_off_) {
-    tear_off_ = MakeGarbageCollected<SVGStringListTearOff>(
-        value_, this, kPropertyIsNotAnimVal);
-  }
+  if (!tear_off_)
+    tear_off_ = MakeGarbageCollected<SVGStringListTearOff>(value_, this);
   return tear_off_.Get();
 }
 

third_party/blink/renderer/core/svg/svg_string_list.cc

@@ -21,62 +21,29 @@
 #include "third_party/blink/renderer/core/svg/svg_string_list.h"
 
 #include "third_party/blink/renderer/core/svg/svg_parser_utilities.h"
-#include "third_party/blink/renderer/platform/bindings/exception_messages.h"
-#include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/wtf/text/string_builder.h"
 
 namespace blink {
 
 SVGStringListBase::~SVGStringListBase() = default;
 
-void SVGStringListBase::Initialize(const String& item) {
+void SVGStringListBase::Clear() {
   values_.clear();
-  values_.push_back(item);
 }
 
-String SVGStringListBase::GetItem(uint32_t index,
-                                  ExceptionState& exception_state) {
-  if (!CheckIndexBound(index, exception_state))
-    return String();
-
-  return values_.at(index);
-}
-
-void SVGStringListBase::InsertItemBefore(const String& new_item,
-                                         uint32_t index) {
-  // Spec: If the index is greater than or equal to numberOfItems, 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.
+void SVGStringListBase::Insert(uint32_t index, const String& new_item) {
   values_.insert(index, new_item);
 }
 
-String SVGStringListBase::RemoveItem(uint32_t index,
-                                     ExceptionState& exception_state) {
-  if (!CheckIndexBound(index, exception_state))
-    return String();
-
-  String old_item = values_.at(index);
+void SVGStringListBase::Remove(uint32_t index) {
   values_.EraseAt(index);
-  return old_item;
 }
 
-void SVGStringListBase::AppendItem(const String& new_item) {
+void SVGStringListBase::Append(const String& new_item) {
   values_.push_back(new_item);
 }
 
-void SVGStringListBase::ReplaceItem(const String& new_item,
-                                    uint32_t index,
-                                    ExceptionState& exception_state) {
-  if (!CheckIndexBound(index, exception_state))
-    return;
-
-  // Update the value at the desired position 'index'.
+void SVGStringListBase::Replace(uint32_t index, const String& new_item) {
   values_[index] = new_item;
 }
 
@@ -138,19 +105,6 @@ String SVGStringListBase::ValueAsStringWithDelimiter(
   return builder.ToString();
 }
 
-bool SVGStringListBase::CheckIndexBound(uint32_t index,
-                                        ExceptionState& exception_state) {
-  if (index >= values_.size()) {
-    exception_state.ThrowDOMException(
-        DOMExceptionCode::kIndexSizeError,
-        ExceptionMessages::IndexExceedsMaximumBound("index", index,
-                                                    values_.size()));
-    return false;
-  }
-
-  return true;
-}
-
 void SVGStringListBase::Add(SVGPropertyBase* other,
                             SVGElement* context_element) {
   // SVGStringList is never animated.

third_party/blink/renderer/core/svg/svg_string_list.h

@@ -33,11 +33,9 @@
 
 #include "third_party/blink/renderer/core/svg/properties/svg_property_helper.h"
 #include "third_party/blink/renderer/core/svg/svg_parsing_error.h"
-#include "third_party/blink/renderer/core/svg/svg_string.h"
 
 namespace blink {
 
-class ExceptionState;
 class SVGStringListTearOff;
 
 // Implementation of SVGStringList spec:
@@ -62,20 +60,16 @@ class SVGStringListBase : public SVGPropertyBase {
 
   const Vector<String>& Values() const { return values_; }
 
-  // SVGStringList DOM Spec implementation. These are only to be called from
-  // SVGStringListTearOff:
   uint32_t length() { return values_.size(); }
-  void clear() { values_.clear(); }
-  void Initialize(const String&);
-  String GetItem(uint32_t, ExceptionState&);
-  void InsertItemBefore(const String&, uint32_t);
-  String RemoveItem(uint32_t, ExceptionState&);
-  void AppendItem(const String&);
-  void ReplaceItem(const String&, uint32_t, ExceptionState&);
+  void Clear();
+  void Insert(uint32_t, const String&);
+  void Remove(uint32_t);
+  void Append(const String&);
+  void Replace(uint32_t, const String&);
 
-  // SVGPropertyBase:
   virtual SVGParsingError SetValueAsString(const String&) = 0;
 
+  // SVGPropertyBase:
   void Add(SVGPropertyBase*, SVGElement*) override;
   void CalculateAnimatedValue(const SVGAnimateElement&,
                               float percentage,
@@ -104,7 +98,6 @@ class SVGStringListBase : public SVGPropertyBase {
   void ParseInternal(const CharType*& ptr,
                      const CharType* end,
                      char list_delimiter);
-  bool CheckIndexBound(uint32_t, ExceptionState&);
 
   Vector<String> values_;
 };

third_party/blink/renderer/core/svg/svg_string_list_tear_off.cc

@@ -32,12 +32,8 @@
 
 namespace blink {
 
-SVGStringListTearOff::SVGStringListTearOff(
-    SVGStringListBase* target,
-    SVGAnimatedPropertyBase* binding,
-    PropertyIsAnimValType property_is_anim_val)
-    : SVGPropertyTearOff<SVGStringListBase>(target,
-                                            binding,
-                                            property_is_anim_val) {}
+SVGStringListTearOff::SVGStringListTearOff(SVGStringListBase* target,
+                                           SVGAnimatedPropertyBase* binding)
+    : SVGPropertyTearOffBase(binding, kPropertyIsNotAnimVal), list_(target) {}
 
 }  // namespace blink

third_party/blink/renderer/core/svg/svg_string_list_tear_off.h

@@ -37,50 +37,51 @@
 
 namespace blink {
 
-class SVGStringListTearOff : public SVGPropertyTearOff<SVGStringListBase> {
+class SVGStringListTearOff final : public SVGPropertyTearOffBase {
   DEFINE_WRAPPERTYPEINFO();
 
  public:
-  SVGStringListTearOff(SVGStringListBase*,
-                       SVGAnimatedPropertyBase* binding,
-                       PropertyIsAnimValType);
+  SVGStringListTearOff(SVGStringListBase*, SVGAnimatedPropertyBase* binding);
 
   // SVGStringList DOM interface:
 
   // WebIDL requires "unsigned long" type which is uint32_t.
-  uint32_t length() { return Target()->length(); }
+  uint32_t length() const { return list_->length(); }
 
   void clear(ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
-      return;
-    }
-    Target()->clear();
+    DCHECK(!IsImmutable());
+    list_->Clear();
     CommitChange();
   }
 
   String initialize(const String& item, ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
-      return String();
-    }
-    Target()->Initialize(item);
+    DCHECK(!IsImmutable());
+    list_->Clear();
+    list_->Append(item);
     CommitChange();
     return item;
   }
 
-  String getItem(uint32_t index, ExceptionState& exception_state) {
-    return Target()->GetItem(index, exception_state);
+  String getItem(uint32_t index, ExceptionState& exception_state) const {
+    if (index >= list_->length()) {
+      ThrowIndexSize(exception_state, index, list_->length());
+      return String();
+    }
+    return list_->Values()[index];
   }
 
   String insertItemBefore(const String& item,
                           uint32_t index,
                           ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
-      return String();
-    }
-    Target()->InsertItemBefore(item, index);
+    DCHECK(!IsImmutable());
+    // Spec: If the index is greater than or equal to numberOfItems, then the
+    // new item is appended to the end of the list.
+    index = std::min(index, list_->length());
+    // 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.
+    list_->Insert(index, item);
     CommitChange();
     return item;
   }
@@ -88,11 +89,12 @@ class SVGStringListTearOff : public SVGPropertyTearOff<SVGStringListBase> {
   String replaceItem(const String& item,
                      uint32_t index,
                      ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
+    DCHECK(!IsImmutable());
+    if (index >= list_->length()) {
+      ThrowIndexSize(exception_state, index, list_->length());
       return String();
     }
-    Target()->ReplaceItem(item, index, exception_state);
+    list_->Replace(index, item);
     CommitChange();
     return item;
   }
@@ -106,24 +108,33 @@ class SVGStringListTearOff : public SVGPropertyTearOff<SVGStringListBase> {
   }
 
   String removeItem(uint32_t index, ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
+    DCHECK(!IsImmutable());
+    if (index >= list_->length()) {
+      ThrowIndexSize(exception_state, index, list_->length());
       return String();
     }
-    String removed_item = Target()->RemoveItem(index, exception_state);
+    String removed_item = list_->Values()[index];
+    list_->Remove(index);
     CommitChange();
     return removed_item;
   }
 
   String appendItem(const String& item, ExceptionState& exception_state) {
-    if (IsImmutable()) {
-      ThrowReadOnly(exception_state);
-      return String();
-    }
-    Target()->AppendItem(item);
+    DCHECK(!IsImmutable());
+    list_->Append(item);
     CommitChange();
     return item;
   }
+
+  void Trace(Visitor* visitor) const override {
+    visitor->Trace(list_);
+    SVGPropertyTearOffBase::Trace(visitor);
+  }
+
+  const Vector<String>& Values() const { return list_->Values(); }
+
+ private:
+  const Member<SVGStringListBase> list_;
 };
 
 }  // namespace blink