Unify SVG's animation and target tracking systems.
authorpdr@google.com <pdr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 00:21:57 +0000 (00:21 +0000)
committerpdr@google.com <pdr@google.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 00:21:57 +0000 (00:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=102655

Reviewed by Dirk Schulze.

Source/WebCore:

This patch unifies our animation target tracking system and regular target tracking system.
This simplifies the code, fixes a bug, and cleans up a historically security-sensitive area.

Background: When <use>, <mpath>, <animate>, etc. reference another element using
xlink:href="#id", we need to track when #id changes to #otherId, when #id is removed, etc.
This bookkeeping of element -> target is done in SVGDocumentExtensions. Additionally, when
a target changes that causes layout (e.g., rect.x is changed), all dependent elements with
renderers are notified (<animate> has no renderer and will not use this).

Previously, xlink:href changes were lazily resolved when targetElement() was called, target
changes were tracked using the animation tracking framework, and pending targets did not
work (e.g., <animate xlink:href="#p"><!--animate is now pending #p --><rect id="p"/>).

After this patch, we no longer lazily resolve targetElement() but instead change it when
our xlink:href attribute changes. Instead of using the animation tracking framework in
SVGDocumentExtensions, we now use the regular target tracking framework. Lastly, by using
the regular target tracking framework we are able to hook into the pending resource handling
which fixes a bug (see the test).

A test has been added to test that the order of animation elements does not matter. A second
test has been added to show we do not regress a pending-while-pending case.

Tests: svg/animations/svg-animation-order.html
       svg/custom/svg-pending-twice.html

* svg/SVGAnimateElement.cpp:
(WebCore::SVGAnimateElement::setTargetElement):

    setTargetElement and setAttributeName now work similarly. When the corresponding attribute
    changes, we update our internal state (target or attributeName) and save it instead of
    looking these values up on each iteration.

(WebCore::SVGAnimateElement::setAttributeName):
(WebCore):
(WebCore::SVGAnimateElement::resetAnimatedPropertyType):
* svg/SVGAnimateElement.h:
(SVGAnimateElement):
* svg/SVGAnimationElement.cpp:
(WebCore::SVGAnimationElement::setAttributeType):
(WebCore::SVGAnimationElement::setTargetElement):
(WebCore::SVGAnimationElement::setAttributeName):
* svg/SVGAnimationElement.h:
(SVGAnimationElement):
* svg/SVGDocumentExtensions.cpp:
(WebCore::SVGDocumentExtensions::~SVGDocumentExtensions):
(WebCore):
* svg/SVGDocumentExtensions.h:
(SVGDocumentExtensions):
* svg/SVGElement.cpp:
(WebCore::SVGElement::~SVGElement):
(WebCore::SVGElement::removedFrom):
(WebCore::SVGElement::attributeChanged):
* svg/SVGElementInstance.cpp:
(WebCore::SVGElementInstance::invalidateAllInstancesOfElement):

    This can be removed after r131631 landed.

* svg/SVGMPathElement.cpp:
(WebCore::SVGMPathElement::buildPendingResource):
* svg/SVGTextPathElement.cpp:
(WebCore::SVGTextPathElement::buildPendingResource):

    A bug was discovered in review with our resource tracking in a pending-while-pending
    case. SVGMpathElement and SVGTextPathElement have been updated to fix this as well.

* svg/animation/SVGSMILElement.cpp:

    The changes in SVGSMILElement should look very similar to SVGFEImageElement,
    SVGMPathElement, etc. The idea is to build pending resources when added or
    removed from the document, or when the href attribute changes.

(WebCore::SVGSMILElement::~SVGSMILElement):
(WebCore):
(WebCore::SVGSMILElement::clearResourceReferences):
(WebCore::SVGSMILElement::buildPendingResource):
(WebCore::SVGSMILElement::insertedInto):
(WebCore::SVGSMILElement::removedFrom):
(WebCore::SVGSMILElement::svgAttributeChanged):
(WebCore::SVGSMILElement::setAttributeName):
(WebCore::SVGSMILElement::setTargetElement):
* svg/animation/SVGSMILElement.h:
(WebCore):
(WebCore::SVGSMILElement::targetElement):
(SVGSMILElement):

LayoutTests:

* svg/animations/svg-animation-order-expected.html: Added.
* svg/animations/svg-animation-order.html: Added.
* svg/custom/svg-pending-twice-expected.html: Added.
* svg/custom/svg-pending-twice.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@136906 268f45cc-cd09-0410-ab3c-d52691b4dbfc

18 files changed:
LayoutTests/ChangeLog
LayoutTests/svg/animations/svg-animation-order-expected.html [new file with mode: 0644]
LayoutTests/svg/animations/svg-animation-order.html [new file with mode: 0644]
LayoutTests/svg/custom/svg-pending-twice-expected.html [new file with mode: 0644]
LayoutTests/svg/custom/svg-pending-twice.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/SVGAnimateElement.cpp
Source/WebCore/svg/SVGAnimateElement.h
Source/WebCore/svg/SVGAnimationElement.cpp
Source/WebCore/svg/SVGAnimationElement.h
Source/WebCore/svg/SVGDocumentExtensions.cpp
Source/WebCore/svg/SVGDocumentExtensions.h
Source/WebCore/svg/SVGElement.cpp
Source/WebCore/svg/SVGElementInstance.cpp
Source/WebCore/svg/SVGMPathElement.cpp
Source/WebCore/svg/SVGTextPathElement.cpp
Source/WebCore/svg/animation/SVGSMILElement.cpp
Source/WebCore/svg/animation/SVGSMILElement.h

index 6adcb75..80f04e6 100644 (file)
@@ -1,3 +1,15 @@
+2012-12-06  Philip Rogers  <pdr@google.com>
+
+        Unify SVG's animation and target tracking systems.
+        https://bugs.webkit.org/show_bug.cgi?id=102655
+
+        Reviewed by Dirk Schulze.
+
+        * svg/animations/svg-animation-order-expected.html: Added.
+        * svg/animations/svg-animation-order.html: Added.
+        * svg/custom/svg-pending-twice-expected.html: Added.
+        * svg/custom/svg-pending-twice.html: Added.
+
 2012-12-06  James Simonsen  <simonjam@chromium.org>
 
         [Chromium] Enable Resource Timing and User Timing
diff --git a/LayoutTests/svg/animations/svg-animation-order-expected.html b/LayoutTests/svg/animations/svg-animation-order-expected.html
new file mode 100644 (file)
index 0000000..a20f04e
--- /dev/null
@@ -0,0 +1,9 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p id="description">Test that the order of elements does not affect animations.</p>
+<svg id="svg" width="300" height="300">
+  <rect x="0" y="0" width="100" height="100" fill="green"/>
+</svg>
+</body>
+</html>
diff --git a/LayoutTests/svg/animations/svg-animation-order.html b/LayoutTests/svg/animations/svg-animation-order.html
new file mode 100644 (file)
index 0000000..4ff728d
--- /dev/null
@@ -0,0 +1,30 @@
+<!DOCTYPE HTML>
+<html>
+<script>
+function runTest() {
+  if (window.testRunner)
+    testRunner.waitUntilDone();
+
+  // Defer to allow animations to run.
+  setTimeout(function() {
+    if (window.testRunner)
+      testRunner.notifyDone();
+  }, 0);
+}
+</script>
+<body onload="runTest()">
+<p id="description">Test that the order of elements does not affect animations.</p>
+<svg id="svg" width="300" height="300">
+  <!-- This red square should be covered up if we pass. -->
+  <rect x="0" y="0" width="100" height="100" fill="red"/>
+
+  <!-- Animate one green rectangle over the top half of the red square. -->
+  <rect id="rect1"  x="-100" y="0" width="100" height="50" fill="green" />
+  <set attributeName="x" xlink:href="#rect1" to="0"/>
+
+  <!-- Animate a second green rectangle over the bottom half of the red square. -->
+  <set attributeName="x" xlink:href="#rect2" to="0"/>
+  <rect id="rect2"  x="-100" y="50" width="100" height="50" fill="green" />
+</svg>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/svg-pending-twice-expected.html b/LayoutTests/svg/custom/svg-pending-twice-expected.html
new file mode 100644 (file)
index 0000000..131d548
--- /dev/null
@@ -0,0 +1,9 @@
+<!DOCTYPE HTML>
+<html>
+<body>
+<p id="description">Test that an animation with a pending resource can be changed to a second pending resource, then resolved correctly.</p>
+<svg id="svg" width="300" height="300">
+  <rect x="0" y="0" width="100" height="100" fill="green"/>
+</svg>
+</body>
+</html>
diff --git a/LayoutTests/svg/custom/svg-pending-twice.html b/LayoutTests/svg/custom/svg-pending-twice.html
new file mode 100644 (file)
index 0000000..72d6f63
--- /dev/null
@@ -0,0 +1,34 @@
+<!DOCTYPE HTML>
+<html>
+<script>
+function runTest() {
+  if (window.testRunner)
+    testRunner.waitUntilDone();
+
+  // The animation is already in a pending state, change it to another
+  // pending id (newId), then make the new id valid and check that
+  // the animation runs.
+  var rect = document.getElementById('rect');
+  var animate = document.getElementById('set');
+  animate.setAttributeNS("http://www.w3.org/1999/xlink", "href", "#newId");
+  rect.setAttribute('id', 'newId');
+
+  // Defer to allow animations to run.
+  setTimeout(function() {
+    if (window.testRunner)
+      testRunner.notifyDone();
+  }, 0);
+}
+</script>
+<body onload="runTest()">
+<p id="description">Test that an animation with a pending resource can be changed to a second pending resource, then resolved correctly.</p>
+<svg id="svg" width="300" height="300">
+  <!-- This red square should be covered up if we pass. -->
+  <rect x="0" y="0" width="100" height="100" fill="red"/>
+
+  <!-- This green square should end up covering the red square if we pass. -->
+  <rect id="rect" x="-100" y="0" width="100" height="100" fill="green"/>
+  <set id="set" xlink:href="#doesNotExist" attributeName="x" to="0" />
+</svg>
+</body>
+</html>
index 8029c5f..f1576c9 100644 (file)
@@ -1,3 +1,95 @@
+2012-12-06  Philip Rogers  <pdr@google.com>
+
+        Unify SVG's animation and target tracking systems.
+        https://bugs.webkit.org/show_bug.cgi?id=102655
+
+        Reviewed by Dirk Schulze.
+
+        This patch unifies our animation target tracking system and regular target tracking system.
+        This simplifies the code, fixes a bug, and cleans up a historically security-sensitive area.
+
+        Background: When <use>, <mpath>, <animate>, etc. reference another element using
+        xlink:href="#id", we need to track when #id changes to #otherId, when #id is removed, etc.
+        This bookkeeping of element -> target is done in SVGDocumentExtensions. Additionally, when
+        a target changes that causes layout (e.g., rect.x is changed), all dependent elements with
+        renderers are notified (<animate> has no renderer and will not use this).
+
+        Previously, xlink:href changes were lazily resolved when targetElement() was called, target
+        changes were tracked using the animation tracking framework, and pending targets did not
+        work (e.g., <animate xlink:href="#p"><!--animate is now pending #p --><rect id="p"/>).
+
+        After this patch, we no longer lazily resolve targetElement() but instead change it when
+        our xlink:href attribute changes. Instead of using the animation tracking framework in
+        SVGDocumentExtensions, we now use the regular target tracking framework. Lastly, by using
+        the regular target tracking framework we are able to hook into the pending resource handling
+        which fixes a bug (see the test).
+
+        A test has been added to test that the order of animation elements does not matter. A second
+        test has been added to show we do not regress a pending-while-pending case.
+
+        Tests: svg/animations/svg-animation-order.html
+               svg/custom/svg-pending-twice.html
+
+        * svg/SVGAnimateElement.cpp:
+        (WebCore::SVGAnimateElement::setTargetElement):
+
+            setTargetElement and setAttributeName now work similarly. When the corresponding attribute
+            changes, we update our internal state (target or attributeName) and save it instead of
+            looking these values up on each iteration.
+
+        (WebCore::SVGAnimateElement::setAttributeName):
+        (WebCore):
+        (WebCore::SVGAnimateElement::resetAnimatedPropertyType):
+        * svg/SVGAnimateElement.h:
+        (SVGAnimateElement):
+        * svg/SVGAnimationElement.cpp:
+        (WebCore::SVGAnimationElement::setAttributeType):
+        (WebCore::SVGAnimationElement::setTargetElement):
+        (WebCore::SVGAnimationElement::setAttributeName):
+        * svg/SVGAnimationElement.h:
+        (SVGAnimationElement):
+        * svg/SVGDocumentExtensions.cpp:
+        (WebCore::SVGDocumentExtensions::~SVGDocumentExtensions):
+        (WebCore):
+        * svg/SVGDocumentExtensions.h:
+        (SVGDocumentExtensions):
+        * svg/SVGElement.cpp:
+        (WebCore::SVGElement::~SVGElement):
+        (WebCore::SVGElement::removedFrom):
+        (WebCore::SVGElement::attributeChanged):
+        * svg/SVGElementInstance.cpp:
+        (WebCore::SVGElementInstance::invalidateAllInstancesOfElement):
+
+            This can be removed after r131631 landed.
+
+        * svg/SVGMPathElement.cpp:
+        (WebCore::SVGMPathElement::buildPendingResource):
+        * svg/SVGTextPathElement.cpp:
+        (WebCore::SVGTextPathElement::buildPendingResource):
+
+            A bug was discovered in review with our resource tracking in a pending-while-pending
+            case. SVGMpathElement and SVGTextPathElement have been updated to fix this as well.
+
+        * svg/animation/SVGSMILElement.cpp:
+
+            The changes in SVGSMILElement should look very similar to SVGFEImageElement,
+            SVGMPathElement, etc. The idea is to build pending resources when added or
+            removed from the document, or when the href attribute changes.
+
+        (WebCore::SVGSMILElement::~SVGSMILElement):
+        (WebCore):
+        (WebCore::SVGSMILElement::clearResourceReferences):
+        (WebCore::SVGSMILElement::buildPendingResource):
+        (WebCore::SVGSMILElement::insertedInto):
+        (WebCore::SVGSMILElement::removedFrom):
+        (WebCore::SVGSMILElement::svgAttributeChanged):
+        (WebCore::SVGSMILElement::setAttributeName):
+        (WebCore::SVGSMILElement::setTargetElement):
+        * svg/animation/SVGSMILElement.h:
+        (WebCore):
+        (WebCore::SVGSMILElement::targetElement):
+        (SVGSMILElement):
+
 2012-12-06  Jon Lee  <jonlee@apple.com>
 
         Retry snapshots if they are too empty
index 6955cde..c777f3f 100644 (file)
@@ -396,16 +396,26 @@ float SVGAnimateElement::calculateDistance(const String& fromString, const Strin
     return ensureAnimator()->calculateDistance(fromString, toString);
 }
 
-void SVGAnimateElement::targetElementWillChange(SVGElement* currentTarget, SVGElement* newTarget)
+void SVGAnimateElement::setTargetElement(SVGElement* target)
 {
-    SVGAnimationElement::targetElementWillChange(currentTarget, newTarget);
+    SVGAnimationElement::setTargetElement(target);
+    resetAnimatedPropertyType();
+}
 
+void SVGAnimateElement::setAttributeName(const QualifiedName& attributeName)
+{
+    SVGAnimationElement::setAttributeName(attributeName);
+    resetAnimatedPropertyType();
+}
+
+void SVGAnimateElement::resetAnimatedPropertyType()
+{
     ASSERT(!m_animatedType);
     m_fromType.clear();
     m_toType.clear();
     m_toAtEndOfDurationType.clear();
     m_animator.clear();
-    m_animatedPropertyType = newTarget ? determineAnimatedPropertyType(newTarget) : AnimatedString;
+    m_animatedPropertyType = targetElement() ? determineAnimatedPropertyType(targetElement()) : AnimatedString;
 }
 
 SVGAnimatedTypeAnimator* SVGAnimateElement::ensureAnimator()
index aaa5bd0..9f324c1 100644 (file)
@@ -45,6 +45,7 @@ protected:
 
     virtual void resetAnimatedType();
     virtual void clearAnimatedType(SVGElement* targetElement);
+
     virtual bool calculateToAtEndOfDurationValue(const String& toAtEndOfDurationString);
     virtual bool calculateFromAndToValues(const String& fromString, const String& toString);
     virtual bool calculateFromAndByValues(const String& fromString, const String& byString);
@@ -53,11 +54,13 @@ protected:
     virtual float calculateDistance(const String& fromString, const String& toString);
     virtual bool isAdditive() const OVERRIDE;
 
-    virtual void targetElementWillChange(SVGElement* currentTarget, SVGElement* oldTarget) OVERRIDE;
+    virtual void setTargetElement(SVGElement*) OVERRIDE;
+    virtual void setAttributeName(const QualifiedName&) OVERRIDE;
 
     AnimatedPropertyType m_animatedPropertyType;
 
 private:
+    void resetAnimatedPropertyType();
     SVGAnimatedTypeAnimator* ensureAnimator();
 
     virtual bool hasValidAttributeType();
index 3c20970..170de3b 100644 (file)
@@ -319,7 +319,7 @@ void SVGAnimationElement::setAttributeType(const AtomicString& attributeType)
         m_attributeType = AttributeTypeXML;
     else
         m_attributeType = AttributeTypeAuto;
-    checkInvalidCSSAttributeType(targetElement(DoNotResolveNewTarget));
+    checkInvalidCSSAttributeType(targetElement());
 }
 
 String SVGAnimationElement::toValue() const
@@ -677,18 +677,16 @@ void SVGAnimationElement::determinePropertyValueTypes(const String& from, const
         m_toPropertyValueType = InheritValue;
 }
 
-void SVGAnimationElement::targetElementWillChange(SVGElement* currentTarget, SVGElement* newTarget)
+void SVGAnimationElement::setTargetElement(SVGElement* target)
 {
-    SVGSMILElement::targetElementWillChange(currentTarget, newTarget);
-
-    checkInvalidCSSAttributeType(newTarget);
+    SVGSMILElement::setTargetElement(target);
+    checkInvalidCSSAttributeType(target);
 }
 
 void SVGAnimationElement::setAttributeName(const QualifiedName& attributeName)
 {
     SVGSMILElement::setAttributeName(attributeName);
-
-    checkInvalidCSSAttributeType(targetElement(DoNotResolveNewTarget));
+    checkInvalidCSSAttributeType(targetElement());
 }
 
 void SVGAnimationElement::checkInvalidCSSAttributeType(SVGElement* target)
index 7439c81..a24ae36 100644 (file)
@@ -197,7 +197,8 @@ protected:
     AnimatedPropertyValueType m_fromPropertyValueType;
     AnimatedPropertyValueType m_toPropertyValueType;
 
-    virtual void targetElementWillChange(SVGElement* currentTarget, SVGElement* oldTarget) OVERRIDE;
+    virtual void setTargetElement(SVGElement*) OVERRIDE;
+    virtual void setAttributeName(const QualifiedName&) OVERRIDE;
     bool hasInvalidCSSAttributeType() const { return m_hasInvalidCSSAttributeType; }
 
     virtual void updateAnimationMode();
@@ -206,7 +207,6 @@ protected:
 
 private:
     virtual void animationAttributeChanged() OVERRIDE;
-    virtual void setAttributeName(const QualifiedName&) OVERRIDE;
     void setAttributeType(const AtomicString&);
 
     void checkInvalidCSSAttributeType(SVGElement*);
index b5d81b6..889229a 100644 (file)
@@ -50,7 +50,6 @@ SVGDocumentExtensions::SVGDocumentExtensions(Document* document)
 
 SVGDocumentExtensions::~SVGDocumentExtensions()
 {
-    deleteAllValues(m_animatedElements);
     deleteAllValues(m_pendingResources);
     deleteAllValues(m_pendingResourcesForRemoval);
 }
@@ -133,58 +132,6 @@ void SVGDocumentExtensions::dispatchSVGLoadEventToOutermostSVGElements()
     }
 }
 
