Tidy up event-/syncbase registration in SVGSMILElement

third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp

Index: third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
diff --git a/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp b/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
index b7f10aac622757aedc738a1d2b7be8b5fc591a38..762155ecd6edcdddc34c3be6a1d67fdbe11ed7e3 100644
--- a/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
+++ b/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
@@ -124,15 +124,10 @@ void ConditionEventListener::handleEvent(ExecutionContext*, Event* event) {
   m_animation->handleConditionEvent(event, m_condition);
 }
 
-void SVGSMILElement::Condition::setEventListener(
-    ConditionEventListener* eventListener) {
-  m_eventListener = eventListener;
-}
-
 SVGSMILElement::Condition::Condition(Type type,
                                      BeginOrEnd beginOrEnd,
-                                     const String& baseID,
-                                     const String& name,
+                                     const AtomicString& baseID,
+                                     const AtomicString& name,
                                      SMILTime offset,
                                      int repeat)
     : m_type(type),
@@ -142,6 +137,76 @@ SVGSMILElement::Condition::Condition(Type type,
       m_offset(offset),
       m_repeat(repeat) {}
 
+SVGSMILElement::Condition::~Condition() = default;
+
+DEFINE_TRACE(SVGSMILElement::Condition) {
+  visitor->trace(m_syncBase);
+  visitor->trace(m_eventListener);
+}
+
+void SVGSMILElement::Condition::connectSyncBase(SVGSMILElement& timedElement) {
+  DCHECK(!m_baseID.isEmpty());
+  Element* element = timedElement.treeScope().getElementById(m_baseID);
+  if (!element || !isSVGSMILElement(*element)) {
+    m_syncBase = nullptr;
+    return;
+  }
+  m_syncBase = toSVGSMILElement(element);
+  m_syncBase->addSyncBaseDependent(timedElement);
+}
+
+void SVGSMILElement::Condition::disconnectSyncBase(
+    SVGSMILElement& timedElement) {
+  if (!m_syncBase)
+    return;
+  m_syncBase->removeSyncBaseDependent(timedElement);
+  m_syncBase = nullptr;
+}
+
+SVGElement* SVGSMILElement::Condition::lookupEventBase(
+    SVGSMILElement& timedElement) const {
+  Element* eventBase = m_baseID.isEmpty()
+                           ? timedElement.targetElement()
+                           : timedElement.treeScope().getElementById(m_baseID);
+  if (!eventBase || !eventBase->isSVGElement())
+    return nullptr;
+  return toSVGElement(eventBase);
+}
+
+void SVGSMILElement::Condition::connectEventBase(SVGSMILElement& timedElement) {
+  DCHECK(!m_syncBase);
+  SVGElement* eventBase = lookupEventBase(timedElement);
+  if (!eventBase) {
+    if (m_baseID.isEmpty())
+      return;
+    SVGTreeScopeResources& treeScopeResources =
+        timedElement.treeScope().ensureSVGTreeScopedResources();
+    if (!treeScopeResources.isElementPendingResource(timedElement, m_baseID))
+      treeScopeResources.addPendingResource(m_baseID, timedElement);
+    return;
+  }
+  DCHECK(!m_eventListener);
+  m_eventListener = ConditionEventListener::create(&timedElement, this);
+  eventBase->addEventListener(m_name, m_eventListener, false);
+  timedElement.addReferenceTo(eventBase);
+}
+
+void SVGSMILElement::Condition::disconnectEventBase(
+    SVGSMILElement& timedElement) {
+  DCHECK(!m_syncBase);
+  if (!m_eventListener)
+    return;
+  // 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
+  // we 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.
+  if (SVGElement* eventBase = lookupEventBase(timedElement))
+    eventBase->removeEventListener(m_name, m_eventListener, false);
+  m_eventListener->disconnectAnimation();
+  m_eventListener = nullptr;
+}
+
 SVGSMILElement::SVGSMILElement(const QualifiedName& tagName, Document& doc)
     : SVGElement(tagName, doc),
       SVGTests(this),
@@ -413,7 +478,8 @@ bool SVGSMILElement::parseCondition(const String& value,
   }
 
   m_conditions.push_back(
-      Condition::create(type, beginOrEnd, baseID, nameString, offset, repeat));
+      Condition::create(type, beginOrEnd, AtomicString(baseID),
+                        AtomicString(nameString), offset, repeat));
 
   if (type == Condition::EventBase && beginOrEnd == End)
     m_hasEndEventConditions = true;
