cc: Split animating and drawing into separate actions

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 072e8e7efbf2d6ae30186f8514e2d47b790961b8..8a262262048c04d308e0a394bb36f0aa77f5cb23 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -461,14 +461,12 @@ void LayerTreeHostImpl::StartPageScaleAnimation(
                                   duration.InSecondsF());
   }
 
-  SetNeedsRedraw();
+  SetNeedsAnimate();
   client_->SetNeedsCommitOnImplThread();
   client_->RenewTreePriority();
 }
 
-void LayerTreeHostImpl::ScheduleAnimation() {
-  SetNeedsRedraw();
-}
+void LayerTreeHostImpl::ScheduleAnimation() { SetNeedsAnimate(); }

[brianderson, 2014/04/01 16:05:11]

Can we get rid of ScheduleAnimation?

[Sami, 2014/04/17 15:16:33]

Looks like we can, done.

 
 bool LayerTreeHostImpl::HaveTouchEventHandlersAt(
     const gfx::Point& viewport_point) {
@@ -1679,6 +1677,10 @@ void LayerTreeHostImpl::SetVisible(bool visible) {
   renderer_->SetVisible(visible);
 }
 
+void LayerTreeHostImpl::SetNeedsAnimate() {
+  client_->SetNeedsAnimateOnImplThread();
+}
+
 void LayerTreeHostImpl::SetNeedsRedraw() {
   NotifySwapPromiseMonitorsOfSetNeedsRedraw();

[brianderson, 2014/04/01 16:05:11]

What does NotifySwapPromiseMonitorsOfSetNeedsRedraw do? Do we need it in
SetNeedsAnimate too?

[Sami, 2014/04/17 15:16:33]

As far as I understand it, that call tells the latency tracking system that we
are about to draw and swap soon. A call to SetNeedsAnimate() by itself isn't a
strong enough guarantee of that, because the animation may end up being a no-op,
so I don't think we need the call there.

+miletus@, could you verify my assumptions?

[danakj, 2014/04/17 15:18:09]

It is meant to tell the monitor that we might draw and swap and cause it to
insert a LatencyInfo. It can have false positives. The LatencyInfo should later
be aborted if we don't end up drawing as a result.

   client_->SetNeedsRedrawOnImplThread();
@@ -2661,6 +2663,8 @@ void LayerTreeHostImpl::AnimatePageScale(base::TimeTicks monotonic_time) {
     page_scale_animation_.reset();
     client_->SetNeedsCommitOnImplThread();
     client_->RenewTreePriority();
+  } else {
+    SetNeedsAnimate();

[brianderson, 2014/04/01 16:05:11]

Can you get rid of the SetNeedsRedraw above?

In the current patch, SetNeedsAnimate eventually implies SetNeedsRedraw in the
scheduler. In the future, it seems like you want the animation system to trigger
a redraw if something has changed. In both cases, I don't think the
SetNeedsRedraw is needed.

[Sami, 2014/04/17 15:16:33]

I think we still need the SetNeedsRedraw here because AnimatePageScale is the
"animation system" for page scale animations, and really only it knows when a
redraw is needed. I guess an alternative would be to move more of this logic to
input/page_scale_animation.cc, or better yet introduce a new
PageScaleAnimationManager.

You're right that it isn't required in the sense that things still work, but
I've made some of the animation unit tests to enforce the separation between
SetNeedsAnimate and SetNeedsRedraw.

   }
 }
 
@@ -2671,9 +2675,9 @@ void LayerTreeHostImpl::AnimateTopControls(base::TimeTicks time) {
   if (active_tree_->TotalScrollOffset().y() == 0.f)
     return;
   if (scroll.IsZero()) {
-    // This may happen on the first animation step. Force redraw otherwise
+    // This may happen on the first animation step. Force animate otherwise
     // the animation would stop because of no new frames.
-    SetNeedsRedraw();
+    SetNeedsAnimate();

[brianderson, 2014/04/01 16:05:11]

I guess this is the way it already is, but I wonder why this SetNeedsAnimate
isn't needed for the else block?

[Sami, 2014/04/17 15:16:33]

Good question. I think we should call SetNeedsAnimate in both cases here. It
looks the way this worked is that top_controls_manager_->Animate() ends up
calling DidChangeTopControlsPosition, which in turn calls SetNeedsRedraw. Now
that we can schedule animation without relying on that callback I think we can
just remove this special case.

   } else {
     ScrollViewportBy(gfx::ScaleVector2d(
         scroll, 1.f / active_tree_->total_page_scale_factor()));
@@ -2699,7 +2703,7 @@ void LayerTreeHostImpl::AnimateLayers(base::TimeTicks monotonic_time) {
        ++iter)
     (*iter).second->Animate(monotonic_time_for_cc_animations);
 
-  SetNeedsRedraw();
+  SetNeedsAnimate();
 }
 
 void LayerTreeHostImpl::UpdateAnimationState(bool start_ready_animations) {
@@ -2809,9 +2813,10 @@ void LayerTreeHostImpl::AnimateScrollbarsRecursive(LayerImpl* layer,
       layer->scrollbar_animation_controller();
   if (scrollbar_controller && scrollbar_controller->Animate(time)) {
     TRACE_EVENT_INSTANT0(
-        "cc", "LayerTreeHostImpl::SetNeedsRedraw due to AnimateScrollbars",
+        "cc",
+        "LayerTreeHostImpl::SetNeedsAnimate due to AnimateScrollbars",
         TRACE_EVENT_SCOPE_THREAD);
-    SetNeedsRedraw();
+    SetNeedsAnimate();
   }
 
   for (size_t i = 0; i < layer->children().size(); ++i)
@@ -2832,10 +2837,11 @@ void LayerTreeHostImpl::StartScrollbarAnimationRecursive(LayerImpl* layer,
       layer->scrollbar_animation_controller();
   if (scrollbar_controller && scrollbar_controller->IsAnimating()) {
     base::TimeDelta delay = scrollbar_controller->DelayBeforeStart(time);
-    if (delay > base::TimeDelta())
+    if (delay > base::TimeDelta()) {
       client_->RequestScrollbarAnimationOnImplThread(delay);
-    else if (scrollbar_controller->Animate(time))
-      SetNeedsRedraw();
+    } else if (scrollbar_controller->Animate(time)) {
+      SetNeedsAnimate();
+    }
   }
 
   for (size_t i = 0; i < layer->children().size(); ++i)