Simplify and test SVGInterpolation effect application

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

Index: third_party/WebKit/Source/core/svg/SVGElement.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGElement.cpp b/third_party/WebKit/Source/core/svg/SVGElement.cpp
index d545acb3ae5e94ea5a436c517a5672d75faff998..dfa808348e6ad22892745261b743dc3d51945bc8 100644
--- a/third_party/WebKit/Source/core/svg/SVGElement.cpp
+++ b/third_party/WebKit/Source/core/svg/SVGElement.cpp
@@ -243,25 +243,15 @@ void SVGElement::setWebAnimationsPending()
     document().accessSVGExtensions().addWebAnimationsPendingSVGElement(*this);
 }
 
-template<typename T>
-static void updateInstancesAnimatedAttribute(SVGElement* element, const QualifiedName& attribute, T callback)
-{
-    SVGElement::InstanceUpdateBlocker blocker(element);

[fs, 2015/12/09 12:19:08]

So, the "benefit" of the scheme you're removing can be explained with this
single line (I hope...) Here we say "prevent the regular instance invalidation
logic from doing anything - we will make sure the instances are up-to-date
'manually'". So the "pro" is that we don't need to tear down and recreate the
instances for each mutation (or, slightly more correct, "frame"). The "con" is
of course complexity...
We use this pattern in the SMIL code (SVGAnimateElement,
SVGAnimatedTypeAnimator) for the above "pro" (AFAIK at least). It's open-coded
in all the places though, so if we could have it once (in SVGElement, preferably
folding SVGAnimateElement::findElementInstances into it) that would be an
improvement too I think. Doesn't necessarily have to imply you doing any of the
legwork either =)

[alancutter (OOO until 2018), 2015/12/09 22:14:11]

Ah so it's an optimisation, that makes sense why it behaved correctly. Thanks
for the explanation, I'll make this patch just the test and the duplicate code
removal.

-    for (SVGElement* instance : SVGAnimateElement::findElementInstances(element)) {
-        RefPtrWillBeRawPtr<SVGAnimatedPropertyBase> animatedProperty = instance->propertyFromAttribute(attribute);
-        if (animatedProperty) {
-            callback(*animatedProperty);
-            instance->invalidateSVGAttributes();
-            instance->svgAttributeChanged(attribute);
-        }
-    }
-}
-
 void SVGElement::setWebAnimatedAttribute(const QualifiedName& attribute, PassRefPtrWillBeRawPtr<SVGPropertyBase> value)
 {
-    updateInstancesAnimatedAttribute(this, attribute, [&value](SVGAnimatedPropertyBase& animatedProperty) {
-        animatedProperty.setAnimatedValue(value.get());
-    });
+    RefPtrWillBeRawPtr<SVGAnimatedPropertyBase> animatedProperty = propertyFromAttribute(attribute);
+    if (!animatedProperty)
+        return;
+
+    animatedProperty->setAnimatedValue(value.get());
+    invalidateSVGAttributes();
+    svgAttributeChanged(attribute);
     ensureSVGRareData()->webAnimatedAttributes().add(&attribute);
 }
 
@@ -269,11 +259,17 @@ void SVGElement::clearWebAnimatedAttributes()
 {
     if (!hasSVGRareData())
         return;
+
     for (const QualifiedName* attribute : svgRareData()->webAnimatedAttributes()) {
-        updateInstancesAnimatedAttribute(this, *attribute, [](SVGAnimatedPropertyBase& animatedProperty) {
-            animatedProperty.animationEnded();
-        });
+        RefPtrWillBeRawPtr<SVGAnimatedPropertyBase> animatedProperty = propertyFromAttribute(*attribute);
+        if (!animatedProperty)
+            continue;
+
+        animatedProperty->animationEnded();
+        invalidateSVGAttributes();
+        svgAttributeChanged(*attribute);
     }
+
     svgRareData()->webAnimatedAttributes().clear();
 }