Unifies LayerTreeHost::SetNeedsUpdateLayers and SetNeedsAnimate

cc/trees/thread_proxy.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/trees/thread_proxy.h"
 
 #include <string>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
@@ skipping 62 matching lines @@
     LayerTreeHost* layer_tree_host,
     scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner) {
   return make_scoped_ptr(
       new ThreadProxy(layer_tree_host, impl_task_runner)).PassAs<Proxy>();
 }
 
 ThreadProxy::ThreadProxy(
     LayerTreeHost* layer_tree_host,
     scoped_refptr<base::SingleThreadTaskRunner> impl_task_runner)
     : Proxy(impl_task_runner),
-      animate_requested_(false),
+      update_requested_(false),
       commit_requested_(false),
       commit_request_sent_to_impl_thread_(false),
       created_offscreen_context_provider_(false),
       layer_tree_host_unsafe_(layer_tree_host),
       contents_texture_manager_unsafe_(NULL),
       started_(false),
       textures_acquired_(true),
       in_composite_and_readback_(false),
       manage_tiles_pending_(false),
       commit_waits_for_activation_(false),
@@ skipping 249 matching lines @@
       base::Bind(&ThreadProxy::SetNeedsCommitOnImplThread,
                  impl_thread_weak_ptr_));
 }
 
 const RendererCapabilities& ThreadProxy::GetRendererCapabilities() const {
   DCHECK(IsMainThread());
   DCHECK(!layer_tree_host()->output_surface_lost());
   return renderer_capabilities_main_thread_copy_;
 }
 
