Drive SVG Animations via requestAnimationFrame

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 c41555d4d18203e592dc1c81d5ce2f275f8c3608..936d795478328df13d2e1e0fdf8f7ef5b7692864 100644
--- a/Source/core/svg/animation/SMILTimeContainer.cpp
+++ b/Source/core/svg/animation/SMILTimeContainer.cpp
@@ -27,6 +27,7 @@
 #include "core/svg/animation/SMILTimeContainer.h"
 
 #include "core/dom/ElementTraversal.h"
+#include "core/frame/FrameView.h"
 #include "core/svg/SVGSVGElement.h"
 #include "core/svg/animation/SVGSMILElement.h"
 #include "wtf/CurrentTime.h"
@@ -35,7 +36,8 @@ using namespace std;
 
 namespace WebCore {
 
-static const double animationFrameDelay = 0.025;
+// Blatantly copied from core/animation/DocumentTimeline.cpp.
+static const double minimumDelay = 0.04;

[pdr., 2014/03/03 17:47:33]

Can you use DocumentTimeline::s_minimumDelay directly instead of copying it?

[dstockwell, 2014/03/04 01:19:04]

I'm OK with this duplication for now, but this logic (schedule-frame-after-time)
should likely be pushed up to PageAnimator at some point.

[fs, 2014/03/04 10:28:19]

I suppose we can trade this two line hack for a one-line hack
(friend-declaration in DocumentTimeline). I do hope we'll be able to consolidate
and converge reasonably quickly here though. Unifying this sounds reasonable.

 
 SMILTimeContainer::SMILTimeContainer(SVGSVGElement* owner)
     : m_beginTime(0)
@@ -44,7 +46,9 @@ SMILTimeContainer::SMILTimeContainer(SVGSVGElement* owner)
     , m_accumulatedActiveTime(0)
     , m_presetStartTime(0)
     , m_documentOrderIndexesDirty(false)
-    , m_timer(this, &SMILTimeContainer::timerFired)
+    , m_framePending(false)
+    , m_clockFrozen(false)

[pdr., 2014/03/03 17:47:33]

I'm wary of adding more clock-related state to this already complex state
machine. Unfortunately I tried to combine m_clockFrozen into another variable
and I don't see an easy way to do it.

Can you think of a way to simplify this? I'm happy if we need it but I'd like to
avoid more state if possible.

[fs, 2014/03/04 10:28:19]

I will try to formalize this somewhat - probably by migrating to AnimationClock,
and either extend it or inherit from it (preferably the former). I'd really like
to come up with a long-term solution for this though...

+    , m_wakeupTimer(this, &SMILTimeContainer::wakeupTimerFired)
     , m_ownerSVGElement(owner)
 #ifndef NDEBUG
     , m_preventScheduledAnimationsChanges(false)
@@ -55,7 +59,7 @@ SMILTimeContainer::SMILTimeContainer(SVGSVGElement* owner)
 SMILTimeContainer::~SMILTimeContainer()
 {
     cancelAnimationFrame();
-    ASSERT(!m_timer.isActive());
+    ASSERT(!m_wakeupTimer.isActive());
 #ifndef NDEBUG
     ASSERT(!m_preventScheduledAnimationsChanges);
 #endif
@@ -99,6 +103,11 @@ void SMILTimeContainer::unschedule(SVGSMILElement* animation, SVGElement* target
     scheduled->remove(idx);
 }
 
+bool SMILTimeContainer::hasAnimations() const
+{
+    return !m_scheduledAnimations.isEmpty();
+}
+
 void SMILTimeContainer::notifyIntervalsChanged()
 {
     // Schedule updateAnimations() to be called asynchronously so multiple intervals
@@ -108,7 +117,7 @@ void SMILTimeContainer::notifyIntervalsChanged()
 
 SMILTime SMILTimeContainer::elapsed() const
 {
-    if (!m_beginTime)
+    if (!m_beginTime || m_clockFrozen)
         return 0;
 
     if (isPaused())
@@ -135,12 +144,15 @@ void SMILTimeContainer::begin()
     // If 'm_presetStartTime' is set, the timeline was modified via setElapsed() before the document began.
     // In this case pass on 'seekToTime=true' to updateAnimations().
     m_beginTime = now - m_presetStartTime;
-    updateAnimations(SMILTime(m_presetStartTime), m_presetStartTime ? true : false);
+    bool hadPresetStartTime = m_presetStartTime ? true : false;
+    updateAnimations(SMILTime(m_presetStartTime), hadPresetStartTime);
     m_presetStartTime = 0;
 
     if (m_pauseTime) {
         m_pauseTime = now;
         cancelAnimationFrame();
+    } else {
+        m_clockFrozen = !hadPresetStartTime;
     }
 }
 
@@ -154,6 +166,7 @@ void SMILTimeContainer::pause()
         cancelAnimationFrame();
     }
     m_resumeTime = 0;
+    m_clockFrozen = false;
 }
 
 void SMILTimeContainer::resume()
@@ -163,6 +176,7 @@ void SMILTimeContainer::resume()
 
     m_pauseTime = 0;
     scheduleAnimationFrame();
+    m_clockFrozen = false;
 }
 
 void SMILTimeContainer::setElapsed(SMILTime time)
