Animations sometimes fail to start
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Jul 2015 01:57:30 +0000 (01:57 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Jul 2015 01:57:30 +0000 (01:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=147394
rdar://problem/21852603

Reviewed by Dean Jackson.
Source/WebCore:

When an accelerated animation or transition was started at the same time as
a non-accelerated one, and then the node for the former was removed, we could
never kick off the non-accelerated animation.

AnimationControllerPrivate has logic to synchronize the two types of animation
when they start in the same animation update, which involves setting the
m_waitingForAsyncStartNotification flag, and waiting for a notifyAnimationStarted()
to come in from the graphics system.

However, it failed to handle the case where the accelerated animation was removed
before the callback was received, which left the m_waitingForAsyncStartNotification flag
set to true, preventing the non-accelerated animation from running.

Test: animations/remove-syncing-animation.html

* page/animation/AnimationBase.h:
(WebCore::AnimationBase::isAccelerated): Make this public.
* page/animation/AnimationController.cpp:
(WebCore::AnimationControllerPrivate::clear): Add logging.
(WebCore::AnimationControllerPrivate::receivedStartTimeResponse): Add logging.
(WebCore::AnimationControllerPrivate::animationWillBeRemoved): Add logging.
After removing animations from the maps, check to see if we expect any of the
remaining animations are waiting for a notifyAnimationStarted(). If not, clear
the m_waitingForAsyncStartNotification flag.
(WebCore::AnimationController::notifyAnimationStarted): Log the renderer.
(WebCore::AnimationControllerPrivate::AnimationControllerPrivate): Remove unneeded
initializations of HashMaps.
* page/animation/CompositeAnimation.cpp:
(WebCore::CompositeAnimation::updateTransitions): Log renderers.
(WebCore::CompositeAnimation::updateKeyframeAnimations): Ditto.

LayoutTests:

Test that starts an accelerated and non-accelerated animation, then removes
the node for the accelerated one.

* animations/remove-syncing-animation-expected.txt: Added.
* animations/remove-syncing-animation.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@187535 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/animations/remove-syncing-animation-expected.txt [new file with mode: 0644]
LayoutTests/animations/remove-syncing-animation.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/animation/AnimationBase.h
Source/WebCore/page/animation/AnimationController.cpp
Source/WebCore/page/animation/CompositeAnimation.cpp

index b4c1af5..84b950d 100644 (file)
@@ -1,3 +1,17 @@
+2015-07-28  Simon Fraser  <simon.fraser@apple.com>
+
+        Animations sometimes fail to start
+        https://bugs.webkit.org/show_bug.cgi?id=147394
+        rdar://problem/21852603
+
+        Reviewed by Dean Jackson.
+        
+        Test that starts an accelerated and non-accelerated animation, then removes
+        the node for the accelerated one.
+
+        * animations/remove-syncing-animation-expected.txt: Added.
+        * animations/remove-syncing-animation.html: Added.
+
 2015-07-28  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [Freetype] Always allow font matching for strong aliases
diff --git a/LayoutTests/animations/remove-syncing-animation-expected.txt b/LayoutTests/animations/remove-syncing-animation-expected.txt
new file mode 100644 (file)
index 0000000..04fd8d8
--- /dev/null
@@ -0,0 +1,5 @@
+The remaining square should show a color animation from orange to green.
+
+Color keyframes.
+PASS - colors animation ran.
+
diff --git a/LayoutTests/animations/remove-syncing-animation.html b/LayoutTests/animations/remove-syncing-animation.html
new file mode 100644 (file)
index 0000000..9eb6be2
--- /dev/null
@@ -0,0 +1,93 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        #container {
+            
+        }
+        
+        .box {
+            height: 100px;
+            width: 100px;
+            margin: 10px;
+            background-color: blue;
+        }
+
+        #container.changed #first {
+            -webkit-animation: fade 0.1s linear;
+        }
+
+        #container.changed #second {
+            -webkit-animation: colors 0.1s linear;
+        }
+        
+        @-webkit-keyframes fade {
+            from { opacity: 0; }
+            to { opacity: 1; }
+        }
+
+        @-webkit-keyframes colors {
+            from { background-color: orange; }
+            to { background-color: green; }
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.waitUntilDone();
+            testRunner.dumpAsText();
+        }
+
+        function log(message)
+        {
+            var results = document.getElementById("results");
+            results.innerHTML = results.innerHTML + message + "<br>";
+        }
+
+        var watchdogTimeoutID;
+
+        function testComplete()
+        {
+            window.clearTimeout(watchdogTimeoutID);
+            watchdogTimeoutID = 0;
+            log('PASS - colors animation ran.');
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+        
+        function doTest()
+        {
+            window.setTimeout(function() {
+                document.getElementById('second').addEventListener('animationend', testComplete);
+                document.getElementById('container').classList.add('changed');
+                
+                window.setTimeout(function() {
+                    document.getElementById('first').remove();
+                }, 0);
+            }, 0);
+
+            watchdogTimeoutID = window.setTimeout(function() {
+                log('FAIL - colors animation did not run.');
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 500);
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+
+<p>The remaining square should show a color animation from orange to green.</p>
+<div id="container">
+    <div id="first" class="box">
+        Opacity keyframes.
+    </div>
+    <div id="second" class="box">
+        Color keyframes.
+    </div>
+</div>
+
+<div id="results"></div>
+</body>
+</html>
index cf825d8..35704aa 100644 (file)
@@ -1,3 +1,42 @@
+2015-07-28  Simon Fraser  <simon.fraser@apple.com>
+
+        Animations sometimes fail to start
+        https://bugs.webkit.org/show_bug.cgi?id=147394
+        rdar://problem/21852603
+
+        Reviewed by Dean Jackson.
+
+        When an accelerated animation or transition was started at the same time as
+        a non-accelerated one, and then the node for the former was removed, we could
+        never kick off the non-accelerated animation.
+        
+        AnimationControllerPrivate has logic to synchronize the two types of animation
+        when they start in the same animation update, which involves setting the
+        m_waitingForAsyncStartNotification flag, and waiting for a notifyAnimationStarted()
+        to come in from the graphics system.
+        
+        However, it failed to handle the case where the accelerated animation was removed
+        before the callback was received, which left the m_waitingForAsyncStartNotification flag
+        set to true, preventing the non-accelerated animation from running.
+        
+        Test: animations/remove-syncing-animation.html
+
+        * page/animation/AnimationBase.h:
+        (WebCore::AnimationBase::isAccelerated): Make this public.
+        * page/animation/AnimationController.cpp:
+        (WebCore::AnimationControllerPrivate::clear): Add logging.
+        (WebCore::AnimationControllerPrivate::receivedStartTimeResponse): Add logging.
+        (WebCore::AnimationControllerPrivate::animationWillBeRemoved): Add logging.
+        After removing animations from the maps, check to see if we expect any of the
+        remaining animations are waiting for a notifyAnimationStarted(). If not, clear
+        the m_waitingForAsyncStartNotification flag.
+        (WebCore::AnimationController::notifyAnimationStarted): Log the renderer.
+        (WebCore::AnimationControllerPrivate::AnimationControllerPrivate): Remove unneeded
+        initializations of HashMaps.
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimation::updateTransitions): Log renderers.
+        (WebCore::CompositeAnimation::updateKeyframeAnimations): Ditto.
+
 2015-07-28  Dean Jackson  <dino@apple.com>
 
         Remove dispatch_apply_f and instead use vImage more directly
index a54145b..0f07332 100644 (file)
@@ -131,6 +131,8 @@ public:
     bool waitingForStartTime() const { return m_animationState == AnimationState::StartWaitResponse; }
     bool waitingForStyleAvailable() const { return m_animationState == AnimationState::StartWaitStyleAvailable; }
 
+    bool isAccelerated() const { return m_isAccelerated; }
+
     virtual double timeToNextService();
 
     double progress(double scale = 1, double offset = 0, const TimingFunction* = nullptr) const;
@@ -230,7 +232,6 @@ protected:
 
     void goIntoEndingOrLoopingState();
 
-    bool isAccelerated() const { return m_isAccelerated; }
     AnimationState state() const { return m_animationState; }
 
     static void setNeedsStyleRecalc(Element*);
index 3f73d36..38e18cc 100644 (file)
@@ -72,8 +72,6 @@ AnimationControllerPrivate::AnimationControllerPrivate(Frame& frame)
     , m_updateStyleIfNeededDispatcher(*this, &AnimationControllerPrivate::updateStyleIfNeededDispatcherFired)
     , m_frame(frame)
     , m_beginAnimationUpdateTime(cBeginAnimationUpdateTimeNotSet)
-    , m_animationsWaitingForStyle()
-    , m_animationsWaitingForStartTimeResponse()
     , m_beginAnimationUpdateCount(0)
     , m_waitingForAsyncStartNotification(false)
     , m_isSuspended(false)
@@ -98,6 +96,8 @@ CompositeAnimation& AnimationControllerPrivate::ensureCompositeAnimation(RenderE
 
 bool AnimationControllerPrivate::clear(RenderElement& renderer)
 {
+    LOG(Animations, "AnimationControllerPrivate %p clear: %p", this, &renderer);
+
     ASSERT(renderer.isCSSAnimating());
     ASSERT(m_compositeAnimations.contains(&renderer));
 
@@ -410,6 +410,8 @@ void AnimationControllerPrivate::endAnimationUpdate()
 
 void AnimationControllerPrivate::receivedStartTimeResponse(double time)
 {
+    LOG(Animations, "AnimationControllerPrivate %p receivedStartTimeResponse %f", this, time);
+
     m_waitingForAsyncStartNotification = false;
     startTimeResponse(time);
 }
@@ -521,11 +523,24 @@ void AnimationControllerPrivate::startTimeResponse(double time)
 
 void AnimationControllerPrivate::animationWillBeRemoved(AnimationBase* animation)
 {
+    LOG(Animations, "AnimationControllerPrivate %p animationWillBeRemoved: %p", this, animation);
+
     removeFromAnimationsWaitingForStyle(animation);
     removeFromAnimationsWaitingForStartTimeResponse(animation);
 #if ENABLE(CSS_ANIMATIONS_LEVEL_2)
     removeFromAnimationsDependentOnScroll(animation);
 #endif
+
+    bool anyAnimationsWaitingForAsyncStart = false;
+    for (auto& animation : m_animationsWaitingForStartTimeResponse) {
+        if (animation->waitingForStartTime() && animation->isAccelerated()) {
+            anyAnimationsWaitingForAsyncStart = true;
+            break;
+        }
+    }
+
+    if (!anyAnimationsWaitingForAsyncStart)
+        m_waitingForAsyncStartNotification = false;
 }
 
 #if ENABLE(CSS_ANIMATIONS_LEVEL_2)
@@ -633,8 +648,11 @@ bool AnimationController::computeExtentOfAnimation(RenderElement& renderer, Layo
     return m_data->computeExtentOfAnimation(renderer, bounds);
 }
 
-void AnimationController::notifyAnimationStarted(RenderElement&, double startTime)
+void AnimationController::notifyAnimationStarted(RenderElement& renderer, double startTime)
 {
+    LOG(Animations, "AnimationController %p notifyAnimationStarted on renderer %p, time=%f", this, &renderer, startTime);
+    UNUSED_PARAM(renderer);
+
     AnimationUpdateBlock animationUpdateBlock(this);
     m_data->receivedStartTimeResponse(startTime);
 }
index 07d84e0..0fb4cf9 100644 (file)
@@ -168,7 +168,7 @@ void CompositeAnimation::updateTransitions(RenderElement* renderer, RenderStyle*
                 if (!equal && isActiveTransition) {
                     // Add the new transition
                     RefPtr<ImplicitAnimation> implicitAnimation = ImplicitAnimation::create(animation, prop, renderer, this, modifiedCurrentStyle ? modifiedCurrentStyle.get() : fromStyle);
-                    LOG(Animations, "Created ImplicitAnimation %p for property %s duration %.2f delay %.2f", implicitAnimation.get(), getPropertyName(prop), animation.duration(), animation.delay());
+                    LOG(Animations, "Created ImplicitAnimation %p on renderer %p for property %s duration %.2f delay %.2f", implicitAnimation.get(), renderer, getPropertyName(prop), animation.duration(), animation.delay());
                     m_transitions.set(prop, implicitAnimation.release());
                 }
                 
@@ -185,7 +185,7 @@ void CompositeAnimation::updateTransitions(RenderElement* renderer, RenderStyle*
         if (!transition->active()) {
             animationController().animationWillBeRemoved(transition.get());
             toBeRemoved.append(transition->animatingProperty());
-            LOG(Animations, "Removing ImplicitAnimation %p for property %s", transition.get(), getPropertyName(transition->animatingProperty()));
+            LOG(Animations, "Removing ImplicitAnimation %p from renderer %p for property %s", transition.get(), renderer, getPropertyName(transition->animatingProperty()));
         }
     }
 
@@ -256,7 +256,7 @@ void CompositeAnimation::updateKeyframeAnimations(RenderElement* renderer, Rende
                     keyframeAnim->setIndex(i);
                 } else if ((animation.duration() || animation.delay()) && animation.iterationCount() && animationName != none) {
                     keyframeAnim = KeyframeAnimation::create(animation, renderer, i, this, targetStyle);
-                    LOG(Animations, "Creating KeyframeAnimation %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
+                    LOG(Animations, "Creating KeyframeAnimation %p on renderer %p with keyframes %s, duration %.2f, delay %.2f, iterations %.2f", keyframeAnim.get(), renderer, animation.name().utf8().data(), animation.duration(), animation.delay(), animation.iterationCount());
                     if (m_suspended) {
                         keyframeAnim->updatePlayState(AnimPlayStatePaused);
                         LOG(Animations, "  (created in suspended/paused state)");
@@ -288,7 +288,7 @@ void CompositeAnimation::updateKeyframeAnimations(RenderElement* renderer, Rende
             animsToBeRemoved.append(animation->name().impl());
             animationController().animationWillBeRemoved(animation.get());
             animation->clear();
-            LOG(Animations, "Removing KeyframeAnimation %p", animation.get());
+            LOG(Animations, "Removing KeyframeAnimation %p from renderer %p", animation.get(), renderer);
         }
     }