-void ThreadProxy::SetNeedsAnimate() {
-  DCHECK(IsMainThread());
-  if (animate_requested_)
-    return;
-
-  TRACE_EVENT0("cc", "ThreadProxy::SetNeedsAnimate");
-  animate_requested_ = true;
-  can_cancel_commit_ = false;
-  SendCommitRequestToImplThreadIfNeeded();
-}
-
 void ThreadProxy::SetNeedsUpdateLayers() {
   DCHECK(IsMainThread());
 
-  if (commit_request_sent_to_impl_thread_)
+  if (update_requested_)
     return;
   TRACE_EVENT0("cc", "ThreadProxy::SetNeedsUpdateLayers");
 
+  update_requested_ = true;
   SendCommitRequestToImplThreadIfNeeded();
 }
 
 void ThreadProxy::SetNeedsCommit() {
   DCHECK(IsMainThread());
   // Unconditionally set here to handle SetNeedsCommit calls during a commit.
   can_cancel_commit_ = false;
 
   if (commit_requested_)
     return;
@@ skipping 398 matching lines @@
   SwapPromiseChecker swap_promise_checker(layer_tree_host());
 
   // Do not notify the impl thread of commit requests that occur during
   // the apply/animate/layout part of the BeginMainFrameAndCommit process since
   // those commit requests will get painted immediately. Once we have done
   // the paint, commit_requested_ will be set to false to allow new commit
   // requests to be scheduled.
   commit_requested_ = true;
   commit_request_sent_to_impl_thread_ = true;
 
-  // On the other hand, the AnimationRequested flag needs to be cleared
-  // here so that any animation requests generated by the apply or animate
+  // On the other hand, the update_requested_ flag needs to be cleared
+  // here so that any update requests generated by the apply or animate
   // callbacks will trigger another frame.
-  animate_requested_ = false;
+  update_requested_ = false;
 
   if (!in_composite_and_readback_ && !layer_tree_host()->visible()) {
     commit_requested_ = false;
     commit_request_sent_to_impl_thread_ = false;
 
     TRACE_EVENT0("cc", "EarlyOut_NotVisible");
     bool did_handle = false;
     Proxy::ImplThreadTaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
@@ skipping 55 matching lines @@
       make_scoped_ptr(new ResourceUpdateQueue);
 
   bool updated = layer_tree_host()->UpdateLayers(queue.get());
 
   // Once single buffered layers are committed, they cannot be modified until
   // they are drawn by the impl thread.
   textures_acquired_ = false;
 
   layer_tree_host()->WillCommit();
 
+  // Before calling animate, we set update_requested_ to false. If it is true
+  // now, it means SetNeedsUpdateLayers was called again, but during a state
+  // when commit_request_sent_to_impl_thread_ = true. We need to force that call

[danakj, 2013/12/05 18:28:16]

Can you explain this? The SetNeedsUpdateLayers() function only checks for
update_requested_ in its early out. So would this not have been called and
post-tasked already if update_requested_ is true here?

This looks to me like old code that is not needed any more. Do any tests fail
without it?

+ajuma as well

[ajuma, 2013/12/05 19:01:06]

SetNeedsUpdateLayers calls SendCommitRequestToImplThreadIfNeeded, which
earlies-out if commit_request_sent_to_impl_thread_ is true. There is a
gap between when update_requested_ is cleared and when
commit_request_sent_to_impl_thread_ is cleared, so we need to make sure
that calls to SetNeedsUpdateLayers during this gap (e.g. those that
occur during UpdateClientAnimations) do eventually cause a commit request.
So I think we either need to close that gap by moving up the clearing of
commit_request_sent_to_impl_thread_ (if we can), or we need to keep this
check.

[danakj, 2013/12/05 19:18:51]

Ah, thanks. I see. SendCommitRequestToImplThreadIfNeeded() will early out and
that is still on the main thread.

I think this comment isn't entirely true though. We could have called
SetNeedsUpdate() inside UpdateLayers() or inside WillCommit(), both of which are
called when commit_request_sent_to_impl_thread_ is false.

I guess if the comment is wrong, the the SetNeedsUpdateLayers() call here will
not do anything because commit_request_sent_to_impl_thread_ will be true again.
It might make sense to move this all the way up to line 846, just below where we
actually set commit_request_sent_to_impl_thread = false.

[trchen, 2013/12/06 03:42:39]

It is doing the right thing. The comment is a bit misleading though.

The purpose of it is to make sure a commit will be scheduled again when the 
update request happened during a commit. Depending on during which phase of
the commit an update is requested, commit_request_sent_to_impl_thread_ may be
true or false. Either way we clear the early-out flag here and request update
again to make sure a commit is always scheduled.

Whether commit_request_sent_to_impl_thread_ is false here is irrelevant. If it
is true that means something in UpdateLayers or WillCommit scheduled a commit,
and we can get a free ride here. If it's false, then we schedule a commit
ourselves.

I think we can both revise the comment and the code:
if (update_requested_)
  SetNeedsCommit();

This should be much less confusing.

[danakj, 2013/12/06 17:21:33]

Ah, that will do something different though, as SNC() causes an un-abortable
commit, but we want to cause an abortable one, so either SetNeedsUpdate or
SendCommitRequestToImpl should be happening here. I guess the comment needs a
bit of love but we're good otherwise.

+  // to happen again now so that the commit request is sent to the impl thread.
+  if (update_requested_) {
+    // Forces SetNeedsUpdateLayers to consider posting a commit task.
+    update_requested_ = false;
+    SetNeedsUpdateLayers();
+  }
+
   if (!updated && can_cancel_this_commit) {
     TRACE_EVENT0("cc", "EarlyOut_NoUpdates");
     bool did_handle = true;
     Proxy::ImplThreadTaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
                    impl_thread_weak_ptr_,
                    did_handle));
 
     // Although the commit is internally aborted, this is because it has been
     // detected to be a no-op.  From the perspective of an embedder, this commit
     // went through, and input should no longer be throttled, etc.
     layer_tree_host()->CommitComplete();
     layer_tree_host()->DidBeginMainFrame();
     return;
   }
 
-  // Before calling animate, we set animate_requested_ to false. If it is true
-  // now, it means SetNeedAnimate was called again, but during a state when
-  // commit_request_sent_to_impl_thread_ = true. We need to force that call to
-  // happen again now so that the commit request is sent to the impl thread.
-  if (animate_requested_) {
-    // Forces SetNeedsAnimate to consider posting a commit task.
-    animate_requested_ = false;
-    SetNeedsAnimate();
-  }
-
   scoped_refptr<cc::ContextProvider> offscreen_context_provider;
   if (renderer_capabilities_main_thread_copy_.using_offscreen_context3d &&
       layer_tree_host()->needs_offscreen_context()) {
     offscreen_context_provider =
         layer_tree_host()->client()->OffscreenContextProvider();
     if (offscreen_context_provider.get())
       created_offscreen_context_provider_ = true;
   }
 
   // Notify the impl thread that the main thread is ready to commit. This will
@@ skipping 746 matching lines @@
   commit_to_activate_duration_history_.InsertSample(
       base::TimeTicks::HighResNow() - commit_complete_time_);
 }
 
 void ThreadProxy::DidManageTiles() {
   DCHECK(IsImplThread());
   scheduler_on_impl_thread_->DidManageTiles();
 }
 
 }  // namespace cc