Fixed https://bugs.webkit.org/show_bug.cgi?id=23088.
authorcmarrin@apple.com <cmarrin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Jan 2009 18:27:18 +0000 (18:27 +0000)
committercmarrin@apple.com <cmarrin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Jan 2009 18:27:18 +0000 (18:27 +0000)
        This was happening because I was calling setChanged() from inside updateRendering()
        which causes an infinite loop. I fixed this by deferring the setChanged to the next
        run loop iteration. That made it not infinite loop, but it still retriggers the
        transition forever. The problem is that there is both an 'all' and specific transition
        on 'opacity'. This tickled a bug in AnimationController which causes the opacity
        transition to get constantly cancelled and then retriggered. The problem is that
        the specific opacity transition has a duration of 0. I got rid of the logic to
        flush out 0 duration transitions and it is no longer constantly triggered. The
        logic to flush them was just an optimization, and you really need to keep them
        around to make the logic to override earlier animations by later ones work. And there is
        very little overhead in this case anyway, so the optimization was not that useful.

        I made a LayoutTest from the original testcase which tests both the infinite
        loop and constantly triggering animation cases.

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

LayoutTests/ChangeLog
LayoutTests/transforms/2d/cssmatrix-interface-expected.txt
LayoutTests/transitions/hang-with-bad-transition-list-expected.txt [new file with mode: 0644]
LayoutTests/transitions/hang-with-bad-transition-list.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/page/animation/AnimationBase.cpp
WebCore/page/animation/AnimationController.cpp
WebCore/page/animation/AnimationController.h
WebCore/page/animation/CompositeAnimation.cpp
WebCore/rendering/style/RenderStyle.cpp

index f9ed7d3..4f2f1b0 100644 (file)
@@ -1,3 +1,12 @@
+2009-01-16  Chris Marrin  <cmarrin@apple.com>
+
+        Reviewed by David Hyatt.
+
+        * transitions/hang-with-bad-transition-list-expected.txt: Added.
+        * transitions/hang-with-bad-transition-list.html: Added.
+
+        https://bugs.webkit.org/show_bug.cgi?id=23088
+
 2009-01-16  Chris Fleizach  <cfleizach@apple.com>
 
         Reviewed by Darin Adler.
index 2d0b6f4..8071346 100644 (file)
@@ -84,10 +84,10 @@ PASS m.e is 0
 PASS m.f is 0
 
 Test rotate
-PASS m.a is 0.9848077893257141
-PASS m.b is 0.1736481785774231
-PASS m.c is -0.1736481785774231
-PASS m.d is 0.9848077893257141
+PASS Math.round(m.a*1000000)/1000000 is 0.984808
+PASS Math.round(m.b*1000000)/1000000 is 0.173648
+PASS Math.round(m.c*1000000)/1000000 is -0.173648
+PASS Math.round(m.d*1000000)/1000000 is 0.984808
 PASS m.e is 0
 PASS m.f is 0
 
