Bug 1181392 part 9 - Remove use of IsFinishedTransition from nsTransitionManager::FlushTransitions; r=dbaron
authorBrian Birtles <birtles@gmail.com>
Fri, 07 Aug 2015 12:29:36 +0900
changeset 288405 76626b00fac91ccc669e704da276498e61695178
parent 288404 c105cadd52e2ba00b5b87718a59c7da7eeb459df
child 288406 9060b380602b0580be686f64176dfcf4732d71cf
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1181392 part 9 - Remove use of IsFinishedTransition from nsTransitionManager::FlushTransitions; r=dbaron This patch updates the logic in nsTransitionManager::FlushTransitions that deals with detecting newly-started (i.e. when the delay phase has just finished) or newly-finished animations. The existing logic had the following behavior: * Calling SetIsFinishedTransition for newly-finished transitions. This is no longer needed since by this point all consumers of IsFinishedTransition have been updated to use alternative means for testing if an animation is relevant. * Setting transitionStartedOrEnded to true so that we would post an animation restyle. We can achieve this same effect by simplying updating the canThrottleTick flag using the result of calling Animation::CanThrottle on each animation. Animation::CanThrottle will return false for a newly-started or newly-finished animation (it returns false for a newly-started animation since the animation's mIsRunningOnCompositor flag won't yet be true at that point.) * Updating the animation generation for newly-started and newly-finished animations in order to trigger a layer update. At least that appears to be the intention of this code. However, it currently has no effect since the animation generation on the pres context is not incremented in any context I could find where a newly started/finished animation might be detected. For this third case, this patch adds tests to ensure that transitions are correctly added and removed from the compositor when they start and finish. This is because most of the existing tests in test_animations_omta.html cover only CSS animations. As noted in the comments in the test, if a tick lands at the exact beginning of a transition we will typically not send it to the layer until the following tick because the RestyleManager will filter out the seemingly redundant change (i.e. no change to the computed style). Presumably updating the animation generation was intended to avoid this. This same behavior is true of CSS animations (and the same kind of comment appears elsewhere in test_animations_omta.html). Long-term this is probably worth fixing so that when a transition is triggered we get it to the compositor one tick earlier which should improve responsiveness when the main thread is busy and the interval between ticks is long. Furthermore, we should do the same for both all type of animations, not just transitions. Currently, however, this patch preserves the existing behavior and helps narrow the difference between transitions and animations so we can apply this optimization to both.
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -932,59 +932,34 @@ nsTransitionManager::FlushTransitions(Fl
           AnimationCollection::CanAnimateFlags(0)) &&
       MOZ_ASSERT(collection->mElement->GetCrossShadowCurrentDoc() ==
                  "Element::UnbindFromTree should have "
                  "destroyed the element transitions object");
-      size_t i = collection->mAnimations.Length();
-      MOZ_ASSERT(i != 0, "empty transitions list?");
-      bool transitionStartedOrEnded = false;
-      do {
-        --i;
-        Animation* anim = collection->mAnimations[i];
-        if (!anim->GetEffect()->IsFinishedTransition()) {
-          MOZ_ASSERT(anim->GetEffect(), "Transitions should have an effect");
-          ComputedTiming computedTiming =
-            anim->GetEffect()->GetComputedTiming();
-          if (computedTiming.mPhase == ComputedTiming::AnimationPhase_After) {
-            // Leave this transition in the list for one more refresh
-            // cycle, since we haven't yet processed its style change, and
-            // if we also have (already, or will have from processing
-            // transitionend events or other refresh driver notifications)
-            // a non-animation style change that would affect it, we need
-            // to know not to start a new transition for the transition
-            // from the almost-completed value to the final value.
-            anim->GetEffect()->SetIsFinishedTransition(true);
-            collection->UpdateAnimationGeneration(mPresContext);
-            transitionStartedOrEnded = true;
-          } else if ((computedTiming.mPhase ==
-                      ComputedTiming::AnimationPhase_Active) &&
-                     canThrottleTick &&
-                     !anim->IsRunningOnCompositor()) {
-            // Start a transition with a delay where we should start the
-            // transition proper.
-            collection->UpdateAnimationGeneration(mPresContext);
-            transitionStartedOrEnded = true;
-          }
-        }
-      } while (i != 0);
+      // Look for any transitions that can't be throttled because they
+      // may be starting or stopping
+      for (auto iter = collection->mAnimations.cbegin();
+           canThrottleTick && iter != collection->mAnimations.cend();
+           ++iter) {
+        canThrottleTick &= (*iter)->CanThrottle();
+      }
       // We need to restyle even if the transition rule no longer
       // applies (in which case we just made it not apply).
       MOZ_ASSERT(collection->mElementProperty ==
                    nsGkAtoms::transitionsProperty ||
                  collection->mElementProperty ==
                    nsGkAtoms::transitionsOfBeforeProperty ||
                  collection->mElementProperty ==
                  "Unexpected element property; might restyle too much");
-      if (!canThrottleTick || transitionStartedOrEnded) {
+      if (!canThrottleTick) {
       } else {
         didThrottle = true;
       if (collection->mAnimations.IsEmpty()) {
         // |collection| is now a dangling pointer!
--- a/layout/style/test/test_animations_omta.html
+++ b/layout/style/test/test_animations_omta.html
@@ -2340,10 +2340,43 @@ addAsyncAnimTest(function *() {
   omta_is("transform", { tx: 50 }, RunningOn.Compositor,
           "transformnone animation at 0ms");
   omta_is("transform", { tx: 0 }, RunningOn.Compositor,
           "transformnone animation at 500ms, interpolating none values");
+addAsyncAnimTest(function *() {
+  new_div("transform: translate(100px); transition: transform 10s 5s linear");
+  yield waitForPaintsFlushed();
+  gDiv.style.transform = "translate(200px)";
+  yield waitForPaintsFlushed();
+  omta_is("transform", { tx: 100 }, RunningOn.MainThread,
+          "transition runs on main thread during delay");
+  // At the *very* start of the transition the start value of the transition
+  // will match the underlying transform value. Various optimizations in
+  // RestyleManager may recognize this a "no change" and filter out the
+  // transition meaning that the animation doesn't get added to the compositor
+  // thread until the first time the value changes. As a result, we fast-forward
+  // a little past the beginning and then wait for the animation to be sent
+  // to the compositor.
+  advance_clock(5100);
+  yield waitForPaints();
+  omta_is("transform", { tx: 101 }, RunningOn.Compositor,
+          "transition runs on compositor at start of active interval");
+  advance_clock(4900);
+  omta_is("transform", { tx: 150 }, RunningOn.Compositor,
+          "transition runs on compositor at during active interval");
+  advance_clock(5000);
+  // Currently the compositor will apply a forwards fill until it gets told by
+  // the main thread to clear the animation. As a result we should wait for
+  // paints before checking that the animated value does *not* appear on the
+  // compositor thread.
+  yield waitForPaints();
+  omta_is("transform", { tx: 200 }, RunningOn.MainThread,
+          "transition runs on main thread at end of active interval");
+  done_div();