-void SVGDocumentExtensions::addAnimationElementToTarget(SVGSMILElement* animationElement, SVGElement* targetElement)
-{
-    ASSERT(targetElement);
-    ASSERT(animationElement);
-
-    if (HashSet<SVGSMILElement*>* animationElementsForTarget = m_animatedElements.get(targetElement)) {
-        animationElementsForTarget->add(animationElement);
-        return;
-    }
-
-    HashSet<SVGSMILElement*>* animationElementsForTarget = new HashSet<SVGSMILElement*>;
-    animationElementsForTarget->add(animationElement);
-    m_animatedElements.set(targetElement, animationElementsForTarget);
-}
-
-void SVGDocumentExtensions::removeAnimationElementFromTarget(SVGSMILElement* animationElement, SVGElement* targetElement)
-{
-    ASSERT(targetElement);
-    ASSERT(animationElement);
-
-    HashMap<SVGElement*, HashSet<SVGSMILElement*>* >::iterator it = m_animatedElements.find(targetElement);
-    ASSERT(it != m_animatedElements.end());
-    
-    HashSet<SVGSMILElement*>* animationElementsForTarget = it->value;
-    ASSERT(!animationElementsForTarget->isEmpty());
-
-    animationElementsForTarget->remove(animationElement);
-    if (animationElementsForTarget->isEmpty()) {
-        m_animatedElements.remove(it);
-        delete animationElementsForTarget;
-    }
-}
-
-void SVGDocumentExtensions::removeAllAnimationElementsFromTarget(SVGElement* targetElement)
-{
-    ASSERT(targetElement);
-    HashMap<SVGElement*, HashSet<SVGSMILElement*>* >::iterator it = m_animatedElements.find(targetElement);
-    if (it == m_animatedElements.end())
-        return;
-
-    HashSet<SVGSMILElement*>* animationElementsForTarget = it->value;
-    Vector<SVGSMILElement*> toBeReset;
-
-    HashSet<SVGSMILElement*>::iterator end = animationElementsForTarget->end();
-    for (HashSet<SVGSMILElement*>::iterator it = animationElementsForTarget->begin(); it != end; ++it)
-        toBeReset.append(*it);
-
-    Vector<SVGSMILElement*>::iterator vectorEnd = toBeReset.end();
-    for (Vector<SVGSMILElement*>::iterator vectorIt = toBeReset.begin(); vectorIt != vectorEnd; ++vectorIt)
-        (*vectorIt)->resetTargetElement();
-}
-
 static void reportMessage(Document* document, MessageLevel level, const String& message)
 {
     if (document->frame())
index b4455c5..3207d36 100644 (file)
@@ -59,10 +59,6 @@ public:
     void pauseAnimations();
     void unpauseAnimations();
     void dispatchSVGLoadEventToOutermostSVGElements();
-    
-    void addAnimationElementToTarget(SVGSMILElement*, SVGElement*);
-    void removeAnimationElementFromTarget(SVGSMILElement*, SVGElement*);
-    void removeAllAnimationElementsFromTarget(SVGElement*);
 
     void reportWarning(const String&);
     void reportError(const String&);
@@ -83,7 +79,6 @@ public:
 private:
     Document* m_document; // weak reference
     HashSet<SVGSVGElement*> m_timeContainers; // For SVG 1.2 support this will need to be made more general.
-    HashMap<SVGElement*, HashSet<SVGSMILElement*>* > m_animatedElements;
 #if ENABLE(SVG_FONTS)
     HashSet<SVGFontFaceElement*> m_svgFontFaceElements;
 #endif
index ce806c5..6cd4e83 100644 (file)
@@ -83,7 +83,6 @@ SVGElement::~SVGElement()
         rareDataMap.remove(it);
     }
     ASSERT(document());