diff --git a/LayoutTests/transitions/hang-with-bad-transition-list-expected.txt b/LayoutTests/transitions/hang-with-bad-transition-list-expected.txt
new file mode 100644 (file)
index 0000000..7830aa6
--- /dev/null
@@ -0,0 +1,2 @@
+If you can see this then we didn't hang!!!
+Number of active animations before transition is (0) as expected
diff --git a/LayoutTests/transitions/hang-with-bad-transition-list.html b/LayoutTests/transitions/hang-with-bad-transition-list.html
new file mode 100644 (file)
index 0000000..431258a
--- /dev/null
@@ -0,0 +1,41 @@
+<html>
+<head>
+  <title>Testing hang when running a bad transition</title>
+  <style type="text/css" media="screen">
+    span {
+        background: blue;
+        color: white;
+        opacity: .666;
+        -webkit-transition: opacity, .25s, .15s ease-out;
+    }
+  </style>
+  <script type="text/javascript" charset="utf-8">
+    function checkRunning()
+    {
+      var current = layoutTestController.numberOfActiveAnimations();
+      if (current == 0)
+        document.getElementById('result').innerHTML = "Number of active animations before transition is (0) as expected";
+      else
+        document.getElementById('result').innerHTML = "Number of active transitions is (" + current + ") but was expecting (0)";
+      layoutTestController.notifyDone();
+    }
+    
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText();
+        layoutTestController.waitUntilDone();
+        window.setTimeout(checkRunning, 300);            
+    }
+  </script>
+</head>
+<body>
+
+<span id="doomSpan">If you can see this then we didn't hang!!!</span>
+
+<script type="text/javascript" charset="utf-8">
+    document.getElementById("doomSpan").style.opacity=1;
+</script>
+
+<div id="result">
+</div>
+</body>
+</html>
index 2e79316..875b2d1 100644 (file)
@@ -1,3 +1,38 @@
+2009-01-16  Chris Marrin  <cmarrin@apple.com>
+
+        Reviewed by David Hyatt.
+
+        Test: transitions/hang-with-bad-transition-list.html
+
+        Fixed https://bugs.webkit.org/show_bug.cgi?id=23088.
+        This was happening because I was calling setChanged() from inside updateRendering()
+        which causes an infinite loop. I fixed this by deferring the setChanged to the next
+        run loop iteration. That made it not infinite loop, but it still retriggers the
+        transition forever. The problem is that there is both an 'all' and specific transition
+        on 'opacity'. This tickled a bug in AnimationController which causes the opacity 
+        transition to get constantly cancelled and then retriggered. The problem is that
+        the specific opacity transition has a duration of 0. I got rid of the logic to 
+        flush out 0 duration transitions and it is no longer constantly triggered. The
+        logic to flush them was just an optimization, and you really need to keep them
+        around to make the logic to override earlier animations by later ones work. And there is
+        very little overhead in this case anyway, so the optimization was not that useful.
+
+        I made a LayoutTest from the original testcase which tests both the infinite
+        loop and constantly triggering animation cases.
+
+        * page/animation/AnimationBase.cpp:
+        (WebCore::AnimationBase::updateStateMachine):
+        * page/animation/AnimationController.cpp:
+        (WebCore::AnimationControllerPrivate::updateRenderingDispatcherFired):
+        (WebCore::AnimationControllerPrivate::addNodeChangeToDispatch):
+        (WebCore::AnimationController::addNodeChangeToDispatch):
+        * page/animation/AnimationController.h:
+        * page/animation/CompositeAnimation.cpp:
+        (WebCore::CompositeAnimationPrivate::updateTransitions):
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::adjustAnimations):
+        (WebCore::RenderStyle::adjustTransitions):
+
 2009-01-16  Chris Fleizach  <cfleizach@apple.com>
 
         Reviewed by Darin Adler.
index 850d942..b7ae4e1 100644 (file)
@@ -565,8 +565,8 @@ void AnimationBase::updateStateMachine(AnimStateInput input, double param)
                 m_compAnim->setWaitingForStyleAvailable(true);
 
                 // Trigger a render so we can start the animation
-                setChanged(m_object->element());
-                m_object->animation()->startUpdateRenderingDispatcher();
+                if (m_object)
+                    m_object->animation()->addNodeChangeToDispatch(m_object->element());
             } else {
                 ASSERT(!paused());
                 // We're waiting for the start timer to fire and we got a pause. Cancel the timer, pause and wait
@@ -607,11 +607,9 @@ void AnimationBase::updateStateMachine(AnimStateInput input, double param)
                 // Decide whether to go into looping or ending state
                 goIntoEndingOrLoopingState();
 
-                // Trigger a render so we can start the animation
-                if (m_object) {
-                    setChanged(m_object->element());
-                    m_compAnim->animationController()->startUpdateRenderingDispatcher();
-                }
+                // Dispatch updateRendering so we can start the animation
+                if (m_object)
+                    m_object->animation()->addNodeChangeToDispatch(m_object->element());
             } else {
                 // We are pausing while waiting for a start response. Cancel the animation and wait. When 
                 // we unpause, we will act as though the start timer just fired
@@ -651,8 +649,7 @@ void AnimationBase::updateStateMachine(AnimStateInput input, double param)
                     resumeOverriddenAnimations();
 
                     // Fire off another style change so we can set the final value
-                    setChanged(m_object->element());
-                    m_object->animation()->startUpdateRenderingDispatcher();
+                    m_object->animation()->addNodeChangeToDispatch(m_object->element());
                 }
             } else {
                 // We are pausing while running. Cancel the animation and wait
index b269e09..a710a70 100644 (file)
@@ -54,6 +54,7 @@ public:
     void updateRenderingDispatcherFired(Timer<AnimationControllerPrivate>*);
     void startUpdateRenderingDispatcher();
     void addEventToDispatch(PassRefPtr<Element> element, const AtomicString& eventType, const String& name, double elapsedTime);
+    void addNodeChangeToDispatch(PassRefPtr<Node>);
 
     bool hasAnimations() const { return !m_compositeAnimations.isEmpty(); }
 
@@ -94,6 +95,7 @@ private:
     };
     
     Vector<EventToDispatch> m_eventsToDispatch;
+    Vector<RefPtr<Node> > m_nodeChangesToDispatch;
     
     double m_beginAnimationUpdateTime;
 };
