Remove SMILAnimationEffectParameters::is_to_animation

The effect of this parameter can be described using the other
parameters (is_additive = is_cumulative = false).

For list types it was previously used to avoid copying a list into
itself (clobbering it in the process), but that can achieved by
just comparing objects.
Similar techniques were applied in SVGPath and SVGTransformList, but
those classes could deal with the issue differently.

Rewrite SVGPath::CalculateAnimatedValue() to eliminate the extra
copy which should (no longer) be needed.

Change SVGTransformList::CalculateAnimatedValue() to compute the
transform first and then handle addition.

Also re-order the computations to try to get a somewhat canonical
order: interpolation, accumulation, addition.

In SVGAnimateMotionElement, always set |to_point_at_end_of_duration_|
instead of setting and checking a separate bool.
AnimateAdditiveNumber() handles the accumulation if required.

Bug: 1017723
Change-Id: I8adb2a7ad415a2d00aa057bfb93f55953450dd07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2382111
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803060}

third_party/blink/renderer/core/svg/animation/smil_animation_effect_parameters.h

@@ -12,7 +12,6 @@ namespace blink {
 // https://www.w3.org/TR/SMIL/smil-animation.html#animationNS-AnimationEffectFcn
 struct SMILAnimationEffectParameters {
   bool is_discrete = false;
-  bool is_to_animation = false;
   bool is_additive = false;
   bool is_cumulative = false;
 };
@@ -31,12 +30,10 @@ inline void AnimateAdditiveNumber(
     number = percentage < 0.5 ? from_number : to_number;
   else
     number = (to_number - from_number) * percentage + from_number;
-  if (!parameters.is_to_animation) {
   if (repeat_count && parameters.is_cumulative)
     number += to_at_end_of_duration_number * repeat_count;
   if (parameters.is_additive)
     number += animated_number;
-  }
   animated_number = number;
 }
 

third_party/blink/renderer/core/svg/properties/svg_list_property_helper.h

@@ -119,8 +119,7 @@ class SVGListPropertyHelper : public SVGListPropertyBase {
 
   bool AdjustFromToListValues(Derived* from_list,
                               Derived* to_list,
-                              float percentage,
-                              bool is_to_animation);
+                              float percentage);
 
   virtual ItemPropertyType* CreatePaddingItem() const {
     return MakeGarbageCollected<ItemPropertyType>();
@@ -139,8 +138,7 @@ template <typename Derived, typename ItemProperty>
 bool SVGListPropertyHelper<Derived, ItemProperty>::AdjustFromToListValues(
     Derived* from_list,
     Derived* to_list,
-    float percentage,
-    bool is_to_animation) {
+    float percentage) {
   // If no 'to' value is given, nothing to animate.
   uint32_t to_list_size = to_list->length();
   if (!to_list_size)
@@ -150,12 +148,12 @@ bool SVGListPropertyHelper<Derived, ItemProperty>::AdjustFromToListValues(
   // list length, fallback to a discrete animation.
   uint32_t from_list_size = from_list->length();
   if (from_list_size != to_list_size && from_list_size) {
-    if (percentage < 0.5) {
-      if (!is_to_animation)
-        DeepCopy(from_list);
-    } else {
-      DeepCopy(to_list);
-    }
+    const Derived* result = percentage < 0.5 ? from_list : to_list;
+    // If this is a 'to' animation, the "from" value will be the same
+    // list as this list, so avoid the copy in that case since it
+    // would clobber the list.
+    if (result != this)
+      DeepCopy(result);
     return false;
   }
 

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

@@ -58,8 +58,7 @@ bool TargetCanHaveMotionTransform(const SVGElement& target) {
 }
 
 SVGAnimateMotionElement::SVGAnimateMotionElement(Document& document)
-    : SVGAnimationElement(svg_names::kAnimateMotionTag, document),
-      has_to_point_at_end_of_duration_(false) {
+    : SVGAnimationElement(svg_names::kAnimateMotionTag, document) {
   SetCalcMode(kCalcModePaced);
 }
 
@@ -177,16 +176,17 @@ void SVGAnimateMotionElement::ClearAnimatedType() {
 bool SVGAnimateMotionElement::CalculateToAtEndOfDurationValue(
     const String& to_at_end_of_duration_string) {
   ParsePoint(to_at_end_of_duration_string, to_point_at_end_of_duration_);
-  has_to_point_at_end_of_duration_ = true;
   return true;
 }
 
 bool SVGAnimateMotionElement::CalculateFromAndToValues(
     const String& from_string,
     const String& to_string) {
-  has_to_point_at_end_of_duration_ = false;
   ParsePoint(from_string, from_point_);
   ParsePoint(to_string, to_point_);
+  // TODO(fs): Looks like this would clobber the at-end-of-duration
+  // value for a cumulative 'values' animation.
+  to_point_at_end_of_duration_ = to_point_;
   return true;
 }
 
@@ -198,6 +198,7 @@ bool SVGAnimateMotionElement::CalculateFromAndByValues(
   // is 'by', |from_string| will be the empty string and yield a point
   // of (0,0).
   to_point_ += from_point_;
+  to_point_at_end_of_duration_ = to_point_;
   return true;
 }
 
@@ -216,22 +217,14 @@ void SVGAnimateMotionElement::CalculateAnimatedValue(float percentage,
     transform->MakeIdentity();
 
   if (GetAnimationMode() != kPathAnimation) {
-    FloatPoint to_point_at_end_of_duration = to_point_;
-    if (!parameters.is_to_animation) {
-      if (repeat_count && parameters.is_cumulative &&
-          has_to_point_at_end_of_duration_) {
-        to_point_at_end_of_duration = to_point_at_end_of_duration_;
-      }
-    }
-
     float animated_x = 0;
     AnimateAdditiveNumber(parameters, percentage, repeat_count, from_point_.X(),
-                          to_point_.X(), to_point_at_end_of_duration.X(),
+                          to_point_.X(), to_point_at_end_of_duration_.X(),
                           animated_x);
 
     float animated_y = 0;
     AnimateAdditiveNumber(parameters, percentage, repeat_count, from_point_.Y(),
-                          to_point_.Y(), to_point_at_end_of_duration.Y(),
+                          to_point_.Y(), to_point_at_end_of_duration_.Y(),
                           animated_y);
 
     transform->Translate(animated_x, animated_y);

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

@@ -60,8 +60,6 @@ class SVGAnimateMotionElement final : public SVGAnimationElement {
   enum RotateMode { kRotateAngle, kRotateAuto, kRotateAutoReverse };
   RotateMode GetRotateMode() const;
 
-  bool has_to_point_at_end_of_duration_;
-
   void UpdateAnimationMode() override;
 
   void InvalidateForAnimateMotionTransformChange(LayoutObject& target);

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

@@ -653,9 +653,11 @@ SMILAnimationEffectParameters SVGAnimationElement::ComputeEffectParameters()
     const {
   SMILAnimationEffectParameters parameters;
   parameters.is_discrete = GetCalcMode() == kCalcModeDiscrete;
-  parameters.is_to_animation = GetAnimationMode() == kToAnimation;
+  // 'to'-animations are neither additive nor cumulative.
+  if (GetAnimationMode() != kToAnimation) {
     parameters.is_additive = IsAdditive() || GetAnimationMode() == kByAnimation;
     parameters.is_cumulative = IsAccumulated();
+  }
   return parameters;
 }
 

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

@@ -121,8 +121,7 @@ void SVGLengthList::CalculateAnimatedValue(
   uint32_t to_at_end_of_duration_list_size =
       to_at_end_of_duration_list->length();
 
-  if (!AdjustFromToListValues(from_list, to_list, percentage,
-                              parameters.is_to_animation))
+  if (!AdjustFromToListValues(from_list, to_list, percentage))
     return;
 
   for (uint32_t i = 0; i < to_length_list_size; ++i) {

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

@@ -91,8 +91,7 @@ void SVGNumberList::CalculateAnimatedValue(
   uint32_t to_at_end_of_duration_list_size =
       to_at_end_of_duration_list->length();
 
-  if (!AdjustFromToListValues(from_list, to_list, percentage,
-                              parameters.is_to_animation))
+  if (!AdjustFromToListValues(from_list, to_list, percentage))
     return;
 
   for (uint32_t i = 0; i < to_list_size; ++i) {

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

@@ -139,37 +139,25 @@ void SVGPath::CalculateAnimatedValue(
     return;
 
   const auto& from = To<SVGPath>(*from_value);
-  const SVGPathByteStream* from_stream = &from.ByteStream();
-
-  std::unique_ptr<SVGPathByteStream> copy;
-  if (parameters.is_to_animation) {
-    copy = ByteStream().Clone();
-    from_stream = copy.get();
-  }
+  const SVGPathByteStream& from_stream = from.ByteStream();
 
   // If the 'from' value is given and it's length doesn't match the 'to' value
   // list length, fallback to a discrete animation.
-  if (from_stream->size() != to_stream.size() && from_stream->size()) {
-    if (percentage < 0.5) {
-      if (!parameters.is_to_animation) {
-        path_value_ = from.PathValue();
+  if (from_stream.size() != to_stream.size() && from_stream.size()) {
+    // If this is a 'to' animation, the "from" value will be the same
+    // object as this object, so this will be a no-op but shouldn't
+    // clobber the object.
+    path_value_ = percentage < 0.5 ? from.PathValue() : to.PathValue();
     return;
   }
-    } else {
-      path_value_ = to.PathValue();
-      return;
-    }
-  }
 
+  // If this is a 'to' animation, the "from" value will be the same
+  // object as this object, so make sure to update the state of this
+  // object as the last thing to avoid clobbering the result. As long
+  // as all intermediate results are computed into |new_stream| that
+  // should be unproblematic.
   std::unique_ptr<SVGPathByteStream> new_stream =
-      BlendPathByteStreams(*from_stream, to_stream, percentage);
-
-  if (!parameters.is_to_animation) {
-    // Handle additive='sum'.
-    if (parameters.is_additive) {
-      new_stream =
-          ConditionallyAddPathByteStreams(std::move(new_stream), ByteStream());
-    }
+      BlendPathByteStreams(from_stream, to_stream, percentage);
 
   // Handle accumulate='sum'.
   if (repeat_count && parameters.is_cumulative) {
@@ -177,7 +165,13 @@ void SVGPath::CalculateAnimatedValue(
         std::move(new_stream),
         To<SVGPath>(to_at_end_of_duration_value)->ByteStream(), repeat_count);
   }
+
+  // Handle additive='sum'.
+  if (parameters.is_additive) {
+    new_stream =
+        ConditionallyAddPathByteStreams(std::move(new_stream), ByteStream());
   }
+
   path_value_ = MakeGarbageCollected<CSSPathValue>(std::move(new_stream));
 }
 

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

@@ -105,8 +105,7 @@ void SVGPointList::CalculateAnimatedValue(
   uint32_t to_at_end_of_duration_list_size =
       to_at_end_of_duration_list->length();
 
-  if (!AdjustFromToListValues(from_list, to_list, percentage,
-                              parameters.is_to_animation))
+  if (!AdjustFromToListValues(from_list, to_list, percentage))
     return;
 
   for (uint32_t i = 0; i < to_point_list_size; ++i) {

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

@@ -468,10 +468,17 @@ void SVGTransformList::CalculateAnimatedValue(
       SVGTransformDistance(effective_from, to_transform)
           .ScaledDistance(percentage)
           .AddToSVGTransform(effective_from);
-  if (parameters.is_to_animation) {
-    Clear();
-    Append(current_transform);
-    return;
+
+  // Handle accumulation.
+  if (repeat_count && parameters.is_cumulative) {
+    SVGTransform* effective_to_at_end =
+        !to_at_end_of_duration_list->IsEmpty()
+            ? to_at_end_of_duration_list->at(0)
+            : MakeGarbageCollected<SVGTransform>(
+                  to_transform->TransformType(),
+                  SVGTransform::kConstructZeroTransform);
+    current_transform = SVGTransformDistance::AddSVGTransforms(
+        current_transform, effective_to_at_end, repeat_count);
   }
 
   // If additive, we accumulate into (append to) the underlying value.
@@ -482,18 +489,7 @@ void SVGTransformList::CalculateAnimatedValue(
       Clear();
   }
 
-  if (repeat_count && parameters.is_cumulative) {
-    SVGTransform* effective_to_at_end =
-        !to_at_end_of_duration_list->IsEmpty()
-            ? to_at_end_of_duration_list->at(0)
-            : MakeGarbageCollected<SVGTransform>(
-                  to_transform->TransformType(),
-                  SVGTransform::kConstructZeroTransform);
-    Append(SVGTransformDistance::AddSVGTransforms(
-        current_transform, effective_to_at_end, repeat_count));
-  } else {
   Append(current_transform);
-  }
 }
 
 float SVGTransformList::CalculateDistance(SVGPropertyBase* to_value,