Fold SMIL animation value application helpers and simplify

third_party/WebKit/Source/core/svg/SVGAnimateElement.cpp

 /*
  * Copyright (C) 2004, 2005 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006 Rob Buis <buis@kde.org>
  * Copyright (C) 2008 Apple Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 188 matching lines @@
     DCHECK_EQ(shouldApply, ApplyCSSAnimation);
 
     // CSS properties animation code-path.
     String baseValue;
     DCHECK(isTargetAttributeCSSProperty(targetElement, attributeName));
     computeCSSPropertyValue(targetElement, cssPropertyID(attributeName.localName()), baseValue);
 
     m_animatedProperty = m_animator.constructFromString(baseValue);
 }
 
-static inline void applyCSSPropertyToTargetAndInstances(SVGElement* targetElement, const QualifiedName& attributeName, const String& valueAsString)
+static bool targetIsUsable(SVGElement* targetElement, const QualifiedName& attribute)
 {
-    ASSERT(targetElement);
-    if (attributeName == anyQName() || !targetElement->isConnected() || !targetElement->parentNode())
-        return;
-
-    CSSPropertyID id = cssPropertyID(attributeName.localName());
-    MutableStylePropertySet* propertySet = targetElement->ensureAnimatedSMILStyleProperties();
-    if (!propertySet->setProperty(id, valueAsString, false, 0))
-        return;
-
-    targetElement->setNeedsStyleRecalc(LocalStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Animation));
-}
-
-static inline void removeCSSPropertyFromTargetAndInstances(SVGElement* targetElement, const QualifiedName& attributeName)
-{
-    ASSERT(targetElement);
-    if (attributeName == anyQName() || !targetElement->isConnected() || !targetElement->parentNode())
-        return;
-
-    CSSPropertyID id = cssPropertyID(attributeName.localName());
-    targetElement->ensureAnimatedSMILStyleProperties()->removeProperty(id);
-    targetElement->setNeedsStyleRecalc(LocalStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Animation));
-}
-
-static inline void notifyTargetAndInstancesAboutAnimValChange(SVGElement* targetElement, const QualifiedName& attributeName)
-{
-    ASSERT(targetElement);
-    if (attributeName == anyQName() || !targetElement->isConnected() || !targetElement->parentNode())
-        return;
-
-    targetElement->invalidateAnimatedAttribute(attributeName);
+    return attribute != anyQName()
+        && targetElement->isConnected() && targetElement->parentNode();
 }
 
 void SVGAnimateElement::clearAnimatedType()
 {
     if (!m_animatedProperty)
         return;
 
     // The animated property lock is held for the "result animation" (see SMILTimeContainer::updateAnimations())
     // while we're processing an animation group. We will very likely crash later if we clear the animated type
     // while the lock is held. See crbug.com/581546.
     DCHECK(!animatedTypeIsLocked());
 
     SVGElement* targetElement = this->targetElement();
     if (!targetElement) {
         m_animatedProperty.clear();
         return;
     }
 
     ShouldApplyAnimationType shouldApply = shouldApplyAnimation(targetElement, attributeName());
-    if (shouldApply == ApplyXMLandCSSAnimation) {
-        removeCSSPropertyFromTargetAndInstances(targetElement, attributeName());
-    } else if (m_animator.isAnimatingCSSProperty()) {
+    if (shouldApply == ApplyXMLandCSSAnimation || m_animator.isAnimatingCSSProperty()) {
         // CSS properties animation code-path.
-        removeCSSPropertyFromTargetAndInstances(targetElement, attributeName());
-        m_animatedProperty.clear();
-        m_animator.clear();
-        return;
+        if (targetIsUsable(targetElement, attributeName())) {
+            CSSPropertyID id = cssPropertyID(attributeName().localName());
+            targetElement->ensureAnimatedSMILStyleProperties()->removeProperty(id);
+            targetElement->setNeedsStyleRecalc(LocalStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Animation));
+        }
     }
-
-    // SVG DOM animVal animation code-path.
-    if (m_animatedProperty) {
+    if (shouldApply == ApplyXMLandCSSAnimation || m_animator.isAnimatingSVGDom()) {
+        // SVG DOM animVal animation code-path.
         m_animator.stopAnimValAnimation();
-        notifyTargetAndInstancesAboutAnimValChange(targetElement, attributeName());
+        if (targetIsUsable(targetElement, attributeName()))

[pdr., 2016/08/26 01:28:12]

The attribute check is sort of unrelated to whether the target is usable. WDYT
of pulling "attribute != anyQName()" out and relying on shouldApply which has a
similar check?

Something like:
if (shouldApply != DontApplyAnimation && targetIsUsable(targetElement))
   ...

[fs, 2016/08/26 08:45:27]

Well, we could make that "shouldApply != DontApplyAnimation &&
targetElement->parentNode()" even (shouldApplyAnimation checks inActiveDocument
too), but technically speaking it's a behavioral change. I'd be happy to do this
as a follow up though (probably by merging targetIsUsable into
shouldApplyAnimation and replace checks with the first part of the conjunction
above.)

(In general I consider "target" in this context to be the <element, attribute>
pair.)

+            targetElement->invalidateAnimatedAttribute(attributeName());
     }
 
     m_animatedProperty.clear();
     m_animator.clear();
 }
 
 void SVGAnimateElement::applyResultsToTarget()
 {
     ASSERT(animatedPropertyType() != AnimatedTransformList || isSVGAnimateTransformElement(*this));
     ASSERT(animatedPropertyType() != AnimatedUnknown);
 
     // Early exit if our animated type got destructed by a previous endedActiveInterval().
     if (!m_animatedProperty)
         return;
 
     // We do update the style and the animation property independent of each other.
     ShouldApplyAnimationType shouldApply = shouldApplyAnimation(targetElement(), attributeName());
-    if (shouldApply == ApplyXMLandCSSAnimation) {
-        applyCSSPropertyToTargetAndInstances(targetElement(), attributeName(), m_animatedProperty->valueAsString());
-    } else if (m_animator.isAnimatingCSSProperty()) {
+    DCHECK(targetElement());
+    if (!targetIsUsable(targetElement(), attributeName()))
+        return;
+    if (shouldApply == ApplyXMLandCSSAnimation || m_animator.isAnimatingCSSProperty()) {
         // CSS properties animation code-path.
-        // Convert the result of the animation to a String and apply it as CSS property on the target & all instances.
-        applyCSSPropertyToTargetAndInstances(targetElement(), attributeName(), m_animatedProperty->valueAsString());
-        return;
+        // Convert the result of the animation to a String and apply it as CSS property on the target.
+        CSSPropertyID id = cssPropertyID(attributeName().localName());
+        MutableStylePropertySet* propertySet = targetElement()->ensureAnimatedSMILStyleProperties();
+        if (propertySet->setProperty(id, m_animatedProperty->valueAsString(), false, 0))
+            targetElement()->setNeedsStyleRecalc(LocalStyleChange, StyleChangeReasonForTracing::create(StyleChangeReason::Animation));
     }
-
-    // SVG DOM animVal animation code-path.
-    // At this point the SVG DOM values are already changed, unlike for CSS.
-    // We only have to trigger update notifications here.
-    notifyTargetAndInstancesAboutAnimValChange(targetElement(), attributeName());
+    if (shouldApply == ApplyXMLandCSSAnimation || m_animator.isAnimatingSVGDom()) {
+        // SVG DOM animVal animation code-path.
+        // At this point the SVG DOM values are already changed, unlike for CSS.
+        // We only have to trigger update notifications here.
+        targetElement()->invalidateAnimatedAttribute(attributeName());
+    }
 }
 
 bool SVGAnimateElement::animatedPropertyTypeSupportsAddition()
 {
     // Spec: http://www.w3.org/TR/SVG/animate.html#AnimationAttributesAndProperties.
     switch (animatedPropertyType()) {
     case AnimatedBoolean:
     case AnimatedEnumeration:
     case AnimatedPreserveAspectRatio:
     case AnimatedString:
@@ skipping 49 matching lines @@
 {
     visitor->trace(m_fromProperty);
     visitor->trace(m_toProperty);
     visitor->trace(m_toAtEndOfDurationProperty);
     visitor->trace(m_animatedProperty);
     visitor->trace(m_animator);
     SVGAnimationElement::trace(visitor);
 }
 
 } // namespace blink