@@ -199,8 +201,8 @@ void AnimationControllerPrivate::updateAnimationTimer(bool callSetChanged/* = fa
 void AnimationControllerPrivate::updateRenderingDispatcherFired(Timer<AnimationControllerPrivate>*)
 {
     // fire all the events
-    Vector<EventToDispatch>::const_iterator end = m_eventsToDispatch.end();
-    for (Vector<EventToDispatch>::const_iterator it = m_eventsToDispatch.begin(); it != end; ++it) {
+    Vector<EventToDispatch>::const_iterator eventsToDispatchEnd = m_eventsToDispatch.end();
+    for (Vector<EventToDispatch>::const_iterator it = m_eventsToDispatch.begin(); it != eventsToDispatchEnd; ++it) {
         if (it->eventType == eventNames().webkitTransitionEndEvent)
             it->element->dispatchWebKitTransitionEvent(it->eventType,it->name, it->elapsedTime);
         else
@@ -209,6 +211,13 @@ void AnimationControllerPrivate::updateRenderingDispatcherFired(Timer<AnimationC
     
     m_eventsToDispatch.clear();
     
+    // call setChanged on all the elements
+    Vector<RefPtr<Node> >::const_iterator nodeChangesToDispatchEnd = m_nodeChangesToDispatch.end();
+    for (Vector<RefPtr<Node> >::const_iterator it = m_nodeChangesToDispatch.begin(); it != nodeChangesToDispatchEnd; ++it)
+        (*it)->setChanged(AnimationStyleChange);
+    
+    m_nodeChangesToDispatch.clear();
+    
     if (m_frame && m_frame->document())
         m_frame->document()->updateRendering();
 }
@@ -231,6 +240,12 @@ void AnimationControllerPrivate::addEventToDispatch(PassRefPtr<Element> element,
     startUpdateRenderingDispatcher();
 }
 
+void AnimationControllerPrivate::addNodeChangeToDispatch(PassRefPtr<Node> node)
+{
+    m_nodeChangesToDispatch.append(node);
+    startUpdateRenderingDispatcher();
+}
+
 void AnimationControllerPrivate::animationTimerFired(Timer<AnimationControllerPrivate>*)
 {
     // Make sure animationUpdateTime is updated, so that it is current even if no
@@ -431,6 +446,13 @@ void AnimationController::addEventToDispatch(PassRefPtr<Element> element, const
     m_data->addEventToDispatch(element, eventType, name, elapsedTime);
 }
 
+void AnimationController::addNodeChangeToDispatch(PassRefPtr<Node> node)
+{
+    ASSERT(!node || (node->document() && !node->document()->inPageCache()));
+    if (node)
+        m_data->addNodeChangeToDispatch(node);
+}
+
 void AnimationController::styleAvailable()
 {
     if (!m_numStyleAvailableWaiters)
index 45d0c7d..61536f9 100644 (file)
@@ -65,6 +65,7 @@ public:
 
     void startUpdateRenderingDispatcher();
     void addEventToDispatch(PassRefPtr<Element>, const AtomicString& eventType, const String& name, double elapsedTime);
+    void addNodeChangeToDispatch(PassRefPtr<Node>);
 
     void styleAvailable();
 
index 57cb774..c7bfea9 100644 (file)
@@ -166,14 +166,12 @@ void CompositeAnimationPrivate::updateTransitions(RenderObject* renderer, Render
             bool equal = true;
 
             if (implAnim) {
-               // This implAnim might 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
-               // behave as though this is a running animation being replaced.
-                if (!isActiveTransition)
-                    m_transitions.remove(prop);
-                else if (!implAnim->isTargetPropertyEqual(prop, targetStyle)) {
+                // This implAnim might 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
+                // behave as though this is a running animation being replaced.
+                if (!implAnim->isTargetPropertyEqual(prop, targetStyle)) {
                     m_transitions.remove(prop);
                     equal = false;
                 }
index cc2b670..ce4c340 100644 (file)
@@ -751,7 +751,7 @@ void RenderStyle::adjustAnimations()
     if (!animationList)
         return;
 
-    // get rid of empty transitions and anything beyond them
+    // Get rid of empty animations and anything beyond them
     for (size_t i = 0; i < animationList->size(); ++i) {
         if (animationList->animation(i)->isEmpty()) {
             animationList->resize(i);
@@ -774,7 +774,7 @@ void RenderStyle::adjustTransitions()
     if (!transitionList)
         return;
 
-    // get rid of empty transitions and anything beyond them
+    // Get rid of empty transitions and anything beyond them
     for (size_t i = 0; i < transitionList->size(); ++i) {
         if (transitionList->animation(i)->isEmpty()) {
             transitionList->resize(i);