Attachment 176414 Details for Bug 102655 – Preview patch 2 - almost ready for review

[patch] Preview patch 2 - almost ready for review

102655.2.patch (text/plain), 30.47 KB, created by Philip Rogers on 2012-11-27 23:54:42 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Philip Rogers

Created: 2012-11-27 23:54:42 PST

Size: 30.47 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 135946)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,77 @@
>+2012-11-27  Philip Rogers  <pdr@google.com>
>+
>+        Remove redundant SVG animation target tracking framework.
>+        https://bugs.webkit.org/show_bug.cgi?id=102655
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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="#someId", we need to track when #someId changes to #otherId,
>+        when #someId is removed, etc. This bookkeeping of element -> target is done in
>+        SVGDocumentExtensions.
>+
>+
>+        TODO: more details here.
>+
>+
>+        A test has been added to test that the order of animation elements does not matter.
>+        The animation target tracking framework did not support that, whereas the proper
>+        SVG target tracking framework does.
>+
>+        * svg/SVGAnimateElement.cpp:
>+        (WebCore::SVGAnimateElement::clearAnimatedType):
>+        (WebCore::SVGAnimateElement::setTargetElement):
>+        * svg/SVGAnimateElement.h:
>+        (SVGAnimateElement):
>+        * svg/SVGAnimateMotionElement.cpp:
>+        (WebCore::SVGAnimateMotionElement::clearAnimatedType):
>+        * svg/SVGAnimateMotionElement.h:
>+        (SVGAnimateMotionElement):
>+        * svg/SVGAnimationElement.cpp:
>+        (WebCore::SVGAnimationElement::~SVGAnimationElement):
>+        (WebCore):
>+        (WebCore::SVGAnimationElement::clearResourceReferences):
>+        (WebCore::SVGAnimationElement::buildPendingResource):
>+        (WebCore::SVGAnimationElement::isSupportedAttribute):
>+        (WebCore::SVGAnimationElement::parseAttribute):
>+        (WebCore::SVGAnimationElement::svgAttributeChanged):
>+        (WebCore::SVGAnimationElement::insertedInto):
>+        (WebCore::SVGAnimationElement::removedFrom):
>+        (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):
>+        * svg/animation/SVGSMILElement.cpp:
>+        (WebCore::SVGSMILElement::SVGSMILElement):
>+        (WebCore::SVGSMILElement::~SVGSMILElement):
>+        (WebCore::SVGSMILElement::reset):
>+        (WebCore::SVGSMILElement::insertedInto):
>+        (WebCore::SVGSMILElement::removedFrom):
>+        (WebCore::SVGSMILElement::isSupportedAttribute):
>+        (WebCore::SVGSMILElement::svgAttributeChanged):
>+        (WebCore::SVGSMILElement::setTargetElement):
>+        (WebCore::SVGSMILElement::progress):
>+        * svg/animation/SVGSMILElement.h:
>+        (WebCore):
>+        (WebCore::SVGSMILElement::targetElement):
>+        (SVGSMILElement):
>+
> 2012-11-27  Dean Jackson  <dino@apple.com>
> 
>         Attempted build fix for my last commit. The private member variable
>Index: Source/WebCore/svg/SVGAnimateElement.cpp
>===================================================================
>--- Source/WebCore/svg/SVGAnimateElement.cpp	(revision 135701)
>+++ Source/WebCore/svg/SVGAnimateElement.cpp	(working copy)
>@@ -396,16 +396,29 @@ float SVGAnimateElement::calculateDistan
>     return ensureAnimator()->calculateDistance(fromString, toString);
> }
> 
>-void SVGAnimateElement::targetElementWillChange(SVGElement* currentTarget, SVGElement* newTarget)
>+void SVGAnimateElement::setTargetElement(SVGElement* target)
> {
>-    SVGAnimationElement::targetElementWillChange(currentTarget, newTarget);
>+    SVGAnimationElement::setTargetElement(target);
> 
>     ASSERT(!m_animatedType);
>     m_fromType.clear();
>     m_toType.clear();
>     m_toAtEndOfDurationType.clear();
>     m_animator.clear();
>-    m_animatedPropertyType = newTarget ? determineAnimatedPropertyType(newTarget) : AnimatedString;
>+    m_animatedPropertyType = target ? determineAnimatedPropertyType(target) : AnimatedString;
>+}
>+
>+void SVGAnimateElement::setAttributeName(const QualifiedName& attributeName)
>+{
>+    SVGAnimationElement::setAttributeName(attributeName);
>+
>+    // FIXME: unify with setTargetElement, possibly in clearAttributeType().
>+    ASSERT(!m_animatedType);
>+    m_fromType.clear();
>+    m_toType.clear();
>+    m_toAtEndOfDurationType.clear();
>+    m_animator.clear();
>+    m_animatedPropertyType = targetElement() ? determineAnimatedPropertyType(targetElement()) : AnimatedString;
> }
> 
> SVGAnimatedTypeAnimator* SVGAnimateElement::ensureAnimator()
>Index: Source/WebCore/svg/SVGAnimateElement.h
>===================================================================
>--- Source/WebCore/svg/SVGAnimateElement.h	(revision 135701)
>+++ Source/WebCore/svg/SVGAnimateElement.h	(working copy)
>@@ -44,7 +44,7 @@ protected:
>     SVGAnimateElement(const QualifiedName&, Document*);
> 
>     virtual void resetAnimatedType();
>-    virtual void clearAnimatedType(SVGElement* targetElement);
>+    virtual void clearAnimatedType(SVGElement*);
>     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,7 +53,8 @@ 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;
> 
>Index: Source/WebCore/svg/SVGAnimateMotionElement.h
>===================================================================
>--- Source/WebCore/svg/SVGAnimateMotionElement.h	(revision 135701)
>+++ Source/WebCore/svg/SVGAnimateMotionElement.h	(working copy)
>@@ -42,7 +42,7 @@ private:
>     virtual void parseAttribute(const QualifiedName&, const AtomicString&) OVERRIDE;
> 
>     virtual void resetAnimatedType();
>-    virtual void clearAnimatedType(SVGElement* targetElement);
>+    virtual void clearAnimatedType(SVGElement*);
>     virtual bool calculateToAtEndOfDurationValue(const String& toAtEndOfDurationString);
>     virtual bool calculateFromAndToValues(const String& fromString, const String& toString);
>     virtual bool calculateFromAndByValues(const String& fromString, const String& byString);
>Index: Source/WebCore/svg/SVGAnimationElement.cpp
>===================================================================
>--- Source/WebCore/svg/SVGAnimationElement.cpp	(revision 135701)
>+++ Source/WebCore/svg/SVGAnimationElement.cpp	(working copy)
>@@ -35,10 +35,14 @@
> #include "FloatConversion.h"
> #include "RenderObject.h"
> #include "SVGAnimateElement.h"
>+#include "SVGDocumentExtensions.h"
> #include "SVGElementInstance.h"
>+#include "SVGExternalResourcesRequired.h"
> #include "SVGNames.h"
> #include "SVGParserUtilities.h"
> #include "SVGStyledElement.h"
>+#include "SVGURIReference.h"
>+#include "XLinkNames.h"
> #include <wtf/MathExtras.h>
> 
> namespace WebCore {
>@@ -64,6 +68,50 @@ SVGAnimationElement::SVGAnimationElement
>     registerAnimatedPropertiesForSVGAnimationElement();
> }
> 
>+SVGAnimationElement::~SVGAnimationElement()
>+{
>+    clearResourceReferences();
>+}
>+
>+void SVGAnimationElement::clearResourceReferences()
>+{
>+    ASSERT(document());
>+    document()->accessSVGExtensions()->removeAllTargetReferencesForElement(this);
>+}
>+
>+void SVGAnimationElement::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) {
>+        if (hasPendingResources())
>+            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 void parseKeyTimes(const String& parse, Vector<float>& result, bool verifyOrder)
> {
>     result.clear();
>@@ -155,6 +203,7 @@ bool SVGAnimationElement::isSupportedAtt
>         supportedAttributes.add(SVGNames::fromAttr);
>         supportedAttributes.add(SVGNames::toAttr);
>         supportedAttributes.add(SVGNames::byAttr);
>+        supportedAttributes.add(XLinkNames::hrefAttr);
>     }
>     return supportedAttributes.contains<QualifiedName, SVGAttributeHashTranslator>(attrName);
> }
>@@ -216,6 +265,8 @@ void SVGAnimationElement::parseAttribute
>         return;
>     if (SVGExternalResourcesRequired::parseAttribute(name, value))
>         return;
>+    if (name.matches(XLinkNames::hrefAttr))
>+        return;
> 
>     ASSERT_NOT_REACHED();
> }
>@@ -227,6 +278,13 @@ void SVGAnimationElement::svgAttributeCh
>         return;
>     }
> 
>+    SVGElementInstance::InvalidationGuard invalidationGuard(this);
>+
>+    if (attrName.matches(XLinkNames::hrefAttr))
>+        buildPendingResource();
>+    else if (SVGExternalResourcesRequired::isKnownAttribute(attrName))
>+        return;
>+
>     animationAttributeChanged();
> }
> 
>@@ -237,6 +295,21 @@ void SVGAnimationElement::animationAttri
>     setInactive();
> }
> 
>+Node::InsertionNotificationRequest SVGAnimationElement::insertedInto(ContainerNode* rootParent)
>+{
>+    SVGSMILElement::insertedInto(rootParent);
>+    if (rootParent->inDocument())
>+        buildPendingResource();
>+    return InsertionDone;
>+}
>+
>+void SVGAnimationElement::removedFrom(ContainerNode* rootParent)
>+{
>+    SVGSMILElement::removedFrom(rootParent);
>+    if (rootParent->inDocument())
>+        clearResourceReferences();
>+}
>+
> float SVGAnimationElement::getStartTime() const
> {
>     return narrowPrecisionToFloat(intervalBegin().value());
>@@ -319,7 +392,7 @@ void SVGAnimationElement::setAttributeTy
>         m_attributeType = AttributeTypeXML;
>     else
>         m_attributeType = AttributeTypeAuto;
>-    checkInvalidCSSAttributeType(targetElement(DoNotResolveNewTarget));
>+    checkInvalidCSSAttributeType(targetElement());
> }
> 
> String SVGAnimationElement::toValue() const
>@@ -677,18 +750,16 @@ void SVGAnimationElement::determinePrope
>         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: Source/WebCore/svg/SVGAnimationElement.h
>===================================================================
>--- Source/WebCore/svg/SVGAnimationElement.h	(revision 135701)
>+++ Source/WebCore/svg/SVGAnimationElement.h	(working copy)
>@@ -29,10 +29,12 @@
> #include "ElementTimeControl.h"
> #include "SMILTime.h"
> #include "SVGAnimatedBoolean.h"
>+#include "SVGAnimatedString.h"
> #include "SVGExternalResourcesRequired.h"
> #include "SVGSMILElement.h"
> #include "SVGStringList.h"
> #include "SVGTests.h"
>+#include "SVGURIReference.h"
> #include "UnitBezier.h"
> 
> namespace WebCore {
>@@ -71,6 +73,8 @@ class SVGAnimationElement : public SVGSM
>                             public SVGExternalResourcesRequired,
>                             public ElementTimeControl {
> public:
>+    virtual ~SVGAnimationElement();
>+
>     // SVGAnimationElement
>     float getStartTime() const;
>     float getCurrentTime() const;
>@@ -197,7 +201,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();
>@@ -205,8 +210,12 @@ protected:
>     void setCalcMode(CalcMode calcMode) { m_calcMode = calcMode; }
> 
> private:
>+    void buildPendingResource();
>+    void clearResourceReferences();
>+    virtual InsertionNotificationRequest insertedInto(ContainerNode*) OVERRIDE;
>+    virtual void removedFrom(ContainerNode*) OVERRIDE;
>+
>     virtual void animationAttributeChanged() OVERRIDE;
>-    virtual void setAttributeName(const QualifiedName&) OVERRIDE;
>     void setAttributeType(const AtomicString&);
> 
>     void checkInvalidCSSAttributeType(SVGElement*);
>Index: Source/WebCore/svg/SVGDocumentExtensions.cpp
>===================================================================
>--- Source/WebCore/svg/SVGDocumentExtensions.cpp	(revision 135701)
>+++ Source/WebCore/svg/SVGDocumentExtensions.cpp	(working copy)
>@@ -50,7 +50,6 @@ SVGDocumentExtensions::SVGDocumentExtens
> 
> SVGDocumentExtensions::~SVGDocumentExtensions()
> {
>-    deleteAllValues(m_animatedElements);
>     deleteAllValues(m_pendingResources);
>     deleteAllValues(m_pendingResourcesForRemoval);
> }
>@@ -133,58 +132,6 @@ void SVGDocumentExtensions::dispatchSVGL
>     }
> }
> 
>-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();
>-}
>-
> // FIXME: Callers should probably use ScriptController::eventHandlerLineNumber()
> static int parserLineNumber(Document* document)
> {
>Index: Source/WebCore/svg/SVGDocumentExtensions.h
>===================================================================
>--- Source/WebCore/svg/SVGDocumentExtensions.h	(revision 135701)
>+++ Source/WebCore/svg/SVGDocumentExtensions.h	(working copy)
>@@ -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: Source/WebCore/svg/SVGElement.cpp
>===================================================================
>--- Source/WebCore/svg/SVGElement.cpp	(revision 135701)
>+++ Source/WebCore/svg/SVGElement.cpp	(working copy)
>@@ -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(ContainerNo
>     StyledElement::removedFrom(rootParent);
> 
>     if (wasInDocument) {
>-        document()->accessSVGExtensions()->removeAllAnimationElementsFromTarget(this);
>         document()->accessSVGExtensions()->removeAllElementReferencesForTarget(this);
>         document()->accessSVGExtensions()->removeElementFromPendingResources(this);
>     }
>@@ -545,10 +543,8 @@ void SVGElement::attributeChanged(const 
>     if (isSynchronizingSVGAttributes())
>         return;
> 
>-    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: Source/WebCore/svg/SVGElementInstance.cpp
>===================================================================
>--- Source/WebCore/svg/SVGElementInstance.cpp	(revision 135701)
>+++ Source/WebCore/svg/SVGElementInstance.cpp	(working copy)
>@@ -140,14 +140,6 @@ void SVGElementInstance::invalidateAllIn
>         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: Source/WebCore/svg/animation/SVGSMILElement.cpp
>===================================================================
>--- Source/WebCore/svg/animation/SVGSMILElement.cpp	(revision 135701)
>+++ Source/WebCore/svg/animation/SVGSMILElement.cpp	(working copy)
>@@ -37,12 +37,9 @@
> #include "FrameView.h"
> #include "HTMLNames.h"
> #include "SMILTimeContainer.h"
>-#include "SVGDocumentExtensions.h"
> #include "SVGNames.h"
> #include "SVGParserUtilities.h"
> #include "SVGSVGElement.h"
>-#include "SVGURIReference.h"
>-#include "XLinkNames.h"
> #include <wtf/MathExtras.h>
> #include <wtf/StdLibExtras.h>
> #include <wtf/Vector.h>
>@@ -142,8 +139,6 @@ SVGSMILElement::~SVGSMILElement()
>     disconnectConditions();
>     if (m_timeContainer && m_targetElement && hasValidAttributeName())
>         m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
>-    if (m_targetElement)
>-        document()->accessSVGExtensions()->removeAnimationElementFromTarget(this, m_targetElement);
> }
> 
> static inline QualifiedName constructQualifiedName(const SVGElement* svgElement, const String& attributeName)
>@@ -216,9 +211,6 @@ Node::InsertionNotificationRequest SVGSM
>     if (m_isWaitingForFirstInterval)
>         resolveFirstInterval();
> 
>-    // Force resolution of target element
>-    if (!m_targetElement)
>-        targetElement();
>     if (m_timeContainer)
>         m_timeContainer->notifyIntervalsChanged();
> 
>@@ -238,11 +230,9 @@ void SVGSMILElement::removedFrom(Contain
>         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);
>-
>+        setTargetElement(0);
>         setAttributeName(anyQName());
>+        animationAttributeChanged();
>     }
> 
>     SVGElement::removedFrom(rootParent);
>@@ -421,7 +411,6 @@ bool SVGSMILElement::isSupportedAttribut
>         supportedAttributes.add(SVGNames::minAttr);
>         supportedAttributes.add(SVGNames::maxAttr);
>         supportedAttributes.add(SVGNames::attributeNameAttr);
>-        supportedAttributes.add(XLinkNames::hrefAttr);
>     }
>     return supportedAttributes.contains<QualifiedName, SVGAttributeHashTranslator>(attrName);
> }
>@@ -474,14 +463,6 @@ void SVGSMILElement::svgAttributeChanged
>             endListChanged(elapsed());
>         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);
>         }
>     }
> 
>@@ -562,57 +543,30 @@ void SVGSMILElement::setAttributeName(co
>             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: Source/WebCore/svg/animation/SVGSMILElement.h
>===================================================================
>--- Source/WebCore/svg/animation/SVGSMILElement.h	(revision 135701)
>+++ Source/WebCore/svg/animation/SVGSMILElement.h	(working copy)
>@@ -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();
>@@ -110,7 +107,7 @@ public:
> 
>     virtual bool isAdditive() const = 0;
>     virtual void resetAnimatedType() = 0;
>-    virtual void clearAnimatedType(SVGElement* targetElement) = 0;
>+    virtual void clearAnimatedType(SVGElement*) = 0;
>     virtual void applyResultsToTarget() = 0;
> 
> protected:
>@@ -120,7 +117,7 @@ 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:
>@@ -195,7 +192,7 @@ private:
>     float calculateAnimationPercentAndRepeat(SMILTime elapsed, unsigned& repeat) const;
>     SMILTime calculateNextProgressTime(SMILTime elapsed) const;
> 
>-    mutable SVGElement* m_targetElement;
>+    SVGElement* m_targetElement;
> 
>     Vector<Condition> m_conditions;
>     bool m_conditionsConnected;
>Index: LayoutTests/ChangeLog
>===================================================================
>--- LayoutTests/ChangeLog	(revision 135946)
>+++ LayoutTests/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2012-11-27  Philip Rogers  <pdr@google.com>
>+
>+        Remove redundant SVG animation target tracking framework.
>+        https://bugs.webkit.org/show_bug.cgi?id=102655
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
>+
>+        * svg/animations/svg-animation-order-expected.html: Added.
>+        * svg/animations/svg-animation-order.html: Added.
>+
> 2012-11-27  Tony Chang  <tony@chromium.org>
> 
>         Unreviewed, land updated Qt results after track limiter removal.
>Index: LayoutTests/svg/animations/svg-animation-order-expected.html
>===================================================================
>--- LayoutTests/svg/animations/svg-animation-order-expected.html	(revision 0)
>+++ LayoutTests/svg/animations/svg-animation-order-expected.html	(revision 0)
>@@ -0,0 +1,9 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<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>
>Index: LayoutTests/svg/animations/svg-animation-order.html
>===================================================================
>--- LayoutTests/svg/animations/svg-animation-order.html	(revision 0)
>+++ LayoutTests/svg/animations/svg-animation-order.html	(revision 0)
>@@ -0,0 +1,30 @@
>+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
>+<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">
>+  
>+  <rect x="0" y="0" width="100" height="100" fill="red"/>
>+
>+  
>+  <rect id="rect1"  x="-100" y="0" width="100" height="50" fill="green" />
>+  <set attributeName="x" xlink:href="#rect1" to="0"/>
>+
>+  
>+  <set attributeName="x" xlink:href="#rect2" to="0"/>
>+  <rect id="rect2"  x="-100" y="50" width="100" height="50" fill="green" />
>+</svg>
>+</body>
>+</html>

Actions: View | Formatted Diff | Diff

Attachments on bug 102655: 176362 | 176414 | 176582 | 177095 | 177096 | 178064