-    document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
     document()->accessSVGExtensions()->removeAllElementReferencesForTarget(this);
 }
 
@@ -183,7 +182,6 @@ void SVGElement::removedFrom(ContainerNode* rootParent)
     StyledElement::removedFrom(rootParent);
 
     if (wasInDocument) {
-        document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
         document()->accessSVGExtensions()->removeAllElementReferencesForTarget(this);
         document()->accessSVGExtensions()->removeElementFromPendingResources(this);
     }
@@ -537,10 +535,8 @@ void SVGElement::attributeChanged(const QualifiedName& name, const AtomicString&
 {
     StyledElement::attributeChanged(name, newValue);
 
-    if (isIdAttributeName(name)) {
-        document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
+    if (isIdAttributeName(name))
         document()->accessSVGExtensions()->removeAllElementReferencesForTarget(this);
-    }
 
     // Changes to the style attribute are processed lazily (see Element::getAttribute() and related methods),
     // so we don't want changes to the style attribute to result in extra work here.
index 8f4d07b..4df3af5 100644 (file)
@@ -140,14 +140,6 @@ void SVGElementInstance::invalidateAllInstancesOfElement(SVGElement* element)
         ASSERT((*it)->correspondingElement() == element);
         (*it)->shadowTreeElement()->setCorrespondingElement(0);
 
-        // The shadow tree, which is eventually animated, is mutated. In order to keep the animVal
-        // logic correct, we have to stop the animation, and restart it once the shadow tree has
-        // recloned. Otherwise we miss to call stopAnimValAnimation() on the old shadow tree element
-        // which leads to an assertion once garbage is collected, if the animVal bindings have been
-        // accessed from JS. It would also assert on the next updateAnimations() call as the new
-        // SVGAnimatedProperty object hasn't been initialized yet (using startAnimValAnimation).
-        element->document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(element);
-
         if (SVGUseElement* element = (*it)->correspondingUseElement()) {
             ASSERT(element->inDocument());
             element->invalidateShadowTree();
index e278fbd..359c40b 100644 (file)
@@ -64,7 +64,8 @@ void SVGMPathElement::buildPendingResource()
     String id;
     Element* target = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
     if (!target) {
-        if (hasPendingResources())
+        // Do not register as pending if we are already pending this resource.
+        if (document()->accessSVGExtensions()->isElementPendingResource(this, id))
             return;
 
         if (!id.isEmpty()) {
index ec4d8f7..3e6b1d1 100644 (file)
@@ -163,11 +163,14 @@ void SVGTextPathElement::buildPendingResource()
     String id;
     Element* target = SVGURIReference::targetElementFromIRIString(href(), document(), &id);
     if (!target) {
-        if (hasPendingResources() || id.isEmpty())
+        // Do not register as pending if we are already pending this resource.
+        if (document()->accessSVGExtensions()->isElementPendingResource(this, id))
             return;
 
-        document()->accessSVGExtensions()->addPendingResource(id, this);
-        ASSERT(hasPendingResources());
+        if (!id.isEmpty()) {
+            document()->accessSVGExtensions()->addPendingResource(id, this);
+            ASSERT(hasPendingResources());
+        }
     } else if (target->isSVGElement()) {
         // Register us with the target in the dependencies map. Any change of hrefElement
         // that leads to relayout/repainting now informs us, so we can react to it.
index 479b2c8..ee43ffa 100644 (file)
@@ -139,11 +139,50 @@ SVGSMILElement::SVGSMILElement(const QualifiedName& tagName, Document* doc)
 
 SVGSMILElement::~SVGSMILElement()
 {
+    clearResourceReferences();
     disconnectConditions();
     if (m_timeContainer && m_targetElement && hasValidAttributeName())
         m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
-    if (m_targetElement)
-        document()->accessSVGExtensions()->removeAnimationElementFromTarget(this, m_targetElement);
+}
+
+void SVGSMILElement::clearResourceReferences()
+{
+    ASSERT(document());
+    document()->accessSVGExtensions()->removeAllTargetReferencesForElement(this);
+}
+
+void SVGSMILElement::buildPendingResource()
+{
+    clearResourceReferences();
+    if (!inDocument())
+        return;
+
+    String id;
+    String href = getAttribute(XLinkNames::hrefAttr);
+    Element* target;
+    if (href.isEmpty())
+        target = parentNode() && parentNode()->isElementNode() ? static_cast<Element*>(parentNode()) : 0;
+    else
+        target = SVGURIReference::targetElementFromIRIString(href, document(), &id);
+    SVGElement* svgTarget = target && target->isSVGElement() ? static_cast<SVGElement*>(target) : 0;
+
+    if (svgTarget != targetElement())
+        setTargetElement(svgTarget);
+
+    if (!svgTarget) {
+        // Do not register as pending if we are already pending this resource.
+        if (document()->accessSVGExtensions()->isElementPendingResource(this, id))
+            return;
+
+        if (!id.isEmpty()) {
+            document()->accessSVGExtensions()->addPendingResource(id, this);
+            ASSERT(hasPendingResources());
+        }
+    } else {
+        // Register us with the target in the dependencies map. Any change of hrefElement
+        // that leads to relayout/repainting now informs us, so we can react to it.
+        document()->accessSVGExtensions()->addElementReferencingTarget(this, svgTarget);
+    }
 }
 
 static inline QualifiedName constructQualifiedName(const SVGElement* svgElement, const String& attributeName)
@@ -216,12 +255,11 @@ Node::InsertionNotificationRequest SVGSMILElement::insertedInto(ContainerNode* r
     if (m_isWaitingForFirstInterval)
         resolveFirstInterval();
 
-    // Force resolution of target element
-    if (!m_targetElement)
-        targetElement();
     if (m_timeContainer)
         m_timeContainer->notifyIntervalsChanged();
 
+    buildPendingResource();
+
     return InsertionDone;
 }
 
@@ -238,11 +276,10 @@ void SVGSMILElement::removedFrom(ContainerNode* rootParent)
         RefPtr<SVGSMILElement> keepAlive(this);
         disconnectConditions();
 
-        // Clear target now, because disconnectConditions calls targetElement() which will recreate the target if we removed it sooner. 
-        if (m_targetElement)
-            resetTargetElement(DoNotResolveNewTarget);
-
+        clearResourceReferences();
+        setTargetElement(0);
         setAttributeName(anyQName());
+        animationAttributeChanged();
     }
 
     SVGElement::removedFrom(rootParent);
@@ -472,16 +509,11 @@ void SVGSMILElement::svgAttributeChanged(const QualifiedName& attrName)
             beginListChanged(elapsed());
         else if (attrName == SVGNames::endAttr)
             endListChanged(elapsed());
-        else if (attrName == SVGNames::attributeNameAttr) {
+        else if (attrName == SVGNames::attributeNameAttr)
             setAttributeName(constructQualifiedName(this, fastGetAttribute(SVGNames::attributeNameAttr)));
-            if (m_targetElement) {
-                resetTargetElement();
-                return;
-            }
-        } else if (attrName.matches(XLinkNames::hrefAttr)) {
-            // targetElement is resolved lazily but targetElement() will handle calling targetElementWillChange().
-            if (SVGElement* targetElement = this->targetElement())
-                document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(targetElement);
+        else if (attrName.matches(XLinkNames::hrefAttr)) {
+            SVGElementInstance::InvalidationGuard invalidationGuard(this);
+            buildPendingResource();
         }
     }
 
@@ -562,57 +594,30 @@ void SVGSMILElement::setAttributeName(const QualifiedName& attributeName)
             m_timeContainer->schedule(this, m_targetElement, m_attributeName);
     } else
         m_attributeName = attributeName;
-}
 
-SVGElement* SVGSMILElement::targetElement(ResolveTarget resolveTarget)
-{
+    // Only clear the animated type, if we had a target before.
     if (m_targetElement)
-        return m_targetElement;
-
-    if (!inDocument() || resolveTarget == DoNotResolveNewTarget)
-        return 0;
-
-    String href = getAttribute(XLinkNames::hrefAttr);
-    ContainerNode* target = href.isEmpty() ? parentNode() : SVGURIReference::targetElementFromIRIString(href, document());
-    if (!target || !target->isSVGElement())
-        return 0;
-
-    SVGElement* targetElement = static_cast<SVGElement*>(target);
-    targetElementWillChange(m_targetElement, targetElement);
-    m_targetElement = targetElement;
-    document()->accessSVGExtensions()->addAnimationElementToTarget(this, m_targetElement);
-    return m_targetElement;
+        clearAnimatedType(m_targetElement);
 }
 
-void SVGSMILElement::targetElementWillChange(SVGElement* currentTarget, SVGElement* newTarget)
+void SVGSMILElement::setTargetElement(SVGElement* target)
 {
     if (m_timeContainer && hasValidAttributeName()) {
-        if (currentTarget)
-            m_timeContainer->unschedule(this, currentTarget, m_attributeName);
-        if (newTarget)
-            m_timeContainer->schedule(this, newTarget, m_attributeName);
+        if (m_targetElement)
+            m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
+        if (target)
+            m_timeContainer->schedule(this, target, m_attributeName);
     }
 
     // Only clear the animated type, if we had a target before.
-    if (currentTarget)
-        clearAnimatedType(currentTarget);
+    if (m_targetElement)
+        clearAnimatedType(m_targetElement);
 
     // If the animation state is not Inactive, always reset to a clear state before leaving the old target element.
     if (m_activeState != Inactive)
         endedActiveInterval();
-}
 
-void SVGSMILElement::resetTargetElement(ResolveTarget resolveTarget)
-{
-    document()->accessSVGExtensions()->removeAnimationElementFromTarget(this, m_targetElement);
-    targetElementWillChange(m_targetElement, 0);
-    m_targetElement = 0;
-
-    // Immediately resolve the new targetElement (and call targetElementWillChange if needed) instead of doing it lazily.
-    if (resolveTarget == ResolveNewTarget)
-        targetElement();
-
-    animationAttributeChanged();
+    m_targetElement = target;
 }
 
 SMILTime SVGSMILElement::elapsed() const
index ede19f9..34ddba5 100644 (file)
@@ -33,8 +33,6 @@
 
 namespace WebCore {
 
-enum ResolveTarget { DoNotResolveNewTarget, ResolveNewTarget };
-
 class ConditionEventListener;
 class SMILTimeContainer;
 
@@ -58,8 +56,7 @@ public:
 
     SMILTimeContainer* timeContainer() const { return m_timeContainer.get(); }
 
-    SVGElement* targetElement(ResolveTarget = ResolveNewTarget);
-    void resetTargetElement(ResolveTarget = ResolveNewTarget);
+    SVGElement* targetElement() const { return m_targetElement; }
     const QualifiedName& attributeName() const { return m_attributeName; }
 
     void beginByLinkActivation();
@@ -120,10 +117,13 @@ protected:
     void setInactive() { m_activeState = Inactive; }
 
     // Sub-classes may need to take action when the target is changed.
-    virtual void targetElementWillChange(SVGElement* currentTarget, SVGElement* newTarget);
+    virtual void setTargetElement(SVGElement*);
     virtual void setAttributeName(const QualifiedName&);
 
 private:
+    void buildPendingResource();
+    void clearResourceReferences();
+
     virtual void startedActiveInterval() = 0;
     void endedActiveInterval();
     virtual void updateAnimation(float percent, unsigned repeat, SVGSMILElement* resultElement) = 0;