[SVG] Refactoring the logic to handle eventBase dependency

Source/core/svg/animation/SVGSMILElement.cpp

Index: Source/core/svg/animation/SVGSMILElement.cpp
diff --git a/Source/core/svg/animation/SVGSMILElement.cpp b/Source/core/svg/animation/SVGSMILElement.cpp
index 257005128f9e0bd9fa7eaf2231628122794f57d7..1ec5cc6639079c91975a78fc539959720c9e55d0 100644
--- a/Source/core/svg/animation/SVGSMILElement.cpp
+++ b/Source/core/svg/animation/SVGSMILElement.cpp
@@ -161,7 +161,7 @@ SVGSMILElement::SVGSMILElement(const QualifiedName& tagName, Document& doc)
     : SVGElement(tagName, doc)
     , m_attributeName(anyQName())
     , m_targetElement(0)
-    , m_conditionsConnected(false)
+    , m_syncBaseConditionsConnected(false)
     , m_hasEndEventConditions(false)
     , m_isWaitingForFirstInterval(true)
     , m_intervalBegin(SMILTime::unresolved())
@@ -188,7 +188,8 @@ SVGSMILElement::~SVGSMILElement()
     smilBeginEventSender().cancelEvent(this);
     smilRepeatEventSender().cancelEvent(this);
     smilRepeatNEventSender().cancelEvent(this);
-    disconnectConditions();
+    disconnectSyncBaseConditions();
+    disconnectEventBaseConditions();
     if (m_timeContainer && m_targetElement && hasValidAttributeName())
         m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
 }
@@ -237,6 +238,7 @@ void SVGSMILElement::buildPendingResource()
         // that leads to relayout/repainting now informs us, so we can react to it.
         document().accessSVGExtensions()->addElementReferencingTarget(this, svgTarget);
     }
+    connectEventBaseConditions();
 }
 
 static inline QualifiedName constructQualifiedName(const SVGElement* svgElement, const AtomicString& attributeName)
