[SVG] Refactoring the logic to handle eventBase dependency

Source/core/svg/animation/SVGSMILElement.cpp

 /*
  * Copyright (C) 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 143 matching lines @@
     , m_name(name)
     , m_offset(offset)
     , m_repeat(repeat)
 {
 }
 
 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())
     , m_intervalEnd(SMILTime::unresolved())
     , m_previousIntervalBegin(SMILTime::unresolved())
     , m_activeState(Inactive)
     , m_lastPercent(0)
     , m_lastRepeat(0)
     , m_nextProgressTime(0)
     , m_documentOrderIndex(0)
     , m_cachedDur(invalidCachedTime)
     , m_cachedRepeatDur(invalidCachedTime)
     , m_cachedRepeatCount(invalidCachedTime)
     , m_cachedMin(invalidCachedTime)
     , m_cachedMax(invalidCachedTime)
 {
     resolveFirstInterval();
 }
 
 SVGSMILElement::~SVGSMILElement()
 {
     clearResourceReferences();
     smilEndEventSender().cancelEvent(this);
     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);
 }
 
 void SVGSMILElement::clearResourceReferences()
 {
     document().accessSVGExtensions()->removeAllTargetReferencesForElement(this);

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

This will also clear the event base conditions we added. It may be a good idea
to refactor this to be called clearResourceAndEventBaseReferences().

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

Done.

 }
 
 void SVGSMILElement::buildPendingResource()
 {
     clearResourceReferences();
 
     if (!inDocument()) {
         // Reset the target element if we are no longer in the document.
         setTargetElement(0);
         return;
@@ skipping 21 matching lines @@
 
         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);
     }
+    connectEventBaseConditions();
 }
 
 static inline QualifiedName constructQualifiedName(const SVGElement* svgElement, const AtomicString& attributeName)
 {
     ASSERT(svgElement);
     if (attributeName.isEmpty())
         return anyQName();
     if (!attributeName.contains(':'))
         return QualifiedName(nullAtom, attributeName, nullAtom);
 
@@ skipping 62 matching lines @@
 
     buildPendingResource();
 
     return InsertionDone;
 }
 
 void SVGSMILElement::removedFrom(ContainerNode* rootParent)
 {
     if (rootParent->inDocument()) {
         clearResourceReferences();
-        disconnectConditions();
+        disconnectSyncBaseConditions();
+        disconnectEventBaseConditions();
         setTargetElement(0);
         setAttributeName(anyQName());
         animationAttributeChanged();
         m_timeContainer = 0;
     }
 
     SVGElement::removedFrom(rootParent);
 }
 
 bool SVGSMILElement::hasValidAttributeName()
@@ skipping 161 matching lines @@
         supportedAttributes.add(SVGNames::attributeNameAttr);
         supportedAttributes.add(XLinkNames::hrefAttr);
     }
     return supportedAttributes.contains<SVGAttributeHashTranslator>(attrName);
 }
 
 void SVGSMILElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     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);
 }
 
 void SVGSMILElement::svgAttributeChanged(const QualifiedName& attrName)
 {
     if (!isSupportedAttribute(attrName)) {
         SVGElement::svgAttributeChanged(attrName);
         return;
     }
@@ skipping 23 matching lines @@
     }
 
     animationAttributeChanged();
 }
 
 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) {
             ASSERT(!condition.m_syncbase);
             if (!condition.m_eventListener)
                 continue;
             // Note: It's a memory optimization to try to remove our condition
             // event listener, but it's not guaranteed to work, since we have
             // no guarantee that eventBaseFor() will be able to find our condition's
             // original eventBase. So, we also have to disconnect ourselves from
             // our condition event listener, in case it later fires.
             Element* eventBase = eventBaseFor(condition);
             if (eventBase)
                 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;
     }
 }
 
 void SVGSMILElement::setAttributeName(const QualifiedName& attributeName)
 {
     if (m_timeContainer && m_targetElement && m_attributeName != attributeName) {
         if (hasValidAttributeName())
             m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
         m_attributeName = attributeName;
         if (hasValidAttributeName())
@@ skipping 11 matching lines @@
     if (m_timeContainer && hasValidAttributeName()) {
         if (m_targetElement)
             m_timeContainer->unschedule(this, m_targetElement, m_attributeName);
         if (target)
             m_timeContainer->schedule(this, target, m_attributeName);
     }
 
     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.
     if (m_activeState != Inactive)
         endedActiveInterval();
 
     m_targetElement = target;
 }
 
 SMILTime SVGSMILElement::elapsed() const
@@ skipping 430 matching lines @@
     // Animation does not contribute during the active time if it is past its repeating duration and has fill=remove.
     return (m_activeState == Active && (fill() == FillFreeze || elapsed <= m_intervalBegin + repeatingDuration())) || m_activeState == Frozen;
 }
 
 bool SVGSMILElement::progress(SMILTime elapsed, SVGSMILElement* resultElement, bool seekToTime)
 {
     ASSERT(resultElement);
     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);
         m_nextProgressTime = SMILTime::unresolved();
         return false;
     }
 
     if (elapsed < m_intervalBegin) {
         ASSERT(m_activeState != Active);
         if (m_activeState == Frozen) {
@@ skipping 75 matching lines @@
     return animationIsContributing;
 }
 
 void SVGSMILElement::notifyDependentsIntervalChanged()
 {
     ASSERT(m_intervalBegin.isFinite());
     DEFINE_STATIC_LOCAL(HashSet<SVGSMILElement*>, loopBreaker, ());
     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);
     }
 
     loopBreaker.remove(this);
 }
 
 void SVGSMILElement::createInstanceTimesFromSyncbase(SVGSMILElement* syncbase)
 {
     // FIXME: To be really correct, this should handle updating exising interval by changing
@@ skipping 10 matching lines @@
                 time = syncbase->m_intervalEnd + condition.m_offset;
             ASSERT(time.isFinite());
             if (condition.m_beginOrEnd == Begin)
                 addBeginTime(elapsed(), time);
             else
                 addEndTime(elapsed(), time);
         }
     }
 }
 
-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)
 {
     if (event->type() == "repeatn" && toRepeatEvent(event)->repeat() != condition->m_repeat)
         return;
 
     SMILTime elapsed = this->elapsed();
     if (condition->m_beginOrEnd == Begin)
         addBeginTime(elapsed, elapsed + condition->m_offset);
@@ skipping 27 matching lines @@
     if (eventType == "repeatn") {
         unsigned repeatEventCount = m_repeatEventCountList.first();
         m_repeatEventCountList.remove(0);
         dispatchEvent(RepeatEvent::create(eventType, repeatEventCount));
     } else {
         dispatchEvent(Event::create(eventType));
     }
 }
 
 }