Separate animation argument validation from interval update

Animation arguments/value computations shouldn't be tied to interval
update, it should only "react" to progress.
Turn the |animation_valid_| bool into an tristate (enum) and refactor
the validation of animation parameters to suit. The UpdateAnimation(...)
override now performs the validation (if needed).
Implement WillChangeAnimationTarget() for SVGAnimationElement and
invalidate animations there. Then fold
SVGAnimateElement::ResetCachedAnimationState into the corresponding
override in SVGAnimateElement, removing the calls that is now handled
by the superclass.

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

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

@@ -139,8 +139,6 @@ class CORE_EXPORT SVGSMILElement : public SVGElement, public SVGTests {
   virtual void WillChangeAnimationTarget();
   virtual void DidChangeAnimationTarget();
 
-  virtual void StartedActiveInterval();
-
   QualifiedName attribute_name_;
 
  private:
@@ -148,6 +146,7 @@ class CORE_EXPORT SVGSMILElement : public SVGElement, public SVGTests {
   void ClearResourceAndEventBaseReferences();
   void ClearConditions();
 
+  void StartedActiveInterval();
   void EndedActiveInterval();
   virtual void UpdateAnimation(float percent,
                                unsigned repeat,

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

@@ -550,7 +550,9 @@ void SVGAnimateElement::WillChangeAnimationTarget() {
   SVGAnimationElement::WillChangeAnimationTarget();
   // Should be cleared by the above.
   DCHECK(!animated_value_);
-  ResetCachedAnimationState();
+  from_property_.Clear();
+  to_property_.Clear();
+  to_at_end_of_duration_property_.Clear();
 }
 
 void SVGAnimateElement::DidChangeAnimationTarget() {
@@ -582,14 +584,6 @@ void SVGAnimateElement::SetAttributeType(
   DidChangeAnimationTarget();
 }
 
-void SVGAnimateElement::ResetCachedAnimationState() {
-  DCHECK(!animated_value_);
-  InvalidatedValuesCache();
-  from_property_.Clear();
-  to_property_.Clear();
-  to_at_end_of_duration_property_.Clear();
-}
-
 void SVGAnimateElement::Trace(blink::Visitor* visitor) {
   visitor->Trace(from_property_);
   visitor->Trace(to_property_);

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

@@ -90,8 +90,6 @@ class CORE_EXPORT SVGAnimateElement : public SVGAnimationElement {
                            stringsShouldNotSupportAddition);
 
  private:
-  void ResetCachedAnimationState();
-
   bool ShouldApplyAnimation(const SVGElement& target_element) const;
 
   void SetAttributeType(const AtomicString&);

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

@@ -38,7 +38,7 @@ namespace blink {
 SVGAnimationElement::SVGAnimationElement(const QualifiedName& tag_name,
                                          Document& document)
     : SVGSMILElement(tag_name, document),
-      animation_valid_(false),
+      animation_valid_(AnimationValidity::kUnknown),
       calc_mode_(kCalcModeLinear),
       animation_mode_(kNoAnimation) {
   UseCounter::Count(document, WebFeature::kSVGAnimationElement);
@@ -237,11 +237,17 @@ void SVGAnimationElement::InvalidatedValuesCache() {
 
 void SVGAnimationElement::AnimationAttributeChanged() {
   // Assumptions may not hold after an attribute change.
-  animation_valid_ = false;
+  animation_valid_ = AnimationValidity::kUnknown;
   InvalidatedValuesCache();
   SetInactive();
 }
 
+void SVGAnimationElement::WillChangeAnimationTarget() {
+  SVGSMILElement::WillChangeAnimationTarget();
+  animation_valid_ = AnimationValidity::kUnknown;
+  InvalidatedValuesCache();
+}
+
 float SVGAnimationElement::getStartTime(ExceptionState& exception_state) const {
   SMILTime start_time = IntervalBegin();
   if (!start_time.IsFinite()) {
@@ -472,7 +478,7 @@ void SVGAnimationElement::CurrentValuesForValuesAnimation(
     String& from,
     String& to) {
   unsigned values_count = values_.size();
-  DCHECK(animation_valid_);
+  DCHECK_EQ(animation_valid_, AnimationValidity::kValid);
   DCHECK_GE(values_count, 1u);
 
   if (percent == 1 || values_count == 1) {
@@ -528,18 +534,14 @@ void SVGAnimationElement::CurrentValuesForValuesAnimation(
   }
 }
 
-void SVGAnimationElement::StartedActiveInterval() {
-  SVGSMILElement::StartedActiveInterval();
-
-  animation_valid_ = false;
-
+bool SVGAnimationElement::CheckAnimationParameters() {
   if (!IsValid() || !HasValidTarget())
-    return;
+    return false;
 
   // These validations are appropriate for all animation modes.
   if (FastHasAttribute(svg_names::kKeyPointsAttr) &&
       key_points_.size() != KeyTimes().size())
-    return;
+    return false;
 
   AnimationMode animation_mode = GetAnimationMode();
   CalcMode calc_mode = GetCalcMode();
@@ -552,34 +554,36 @@ void SVGAnimationElement::StartedActiveInterval() {
          values_.size() - 1 != splines_count) ||
         (FastHasAttribute(svg_names::kKeyTimesAttr) &&
          KeyTimes().size() - 1 != splines_count))
-      return;
+      return false;
   }
 
   String from = FromValue();
   String to = ToValue();
   String by = ByValue();
   if (animation_mode == kNoAnimation)
-    return;
+    return false;
   if ((animation_mode == kFromToAnimation ||
        animation_mode == kFromByAnimation || animation_mode == kToAnimation ||
        animation_mode == kByAnimation) &&
       (FastHasAttribute(svg_names::kKeyPointsAttr) &&
        FastHasAttribute(svg_names::kKeyTimesAttr) &&
        (KeyTimes().size() < 2 || KeyTimes().size() != key_points_.size())))
-    return;
-  if (animation_mode == kFromToAnimation) {
-    animation_valid_ = CalculateFromAndToValues(from, to);
-  } else if (animation_mode == kToAnimation) {
+    return false;
+  if (animation_mode == kFromToAnimation)
+    return CalculateFromAndToValues(from, to);
+  if (animation_mode == kToAnimation) {
     // For to-animations the from value is the current accumulated value from
     // lower priority animations.
     // The value is not static and is determined during the animation.
-    animation_valid_ = CalculateFromAndToValues(g_empty_string, to);
-  } else if (animation_mode == kFromByAnimation) {
-    animation_valid_ = CalculateFromAndByValues(from, by);
-  } else if (animation_mode == kByAnimation) {
-    animation_valid_ = CalculateFromAndByValues(g_empty_string, by);
-  } else if (animation_mode == kValuesAnimation) {
-    animation_valid_ =
+    return CalculateFromAndToValues(g_empty_string, to);
+  }
+  if (animation_mode == kFromByAnimation)
+    return CalculateFromAndByValues(from, by);
+  if (animation_mode == kByAnimation)
+    return CalculateFromAndByValues(g_empty_string, by);
+  if (animation_mode == kValuesAnimation) {
+    // o_O - TODO(fs): move this to a helper function.
+    bool animation_valid =
         values_.size() >= 1 &&
         (calc_mode == kCalcModePaced ||
          !FastHasAttribute(svg_names::kKeyTimesAttr) ||
@@ -593,25 +597,38 @@ void SVGAnimationElement::StartedActiveInterval() {
           key_splines_.size() == key_points_.size() - 1)) &&
         (!FastHasAttribute(svg_names::kKeyPointsAttr) ||
          (KeyTimes().size() > 1 && KeyTimes().size() == key_points_.size()));
-    if (animation_valid_)
-      animation_valid_ = CalculateToAtEndOfDurationValue(values_.back());
-    if (calc_mode == kCalcModePaced && animation_valid_)
+    if (animation_valid)
+      animation_valid = CalculateToAtEndOfDurationValue(values_.back());
+    if (calc_mode == kCalcModePaced && animation_valid)
       CalculateKeyTimesForCalcModePaced();
-  } else if (animation_mode == kPathAnimation) {
-    animation_valid_ =
-        calc_mode == kCalcModePaced ||
-        !FastHasAttribute(svg_names::kKeyPointsAttr) ||
-        (KeyTimes().size() > 1 && KeyTimes().size() == key_points_.size());
+    return animation_valid;
   }
-
-  if (animation_valid_ && (IsAdditive() || IsAccumulated()))
-    UseCounter::Count(&GetDocument(), WebFeature::kSVGSMILAdditiveAnimation);
+  if (animation_mode == kPathAnimation) {
+    return calc_mode == kCalcModePaced ||
+           !FastHasAttribute(svg_names::kKeyPointsAttr) ||
+           (KeyTimes().size() > 1 && KeyTimes().size() == key_points_.size());
+  }
+  return false;
 }
 
 void SVGAnimationElement::UpdateAnimation(float percent,
                                           unsigned repeat_count,
                                           SVGSMILElement* result_element) {
-  if (!animation_valid_ || !targetElement())
+  if (animation_valid_ == AnimationValidity::kUnknown) {
+    if (CheckAnimationParameters()) {
+      animation_valid_ = AnimationValidity::kValid;
+
+      if (IsAdditive() || IsAccumulated()) {
+        UseCounter::Count(&GetDocument(),
+                          WebFeature::kSVGSMILAdditiveAnimation);
+      }
+    } else {
+      animation_valid_ = AnimationValidity::kInvalid;
+    }
+  }
+  DCHECK_NE(animation_valid_, AnimationValidity::kUnknown);
+
+  if (animation_valid_ != AnimationValidity::kValid || !targetElement())
     return;
 
   float effective_percent;
@@ -623,9 +640,10 @@ void SVGAnimationElement::UpdateAnimation(float percent,
     CurrentValuesForValuesAnimation(percent, effective_percent, from, to);
     if (from != last_values_animation_from_ ||
         to != last_values_animation_to_) {
-      animation_valid_ = CalculateFromAndToValues(from, to);
-      if (!animation_valid_)
+      if (!CalculateFromAndToValues(from, to)) {
+        animation_valid_ = AnimationValidity::kInvalid;
         return;
+      }
       last_values_animation_from_ = from;
       last_values_animation_to_ = to;
     }

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

@@ -121,7 +121,6 @@ class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
   String FromValue() const;
 
   // from SVGSMILElement
-  void StartedActiveInterval() override;
   void UpdateAnimation(float percent,
                        unsigned repeat,
                        SVGSMILElement* result_element) override;
@@ -144,10 +143,12 @@ class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
 
   void InvalidatedValuesCache();
   void AnimationAttributeChanged() override;
+  void WillChangeAnimationTarget() override;
 
  private:
   bool IsValid() const final { return SVGTests::IsValid(); }
 
+  bool CheckAnimationParameters();
   virtual bool CalculateToAtEndOfDurationValue(
       const String& to_at_end_of_duration_string) = 0;
   virtual bool CalculateFromAndToValues(const String& from_string,
@@ -186,7 +187,12 @@ class CORE_EXPORT SVGAnimationElement : public SVGSMILElement {
 
   void SetCalcMode(const AtomicString&);
 
-  bool animation_valid_;
+  enum class AnimationValidity : unsigned char {
+    kUnknown,
+    kValid,
+    kInvalid,
+  };
+  AnimationValidity animation_valid_;
   bool use_paced_key_times_;
 
   Vector<String> values_;