Simplify and test SVGInterpolation effect application

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

 /*
  * Copyright (C) 2004, 2005, 2006, 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2008 Rob Buis <buis@kde.org>
  * Copyright (C) 2008 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) 2009 Cameron McCormack <cam@mcc.id.au>
  *
  * 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
@@ skipping 225 matching lines @@
 {
     if (hasSVGRareData())
         svgRareData()->setInstanceUpdatesBlocked(value);
 }
 
 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);
 }
 
 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();
 }
 
 AffineTransform SVGElement::localCoordinateSpaceTransform(CTMScope) const
 {
     // To be overriden by SVGGraphicsElement (or as special case SVGTextElement and SVGPatternElement)
     return AffineTransform();
 }
 
 Node::InsertionNotificationRequest SVGElement::insertedInto(ContainerNode* rootParent)
@@ skipping 901 matching lines @@
     Element::trace(visitor);
 }
 
 const AtomicString& SVGElement::eventParameterName()
 {
     DEFINE_STATIC_LOCAL(const AtomicString, evtString, ("evt", AtomicString::ConstructFromLiteral));
     return evtString;
 }
 
 } // namespace blink