2009-04-10 Chris Marrin <cmarrin@apple.com>
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Apr 2009 23:19:00 +0000 (23:19 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Apr 2009 23:19:00 +0000 (23:19 +0000)
        Reviewed by Dan Bernstein

        https://bugs.webkit.org/show_bug.cgi?id=25108

        If you remove a class with a transition while that transition is running
        the animation timer will continue to fire after the transition is finished.
        This has no visual indication, but it does drain the processor. And in some
        cases it might even cause a glitch in future animations. Unfortunately there
        is no way to test this without putting in printfs.

        This happens because the animation logic is never traversed after a transition
        is removed, so we never get a chance to cleanup. So I added cleanup in the logic
        that fires the dispatch of the last style change when the animation finishes.

        Test: transitions/remove-transition-style.html

        * page/animation/AnimationController.cpp:
        (WebCore::AnimationControllerPrivate::updateStyleIfNeededDispatcherFired):
        * page/animation/CompositeAnimation.cpp:
        (WebCore::CompositeAnimation::updateTransitions):
        (WebCore::CompositeAnimation::animate):
        (WebCore::CompositeAnimation::cleanupFinishedAnimations):
        * page/animation/CompositeAnimation.h:

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

LayoutTests/ChangeLog
LayoutTests/transitions/remove-transition-style-expected.txt [new file with mode: 0644]
LayoutTests/transitions/remove-transition-style.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/page/animation/AnimationController.cpp
WebCore/page/animation/CompositeAnimation.cpp
WebCore/page/animation/CompositeAnimation.h

index 437a551..6bd562b 100644 (file)
@@ -1,3 +1,16 @@
+2009-04-10  Chris Marrin  <cmarrin@apple.com>
+
+        Reviewed by Dan Bernstein
+        
+        https://bugs.webkit.org/show_bug.cgi?id=25108
+
+        Testcase which reproduces the issue described in bug 25108.
+        Won't actually reveal failure, since we have no way to test
+        whether animation timers keep firing from DRT.
+
+        * transitions/remove-transition-style-expected.txt: Added.
+        * transitions/remove-transition-style.html: Added.
+
 2009-04-10  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by Adam Roben.
diff --git a/LayoutTests/transitions/remove-transition-style-expected.txt b/LayoutTests/transitions/remove-transition-style-expected.txt
new file mode 100644 (file)
index 0000000..6ed080c
--- /dev/null
@@ -0,0 +1,2 @@
+No running transitions: PASS
+
diff --git a/LayoutTests/transitions/remove-transition-style.html b/LayoutTests/transitions/remove-transition-style.html
new file mode 100644 (file)
index 0000000..371f81e
--- /dev/null
@@ -0,0 +1,70 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
+   "http://www.w3.org/TR/html4/loose.dtd">
+
+<html lang="en">
+<head>
+  <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
+  <title>Dynamic transition removal</title>
+  <style type="text/css" media="screen">
+    #box {
+      height: 200px;
+      width: 200px;
+      margin: 20px;
+      background-color: blue;
+    }
+    
+    .animated {
+      -webkit-transition: opacity 200ms;
+    }
+  </style>
+  <script type="text/javascript" charset="utf-8">
+    if (window.layoutTestController) {
+      layoutTestController.waitUntilDone();
+      layoutTestController.dumpAsText();
+    }
+
+    function log(s)
+    {
+      var results = document.getElementById('results');
+      results.innerHTML += s + '<br>';
+    }
+
+    function testTransitions()
+    {
+      if (window.layoutTestController) {
+        var numAnims = layoutTestController.numberOfActiveAnimations();
+        if (numAnims == 0)
+          log('No running transitions: PASS');
+        else
+          log('Still ' + numAnims + ' transitions running: FAIL')
+
+        layoutTestController.notifyDone();
+      }
+    }
+    
+    function removeStyle()
+    {
+      box.className = '';
+      window.setTimeout(testTransitions, 50);
+    }
+
+    function startTransition()
+    {
+      var box = document.getElementById('box');
+      box.className = 'animated';
+      window.setTimeout(function() {
+        box.style.opacity = '0.5';
+        window.setTimeout(removeStyle, 200);
+      }, 0);
+    }
+    
+    window.addEventListener('load', startTransition, false);
+  </script>
+</head>
+<body>
+
+<div id="box"></div>
+
+<div id="results"></div>
+</body>
+</html>
\ No newline at end of file
index 8cd78f4..35a7b91 100644 (file)
@@ -1,3 +1,29 @@
+2009-04-10  Chris Marrin  <cmarrin@apple.com>
+
+        Reviewed by Dan Bernstein
+
+        https://bugs.webkit.org/show_bug.cgi?id=25108
+
+        If you remove a class with a transition while that transition is running
+        the animation timer will continue to fire after the transition is finished.
+        This has no visual indication, but it does drain the processor. And in some
+        cases it might even cause a glitch in future animations. Unfortunately there
+        is no way to test this without putting in printfs.
+
+        This happens because the animation logic is never traversed after a transition
+        is removed, so we never get a chance to cleanup. So I added cleanup in the logic
+        that fires the dispatch of the last style change when the animation finishes.
+
+        Test: transitions/remove-transition-style.html
+
+        * page/animation/AnimationController.cpp:
+        (WebCore::AnimationControllerPrivate::updateStyleIfNeededDispatcherFired):
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimation::updateTransitions):
+        (WebCore::CompositeAnimation::animate):
+        (WebCore::CompositeAnimation::cleanupFinishedAnimations):
+        * page/animation/CompositeAnimation.h:
+
 2009-04-10  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Geoffrey Garen.
