Prepare SMILTimeContainer for oilpan

Source/core/svg/animation/SMILTimeContainer.cpp

Index: Source/core/svg/animation/SMILTimeContainer.cpp
diff --git a/Source/core/svg/animation/SMILTimeContainer.cpp b/Source/core/svg/animation/SMILTimeContainer.cpp
index 43db4f317fafcdceec09fdbfac5177de78ba175b..94a71ae421a2d8a75d947fed5a851f91a08558dc 100644
--- a/Source/core/svg/animation/SMILTimeContainer.cpp
+++ b/Source/core/svg/animation/SMILTimeContainer.cpp
@@ -87,11 +87,11 @@ void SMILTimeContainer::schedule(SVGSMILElement* animation, SVGElement* target,
 #endif
 
     ElementAttributePair key(target, attributeName);
-    OwnPtr<AnimationsVector>& scheduled = m_scheduledAnimations.add(key, nullptr).storedValue->value;
+    OwnPtrWillBeMember<AnimationsLinkedHashSet>& scheduled = m_scheduledAnimations.add(key, nullptr).storedValue->value;
     if (!scheduled)
-        scheduled = adoptPtr(new AnimationsVector);
+        scheduled = adoptPtrWillBeNoop(new AnimationsLinkedHashSet);
     ASSERT(!scheduled->contains(animation));
-    scheduled->append(animation);
+    scheduled->add(animation);
 
     SMILTime nextFireTime = animation->nextProgressTime();
     if (nextFireTime.isFinite())
@@ -107,11 +107,16 @@ void SMILTimeContainer::unschedule(SVGSMILElement* animation, SVGElement* target
 #endif
 
     ElementAttributePair key(target, attributeName);
-    AnimationsVector* scheduled = m_scheduledAnimations.get(key);
+    GroupedAnimationsMap::iterator it = m_scheduledAnimations.find(key);
+    ASSERT(it != m_scheduledAnimations.end());
+    AnimationsLinkedHashSet* scheduled = it->value;
     ASSERT(scheduled);
-    size_t idx = scheduled->find(animation);
-    ASSERT(idx != kNotFound);
-    scheduled->remove(idx);
+    AnimationsLinkedHashSet::iterator itAnimation = scheduled->find(animation);
+    ASSERT(itAnimation != scheduled->end());
+    scheduled->remove(itAnimation);
+
+    if (scheduled->isEmpty())
+        m_scheduledAnimations.remove(it);

[haraken, 2014/05/27 08:25:56]

Just help me understand: Having this remove() looks fine, but how was the
previous code working without having the remove()?

[kouhei (in TOK), 2014/05/27 08:50:51]

I believe the invalid pointer was just there, but as |scheduled| was empty,
there weren't any problem.

 }
 
 bool SMILTimeContainer::hasAnimations() const
@@ -235,10 +240,9 @@ void SMILTimeContainer::setElapsed(SMILTime time)
 #endif
     GroupedAnimationsMap::iterator end = m_scheduledAnimations.end();
     for (GroupedAnimationsMap::iterator it = m_scheduledAnimations.begin(); it != end; ++it) {

[Erik Corry, 2014/05/27 08:55:23]

I think you should check for null keys here too.  It may not be necessary to
remove them.

[kouhei (in TOK), 2014/05/28 01:00:50]

Done.

-        AnimationsVector* scheduled = it->value.get();
-        unsigned size = scheduled->size();
-        for (unsigned n = 0; n < size; n++)
-            scheduled->at(n)->reset();
+        AnimationsLinkedHashSet* scheduled = it->value.get();
+        for (AnimationsLinkedHashSet::const_iterator itAnimation = scheduled->begin(), itAnimationEnd = scheduled->end(); itAnimation != itAnimationEnd; ++itAnimation)
+            (*itAnimation)->reset();
     }
 #ifndef NDEBUG
     m_preventScheduledAnimationsChanges = false;
@@ -376,21 +380,25 @@ SMILTime SMILTimeContainer::updateAnimations(SMILTime elapsed, bool seekToTime)
     if (m_documentOrderIndexesDirty)
         updateDocumentOrderIndexes();
 
-    WillBeHeapVector<RefPtrWillBeMember<SVGSMILElement> >  animationsToApply;
-    GroupedAnimationsMap::iterator end = m_scheduledAnimations.end();
-    for (GroupedAnimationsMap::iterator it = m_scheduledAnimations.begin(); it != end; ++it) {
-        AnimationsVector* scheduled = it->value.get();
+    WillBeHeapHashSet<ElementAttributePair> invalidKeys;
+    AnimationsVector animationsToApply;
+    for (GroupedAnimationsMap::iterator it = m_scheduledAnimations.begin(), end = m_scheduledAnimations.end(); it != end; ++it) {
+        if (!it->key.first || !it->value || it->value->isEmpty())

[haraken, 2014/05/27 08:25:56]

I don't know if we need the !it->key.first check. It would depend on how the
weak processing for HeapHashMap<pair<WeakMember<X>, Y>, Z> works. When X dies,
is the associated entry removed from the heap hash map?

Also I wonder if we need the !it->value check.

[Erik Corry, 2014/05/27 08:41:48]

No, the associated entry is not removed, that's why we need this code.
I guess we don't

[kouhei (in TOK), 2014/05/27 08:50:51]

Ack.

[kouhei (in TOK), 2014/05/28 01:00:50]

Done.

+            invalidKeys.add(it->key);

[Erik Corry, 2014/05/27 08:28:06]

Should there not also be "continue;" here?

[kouhei (in TOK), 2014/05/27 08:50:51]

Yes! Ack.

[kouhei (in TOK), 2014/05/28 01:00:50]

Done.

+
+        AnimationsLinkedHashSet* scheduled = it->value.get();
 
         // Sort according to priority. Elements with later begin time have higher priority.
         // In case of a tie, document order decides.
         // FIXME: This should also consider timing relationships between the elements. Dependents
         // have higher priority.
-        std::sort(scheduled->begin(), scheduled->end(), PriorityCompare(elapsed));
+        AnimationsVector scheduledAnimations;
+        copyToVector(*scheduled, scheduledAnimations);
+        std::sort(scheduledAnimations.begin(), scheduledAnimations.end(), PriorityCompare(elapsed));
 
         SVGSMILElement* resultElement = 0;
-        unsigned size = scheduled->size();
-        for (unsigned n = 0; n < size; n++) {
-            SVGSMILElement* animation = scheduled->at(n);
+        for (AnimationsVector::const_iterator itAnimation = scheduledAnimations.begin(), itAnimationEnd = scheduledAnimations.end(); itAnimation != itAnimationEnd; ++itAnimation) {
+            SVGSMILElement* animation = *itAnimation;
             ASSERT(animation->timeContainer() == this);
             ASSERT(animation->targetElement());
             ASSERT(animation->hasValidAttributeName());
@@ -415,6 +423,7 @@ SMILTime SMILTimeContainer::updateAnimations(SMILTime elapsed, bool seekToTime)
         if (resultElement)
             animationsToApply.append(resultElement);
     }
+    m_scheduledAnimations.removeAll(invalidKeys);

[haraken, 2014/05/27 08:25:56]

Ditto. I'm curious how the previous code was working without this removeAll().

[kouhei (in TOK), 2014/05/27 08:50:51]

if !ENABLE(OILPAN), |unschedule| is guaranteed to be called. if ENABLE(OILPAN)
it is processed through weak processing in (HeapHashMap<pair<Weak, attr>, >), so
invalid keys remain.

 
     std::sort(animationsToApply.begin(), animationsToApply.end(), PriorityCompare(elapsed));
 
@@ -452,4 +461,9 @@ SMILTime SMILTimeContainer::updateAnimations(SMILTime elapsed, bool seekToTime)
     return earliestFireTime;
 }
 
+void SMILTimeContainer::trace(Visitor* visitor)
+{
+    visitor->trace(m_scheduledAnimations);
+}
+
 }