Prepare SMILTimeContainer for oilpan

Source/core/svg/animation/SMILTimeContainer.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 69 matching lines @@
 {
     ASSERT(animation->timeContainer() == this);
     ASSERT(target);
     ASSERT(animation->hasValidAttributeName());
 
 #ifndef NDEBUG
     ASSERT(!m_preventScheduledAnimationsChanges);
 #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())
         notifyIntervalsChanged();
 }
 
 void SMILTimeContainer::unschedule(SVGSMILElement* animation, SVGElement* target, const QualifiedName& attributeName)
 {
     ASSERT(animation->timeContainer() == this);
 
 #ifndef NDEBUG
     ASSERT(!m_preventScheduledAnimationsChanges);
 #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
 {
     return !m_scheduledAnimations.isEmpty();
 }
 
 bool SMILTimeContainer::hasPendingSynchronization() const
 {
     return m_frameSchedulingState == SynchronizeAnimations && m_wakeupTimer.isActive() && !m_wakeupTimer.nextFireInterval();
@@ skipping 102 matching lines @@
         m_pauseTime = now;
         m_accumulatedActiveTime = time.value();
     } else {
         m_accumulatedActiveTime = 0;
     }
 
 #ifndef NDEBUG
     m_preventScheduledAnimationsChanges = true;
 #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;
 #endif
 
     updateAnimationsAndScheduleFrameIfNeeded(time, true);
 }
 
 bool SMILTimeContainer::isTimelineRunning() const
 {
@@ skipping 117 matching lines @@
 
 #ifndef NDEBUG
     // This boolean will catch any attempts to schedule/unschedule scheduledAnimations during this critical section.
     // Similarly, any elements removed will unschedule themselves, so this will catch modification of animationsToApply.
     m_preventScheduledAnimationsChanges = true;
 #endif
 
     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());
 
             // Results are accumulated to the first animation that animates and contributes to a particular element/attribute pair.
             // FIXME: we should ensure that resultElement is of an appropriate type.
             if (!resultElement) {
                 if (!animation->hasValidAttributeType())
                     continue;
                 resultElement = animation;
             }
 
             // This will calculate the contribution from the animation and add it to the resultsElement.
             if (!animation->progress(elapsed, resultElement, seekToTime) && resultElement == animation)
                 resultElement = 0;
 
             SMILTime nextFireTime = animation->nextProgressTime();
             if (nextFireTime.isFinite())
                 earliestFireTime = min(nextFireTime, earliestFireTime);
         }
 
         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));
 
     unsigned animationsToApplySize = animationsToApply.size();
     if (!animationsToApplySize) {
 #ifndef NDEBUG
         m_preventScheduledAnimationsChanges = false;
 #endif
         return earliestFireTime;
     }
@@ skipping 17 matching lines @@
 
             if (animDiscard->inDocument()) {
                 animDiscard->remove(IGNORE_EXCEPTION);
                 ASSERT(!animDiscard->inDocument());
             }
         }
     }
     return earliestFireTime;
 }
 
+void SMILTimeContainer::trace(Visitor* visitor)
+{
+    visitor->trace(m_scheduledAnimations);
 }
+
+}