index b1fae38..d7e3ab7 100644 (file)
@@ -89,7 +89,7 @@ void AnimationControllerPrivate::updateAnimationTimer(bool callSetChanged/* = fa
 
     RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
     for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
-        RefPtr<CompositeAnimation> compAnim = it->second;
+        CompositeAnimation* compAnim = it->second.get();
         if (!compAnim->isSuspended() && compAnim->hasAnimations()) {
             double t = compAnim->timeToNextService();
             if (t != -1 && (t < needsService || needsService == -1))
@@ -152,6 +152,20 @@ void AnimationControllerPrivate::updateStyleIfNeededDispatcherFired(Timer<Animat
     
     if (m_frame)
         m_frame->document()->updateStyleIfNeeded();
+
+    // We can now safely remove any animations or transitions that are finished.
+    // We can't remove them any earlier because we might get a false restart of
+    // a transition. This can happen because we have not yet set the final property
+    // value until we call the rendering dispatcher. So this can make the current
+    // style slightly different from the desired final style (because our last 
+    // animation step was, say 0.9999 or something). And we need to remove them
+    // here because if there are no more animations running we'll never get back
+    // into the animation code to clean them up.
+    RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
+    for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
+        CompositeAnimation* compAnim = it->second.get();
+        compAnim->cleanupFinishedAnimations(); // will not modify m_compositeAnimations, so OK to call while iterating
+    }
 }
 
 void AnimationControllerPrivate::startupdateStyleIfNeededDispatcher()
@@ -209,9 +223,10 @@ void AnimationControllerPrivate::suspendAnimations(Document* document)
     RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
     for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
         RenderObject* renderer = it->first;
-        RefPtr<CompositeAnimation> compAnim = it->second;
-        if (renderer->document() == document)
+        if (renderer->document() == document) {
+            CompositeAnimation* compAnim = it->second.get();
             compAnim->suspendAnimations();
+        }
     }
     
     updateAnimationTimer();
@@ -224,9 +239,10 @@ void AnimationControllerPrivate::resumeAnimations(Document* document)
     RenderObjectAnimationMap::const_iterator animationsEnd = m_compositeAnimations.end();
     for (RenderObjectAnimationMap::const_iterator it = m_compositeAnimations.begin(); it != animationsEnd; ++it) {
         RenderObject* renderer = it->first;
-        RefPtr<CompositeAnimation> compAnim = it->second;
-        if (renderer->document() == document)
+        if (renderer->document() == document) {
+            CompositeAnimation* compAnim = it->second.get();
             compAnim->resumeAnimations();
+        }
     }
     
     updateAnimationTimer();
index ffefaae..d60455a 100644 (file)
@@ -110,7 +110,11 @@ void CompositeAnimation::updateTransitions(RenderObject* renderer, RenderStyle*
             bool equal = true;
 
             if (implAnim) {
-                // This implAnim might not be an already running transition. It might be
+                // This might be a transition that is just finishing. That would be the case
+                // if it were postActive. But we still need to check for equality because
+                // it could be just finishing AND changing to a new goal state.
+                //
+                // This implAnim might also not be an already running transition. It might be
                 // newly added to the list in a previous iteration. This would happen if
                 // you have both an explicit transition-property and 'all' in the same
                 // list. In this case, the latter one overrides the earlier one, so we
@@ -245,7 +249,7 @@ PassRefPtr<RenderStyle> CompositeAnimation::animate(RenderObject* renderer, Rend
             keyframeAnim->animate(this, renderer, currentStyle, targetStyle, resultStyle);
     }
 
-    cleanupFinishedAnimations(renderer);
+    cleanupFinishedAnimations();
 
     return resultStyle ? resultStyle.release() : targetStyle;
 }
@@ -337,7 +341,7 @@ PassRefPtr<KeyframeAnimation> CompositeAnimation::getAnimationForProperty(int pr
     return retval;
 }
 
-void CompositeAnimation::cleanupFinishedAnimations(RenderObject*)
+void CompositeAnimation::cleanupFinishedAnimations()
 {
     if (isSuspended())
         return;
index 19f9fa2..739cfdc 100644 (file)
@@ -74,7 +74,7 @@ public:
     
     PassRefPtr<KeyframeAnimation> getAnimationForProperty(int property) const;
 
-    void cleanupFinishedAnimations(RenderObject*);
+    void cleanupFinishedAnimations();
 
     void overrideImplicitAnimations(int property);
     void resumeOverriddenImplicitAnimations(int property);