@@ -319,7 +321,8 @@ void SVGSMILElement::removedFrom(ContainerNode* rootParent)
 {
     if (rootParent->inDocument()) {
         clearResourceReferences();
-        disconnectConditions();
+        disconnectSyncBaseConditions();
+        disconnectEventBaseConditions();
         setTargetElement(0);
         setAttributeName(anyQName());
         animationAttributeChanged();
@@ -501,22 +504,22 @@ void SVGSMILElement::parseAttribute(const QualifiedName& name, const AtomicStrin
 {
     if (name == SVGNames::beginAttr) {
         if (!m_conditions.isEmpty()) {
-            disconnectConditions();
+            disconnectSyncBaseConditions();

[pdr., 2014/01/07 00:25:28]

Why disconnect sync base but not disconnect event base conditions? (here, and
elsewhere)

[pavane, 2014/01/07 18:20:58]

event base functionality is based on adding event listeners and firing the
events at appropriate times. So, only dependency is that eventBase should be
present. For achieving this we use addElementReferencingTarget(). If the
eventBase is removed, it adds itself to pending resources automatically.

whereas sync bases are sensitive to attribute changes also. So, we may have to
disconnect and reconnect on several other conditions

             m_conditions.clear();
             parseBeginOrEnd(fastGetAttribute(SVGNames::endAttr), End);
         }
         parseBeginOrEnd(value.string(), Begin);
         if (inDocument())
-            connectConditions();
+            connectSyncBaseConditions();
     } else if (name == SVGNames::endAttr) {
         if (!m_conditions.isEmpty()) {
-            disconnectConditions();
+            disconnectSyncBaseConditions();
             m_conditions.clear();
             parseBeginOrEnd(fastGetAttribute(SVGNames::beginAttr), Begin);
         }
         parseBeginOrEnd(value.string(), End);
         if (inDocument())
-            connectConditions();
+            connectSyncBaseConditions();
     } else
         SVGElement::parseAttribute(name, value);
 }
@@ -560,38 +563,62 @@ inline Element* SVGSMILElement::eventBaseFor(const Condition& condition)
     return condition.m_baseID.isEmpty() ? targetElement() : treeScope().getElementById(AtomicString(condition.m_baseID));
 }
 
-void SVGSMILElement::connectConditions()
+void SVGSMILElement::connectSyncBaseConditions()
 {
-    if (m_conditionsConnected)
-        disconnectConditions();
-    m_conditionsConnected = true;
+    if (m_syncBaseConditionsConnected)
+        disconnectSyncBaseConditions();
+    m_syncBaseConditionsConnected = true;
     for (unsigned n = 0; n < m_conditions.size(); ++n) {
         Condition& condition = m_conditions[n];
-        if (condition.m_type == Condition::EventBase) {
-            ASSERT(!condition.m_syncbase);
-            Element* eventBase = eventBaseFor(condition);
-            if (!eventBase)
-                continue;
-            ASSERT(!condition.m_eventListener);
-            condition.m_eventListener = ConditionEventListener::create(this, &condition);
-            eventBase->addEventListener(AtomicString(condition.m_name), condition.m_eventListener, false);
-        } else if (condition.m_type == Condition::Syncbase) {
+        if (condition.m_type == Condition::Syncbase) {
             ASSERT(!condition.m_baseID.isEmpty());
             condition.m_syncbase = treeScope().getElementById(AtomicString(condition.m_baseID));
             if (!condition.m_syncbase || !isSVGSMILElement(*condition.m_syncbase)) {
                 condition.m_syncbase = 0;
                 continue;
             }
-            toSVGSMILElement(condition.m_syncbase.get())->addTimeDependent(this);
+            toSVGSMILElement(condition.m_syncbase.get())->addSyncBaseDependent(this);
         }
     }
 }
 
-void SVGSMILElement::disconnectConditions()
+void SVGSMILElement::disconnectSyncBaseConditions()
 {
-    if (!m_conditionsConnected)
+    if (!m_syncBaseConditionsConnected)
         return;
-    m_conditionsConnected = false;
+    m_syncBaseConditionsConnected = false;
+    for (unsigned n = 0; n < m_conditions.size(); ++n) {
+        Condition& condition = m_conditions[n];
+        if (condition.m_type == Condition::Syncbase) {
+            if (condition.m_syncbase)
+                toSVGSMILElement(condition.m_syncbase.get())->removeSyncBaseDependent(this);
+            condition.m_syncbase = 0;
+        }
+    }
+}
+
+void SVGSMILElement::connectEventBaseConditions()
+{
+    disconnectEventBaseConditions();
+    for (unsigned n = 0; n < m_conditions.size(); ++n) {
+        Condition& condition = m_conditions[n];
+        if (condition.m_type == Condition::EventBase) {
+            ASSERT(!condition.m_syncbase);
+            Element* eventBase = eventBaseFor(condition);
+            if (!eventBase) {

[pdr., 2014/01/07 00:25:28]

Can you add an assert here that we don't add a pending resource twice?

ASSERT(!document().accessSVGExtensions()->isElementPendingResource(this,
condition.m_baseID));

[pavane, 2014/01/07 18:20:58]

Done.

+                document().accessSVGExtensions()->addPendingResource(condition.m_baseID, this);

[pdr., 2014/01/07 00:25:28]

Can you assert that m_baseID is not empty?

[pavane, 2014/01/07 18:20:58]

Done.

+                continue;
+            }
+            ASSERT(!condition.m_eventListener);
+            condition.m_eventListener = ConditionEventListener::create(this, &condition);
+            eventBase->addEventListener(AtomicString(condition.m_name), condition.m_eventListener, false);
+            document().accessSVGExtensions()->addElementReferencingTarget(this, toSVGElement(eventBase));

[pdr., 2014/01/07 00:25:28]

Here you add an element referencing a target but nowhere is this removed! We
should probably remove this at some point, maybe in
disconnectEventBaseConditions?

[pavane, 2014/01/07 18:20:58]

I think it is handled already. We are calling disconnectEventBaseConditions only
on element removal. So, removing the references is already handled there by
clearResourceAndEventBaseReferences()

+        }
+    }
+}
+
+void SVGSMILElement::disconnectEventBaseConditions()
+{
     for (unsigned n = 0; n < m_conditions.size(); ++n) {
         Condition& condition = m_conditions[n];
         if (condition.m_type == Condition::EventBase) {
@@ -608,11 +635,7 @@ void SVGSMILElement::disconnectConditions()
                 eventBase->removeEventListener(AtomicString(condition.m_name), condition.m_eventListener.get(), false);
             condition.m_eventListener->disconnectAnimation();
             condition.m_eventListener = 0;
-        } else if (condition.m_type == Condition::Syncbase) {
-            if (condition.m_syncbase)
-                toSVGSMILElement(condition.m_syncbase.get())->removeTimeDependent(this);
         }
-        condition.m_syncbase = 0;
     }
 }
 
@@ -644,7 +667,7 @@ void SVGSMILElement::setTargetElement(SVGElement* target)
     if (m_targetElement) {
         // Clear values that may depend on the previous target.
         clearAnimatedType(m_targetElement);
-        disconnectConditions();
+        disconnectSyncBaseConditions();
     }
 
     // If the animation state is not Inactive, always reset to a clear state before leaving the old target element.
@@ -1095,8 +1118,8 @@ bool SVGSMILElement::progress(SMILTime elapsed, SVGSMILElement* resultElement, b
     ASSERT(m_timeContainer);
     ASSERT(m_isWaitingForFirstInterval || m_intervalBegin.isFinite());
 
-    if (!m_conditionsConnected)
-        connectConditions();
+    if (!m_syncBaseConditionsConnected)
+        connectSyncBaseConditions();
 
     if (!m_intervalBegin.isFinite()) {
         ASSERT(m_activeState == Inactive);
@@ -1192,8 +1215,8 @@ void SVGSMILElement::notifyDependentsIntervalChanged()
     if (!loopBreaker.add(this).isNewEntry)
         return;
 
-    TimeDependentSet::iterator end = m_timeDependents.end();
-    for (TimeDependentSet::iterator it = m_timeDependents.begin(); it != end; ++it) {
+    TimeDependentSet::iterator end = m_syncBaseDependents.end();
+    for (TimeDependentSet::iterator it = m_syncBaseDependents.begin(); it != end; ++it) {
         SVGSMILElement* dependent = *it;
         dependent->createInstanceTimesFromSyncbase(this);
     }
@@ -1224,16 +1247,16 @@ void SVGSMILElement::createInstanceTimesFromSyncbase(SVGSMILElement* syncbase)
     }
 }
 
-void SVGSMILElement::addTimeDependent(SVGSMILElement* animation)
+void SVGSMILElement::addSyncBaseDependent(SVGSMILElement* animation)
 {
-    m_timeDependents.add(animation);
+    m_syncBaseDependents.add(animation);
     if (m_intervalBegin.isFinite())
         animation->createInstanceTimesFromSyncbase(this);
 }
 
-void SVGSMILElement::removeTimeDependent(SVGSMILElement* animation)
+void SVGSMILElement::removeSyncBaseDependent(SVGSMILElement* animation)
 {
-    m_timeDependents.remove(animation);
+    m_syncBaseDependents.remove(animation);
 }
 
 void SVGSMILElement::handleConditionEvent(Event* event, Condition* condition)