@@ -173,6 +187,8 @@ void SMILTimeContainer::setElapsed(SMILTime time)
         return;
     }
 
+    m_clockFrozen = false;
+
     if (m_beginTime)

[pdr., 2014/03/03 17:47:33]

How did this if statement get here? :P

[fs, 2014/03/04 10:28:19]

Magic? =P

Maybe it should've been some other condition... I'll just drop it for now.

         cancelAnimationFrame();
 
@@ -216,8 +232,11 @@ void SMILTimeContainer::scheduleAnimationFrame(SMILTime fireTime)
     if (!fireTime.isFinite())
         return;
 
-    SMILTime delay = max(fireTime - elapsed(), SMILTime(animationFrameDelay));
-    m_timer.startOneShot(delay.value());
+    SMILTime delay = fireTime - elapsed();
+    if (delay.value() < minimumDelay)
+        serviceOnNextFrame();
+    else
+        m_wakeupTimer.startOneShot(delay.value() - minimumDelay);
 }
 
 void SMILTimeContainer::scheduleAnimationFrame()
@@ -225,18 +244,21 @@ void SMILTimeContainer::scheduleAnimationFrame()
     if (!isTimelineRunning())
         return;
 
-    m_timer.startOneShot(0);
+    // Could also schedule a wakeup at +0 seconds, but that could still
+    // potentially race with the servicing of the next frame.
+    serviceOnNextFrame();
 }
 
 void SMILTimeContainer::cancelAnimationFrame()
 {
-    m_timer.stop();
+    m_framePending = false;
+    m_wakeupTimer.stop();
 }
 
-void SMILTimeContainer::timerFired(Timer<SMILTimeContainer>*)
+void SMILTimeContainer::wakeupTimerFired(Timer<SMILTimeContainer>*)
 {
     ASSERT(isTimelineRunning());
-    updateAnimations(elapsed());
+    serviceOnNextFrame();
 }
 
 void SMILTimeContainer::updateDocumentOrderIndexes()
@@ -266,6 +288,30 @@ struct PriorityCompare {
     SMILTime m_elapsed;
 };
 
+Document& SMILTimeContainer::document() const
+{
+    ASSERT(m_ownerSVGElement);
+    return m_ownerSVGElement->document();
+}
+
+void SMILTimeContainer::serviceOnNextFrame()
+{
+    if (document().view()) {
+        document().view()->scheduleAnimation();
+        m_framePending = true;
+    }
+}
+
+void SMILTimeContainer::serviceAnimations(double monotonicAnimationStartTime)

[dstockwell, 2014/03/04 01:19:04]

This patch just moves updates to be driven by the frame callback, but does not
use the frame times yet? Is that planned?

[fs, 2014/03/04 10:28:19]

Yes, I plan to that in follow up CLs. Had originally thought that I might be
able to make use of it here directly (in this CL), but figured it might just be
"too much" at the same time. If it ends up being unused I'll de-plumb it and
move it to a/the later CL.

+{
+    if (!m_framePending)
+        return;
+
+    m_framePending = false;
+    updateAnimations(elapsed());
+    m_clockFrozen = false;
+}
+
 void SMILTimeContainer::updateAnimations(SMILTime elapsed, bool seekToTime)
 {
     SMILTime earliestFireTime = SMILTime::unresolved();