@@ -516,34 +582,13 @@ void SVGSMILElement::svgAttributeChanged(const QualifiedName& attrName) {
   animationAttributeChanged();
 }
 
-inline SVGElement* SVGSMILElement::eventBaseFor(const Condition& condition) {
-  Element* eventBase =
-      condition.baseID().isEmpty()
-          ? targetElement()
-          : treeScope().getElementById(AtomicString(condition.baseID()));
-  if (eventBase && eventBase->isSVGElement())
-    return toSVGElement(eventBase);
-  return nullptr;
-}
-
 void SVGSMILElement::connectSyncBaseConditions() {
   if (m_syncBaseConditionsConnected)
     disconnectSyncBaseConditions();
   m_syncBaseConditionsConnected = true;
-  for (unsigned n = 0; n < m_conditions.size(); ++n) {
-    Condition* condition = m_conditions[n].get();
-    if (condition->getType() == Condition::Syncbase) {
-      ASSERT(!condition->baseID().isEmpty());
-      Element* element =
-          treeScope().getElementById(AtomicString(condition->baseID()));
-      if (!element || !isSVGSMILElement(*element)) {
-        condition->setSyncBase(0);
-        continue;
-      }
-      SVGSMILElement* svgSMILElement = toSVGSMILElement(element);
-      condition->setSyncBase(svgSMILElement);
-      svgSMILElement->addSyncBaseDependent(this);
-    }
+  for (Condition* condition : m_conditions) {
+    if (condition->getType() == Condition::Syncbase)
+      condition->connectSyncBase(*this);
   }
 }
 
@@ -551,63 +596,24 @@ void SVGSMILElement::disconnectSyncBaseConditions() {
   if (!m_syncBaseConditionsConnected)
     return;
   m_syncBaseConditionsConnected = false;
-  for (unsigned n = 0; n < m_conditions.size(); ++n) {
-    Condition* condition = m_conditions[n].get();
-    if (condition->getType() == Condition::Syncbase) {
-      if (condition->syncBase())
-        condition->syncBase()->removeSyncBaseDependent(this);
-      condition->setSyncBase(0);
-    }
+  for (Condition* condition : m_conditions) {
+    if (condition->getType() == Condition::Syncbase)
+      condition->disconnectSyncBase(*this);
   }
 }
 
 void SVGSMILElement::connectEventBaseConditions() {
   disconnectEventBaseConditions();
-  for (unsigned n = 0; n < m_conditions.size(); ++n) {
-    Condition* condition = m_conditions[n].get();
-    if (condition->getType() == Condition::EventBase) {
-      ASSERT(!condition->syncBase());
-      SVGElement* eventBase = eventBaseFor(*condition);
-      if (!eventBase) {
-        if (!condition->baseID().isEmpty() &&
-            !treeScope()
-                 .ensureSVGTreeScopedResources()
-                 .isElementPendingResource(*this,
-                                           AtomicString(condition->baseID()))) {
-          treeScope().ensureSVGTreeScopedResources().addPendingResource(
-              AtomicString(condition->baseID()), *this);
-        }
-        continue;
-      }
-      ASSERT(!condition->eventListener());
-      condition->setEventListener(
-          ConditionEventListener::create(this, condition));
-      eventBase->addEventListener(AtomicString(condition->name()),
-                                  condition->eventListener(), false);
-      addReferenceTo(eventBase);
-    }
+  for (Condition* condition : m_conditions) {
+    if (condition->getType() == Condition::EventBase)
+      condition->connectEventBase(*this);
   }
 }
 
 void SVGSMILElement::disconnectEventBaseConditions() {
-  for (unsigned n = 0; n < m_conditions.size(); ++n) {
-    Condition* condition = m_conditions[n].get();
-    if (condition->getType() == Condition::EventBase) {
-      ASSERT(!condition->syncBase());
-      if (!condition->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.
-      SVGElement* eventBase = eventBaseFor(*condition);
-      if (eventBase)
-        eventBase->removeEventListener(AtomicString(condition->name()),
-                                       condition->eventListener(), false);
-      condition->eventListener()->disconnectAnimation();
-      condition->setEventListener(nullptr);
-    }
+  for (Condition* condition : m_conditions) {
+    if (condition->getType() == Condition::EventBase)
+      condition->disconnectEventBase(*this);
   }
 }
 
@@ -1183,26 +1189,25 @@ void SVGSMILElement::notifyDependentsIntervalChanged() {
     return;
 
   for (SVGSMILElement* element : m_syncBaseDependents)
-    element->createInstanceTimesFromSyncbase(this);
+    element->createInstanceTimesFromSyncbase(*this);
 
   loopBreaker.erase(this);
 }
 
-void SVGSMILElement::createInstanceTimesFromSyncbase(SVGSMILElement* syncBase) {
+void SVGSMILElement::createInstanceTimesFromSyncbase(SVGSMILElement& syncBase) {
   // FIXME: To be really correct, this should handle updating exising interval
   // by changing the associated times instead of creating new ones.
-  for (unsigned n = 0; n < m_conditions.size(); ++n) {
-    Condition* condition = m_conditions[n].get();
+  for (Condition* condition : m_conditions) {
     if (condition->getType() == Condition::Syncbase &&
-        condition->syncBase() == syncBase) {
+        condition->syncBaseEquals(syncBase)) {
       ASSERT(condition->name() == "begin" || condition->name() == "end");
       // No nested time containers in SVG, no need for crazy time space
       // conversions. Phew!
       SMILTime time = 0;
       if (condition->name() == "begin")
-        time = syncBase->m_interval.begin + condition->offset();
+        time = syncBase.m_interval.begin + condition->offset();
       else
-        time = syncBase->m_interval.end + condition->offset();
+        time = syncBase.m_interval.end + condition->offset();
       if (!time.isFinite())
         continue;
       SMILTime elapsed = this->elapsed();
@@ -1216,14 +1221,14 @@ void SVGSMILElement::createInstanceTimesFromSyncbase(SVGSMILElement* syncBase) {
   }
 }
 
-void SVGSMILElement::addSyncBaseDependent(SVGSMILElement* animation) {
-  m_syncBaseDependents.insert(animation);
+void SVGSMILElement::addSyncBaseDependent(SVGSMILElement& animation) {
+  m_syncBaseDependents.insert(&animation);
   if (m_interval.begin.isFinite())
-    animation->createInstanceTimesFromSyncbase(this);
+    animation.createInstanceTimesFromSyncbase(*this);
 }
 
-void SVGSMILElement::removeSyncBaseDependent(SVGSMILElement* animation) {
-  m_syncBaseDependents.erase(animation);
+void SVGSMILElement::removeSyncBaseDependent(SVGSMILElement& animation) {
+  m_syncBaseDependents.erase(&animation);
 }
 
 void SVGSMILElement::handleConditionEvent(Event* event, Condition* condition) {
@@ -1302,13 +1307,6 @@ void SVGSMILElement::unscheduleIfScheduled() {
   m_isScheduled = false;
 }
 
-SVGSMILElement::Condition::~Condition() {}
-
-DEFINE_TRACE(SVGSMILElement::Condition) {
-  visitor->trace(m_syncBase);
-  visitor->trace(m_eventListener);
-}
-
 DEFINE_TRACE(SVGSMILElement) {
   visitor->trace(m_targetElement);
   visitor->trace(m_timeContainer);