Remove early exit from LayerTreeHost::AnimateLayers for BGPT

Running LayerTreeHost::AnimateLayers from the main thread is necessary
in order for the main thread KeyframeModels to progress from
WAITING_FOR_TARGET_AVAILABILITY to STARTING. Failing to advance to this
state results in the cc KeyframeModels never being removed as the
animation is still considered current and needing to affect the
pending tree.

(cherry picked from commit 93fe205c2143050ee45027cb1a3e926905373067)

Bug: 762717,962346
Change-Id: Ia559c0a7e3c73e12cc883e64fb4adb284a924418
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1256422
Commit-Queue: Robert Flack <flackr@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#669075}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1660638
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#354}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}

diff --git a/cc/animation/animation_host.cc b/cc/animation/animation_host.cc
index add57dd..ccb6a42b 100644
--- a/cc/animation/animation_host.cc
+++ b/cc/animation/animation_host.cc

@@ -222,6 +222,8 @@
     return;
 
   mutator_host_client_ = client;
+  if (mutator_host_client_ && needs_push_properties_)
+    mutator_host_client_->SetMutatorsNeedCommit();
 }
 
 void AnimationHost::SetNeedsCommit() {
@@ -233,6 +235,8 @@
 
 void AnimationHost::SetNeedsPushProperties() {
   needs_push_properties_ = true;
+  if (mutator_host_client_)
+    mutator_host_client_->SetMutatorsNeedCommit();
 }
 
 void AnimationHost::PushPropertiesTo(MutatorHost* mutator_host_impl) {

diff --git a/cc/animation/animation_unittest.cc b/cc/animation/animation_unittest.cc
index a0067e8..9ae34304 100644
--- a/cc/animation/animation_unittest.cc
+++ b/cc/animation/animation_unittest.cc

@@ -464,7 +464,8 @@
   timeline_->AttachAnimation(animation_);
   animation_->AttachElementForKeyframeEffect(element_id_, keyframe_effect_id_);
 
-  EXPECT_FALSE(client_.mutators_need_commit());
+  EXPECT_TRUE(client_.mutators_need_commit());
+  client_.set_mutators_need_commit(false);
 
   const int keyframe_model_id = AddOpacityTransitionToAnimation(
       animation_.get(), 1., .7f, .3f, false, keyframe_effect_id_);

diff --git a/cc/animation/worklet_animation.cc b/cc/animation/worklet_animation.cc
index d50f2d3d..a6d109fa 100644
--- a/cc/animation/worklet_animation.cc
+++ b/cc/animation/worklet_animation.cc

@@ -92,8 +92,8 @@
 }
 
 void WorkletAnimation::Tick(base::TimeTicks monotonic_time) {
-  // Do not tick worklet animations on main thread. This should be removed if we
-  // skip ticking all animations on main thread in http://crbug.com/762717.
+  // Do not tick worklet animations on main thread as we will tick them on the
+  // compositor and the tick is more expensive than regular animations.
   if (!is_impl_instance_)
     return;
   if (!local_time_.has_value())

diff --git a/cc/layers/layer_unittest.cc b/cc/layers/layer_unittest.cc
index 80a1ff2f..0a02373e 100644
--- a/cc/layers/layer_unittest.cc
+++ b/cc/layers/layer_unittest.cc

@@ -1611,8 +1611,9 @@
   ElementId element_id = ElementId(2);
   test_layer->SetElementId(element_id);
 
-  // Expect additional call due to has-animation check.
-  EXPECT_CALL(*layer_tree_host_, SetNeedsCommit()).Times(2);
+  // Expect additional calls due to has-animation check and initialization
+  // of keyframes.
+  EXPECT_CALL(*layer_tree_host_, SetNeedsCommit()).Times(7);
   scoped_refptr<AnimationTimeline> timeline =
       AnimationTimeline::Create(AnimationIdProvider::NextTimelineId());
   animation_host_->AddAnimationTimeline(timeline);

diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
index 5fdc23f..4e9a939 100644
--- a/cc/trees/layer_tree_host.cc
+++ b/cc/trees/layer_tree_host.cc

@@ -619,6 +619,12 @@
     std::unique_ptr<MutatorEvents> events) {
   DCHECK(task_runner_provider_->IsMainThread());
   mutator_host_->SetAnimationEvents(std::move(events));
+
+  // Events are added to a queue to be dispatched but we need a main frame
+  // in order to dispatch the events. Also, finished animations require
+  // a commit in order to clean up their KeyframeModels but without a main
+  // frame we could indefinitely delay cleaning up the animation.
+  SetNeedsAnimate();
 }
 
 void LayerTreeHost::SetDebugState(
@@ -992,22 +998,21 @@
 }
 
 void LayerTreeHost::AnimateLayers(base::TimeTicks monotonic_time) {
-  // We do not need to animate on the main thread in this case because all
-  // relevant changes will be processed on the compositor thread, or proxy,
-  // and propagated back to the correct trees.
-  // TODO(majidvp): We should be able to eliminate this in the non-
-  // slimming path and will do so in a follow up. (762717)
-  if (IsUsingLayerLists())
-    return;
-
   std::unique_ptr<MutatorEvents> events = mutator_host_->CreateEvents();
 
   if (mutator_host_->TickAnimations(monotonic_time,
                                     property_trees()->scroll_tree, true))
     mutator_host_->UpdateAnimationState(true, events.get());
 
-  if (!events->IsEmpty())
-    property_trees_.needs_rebuild = true;
+  if (!events->IsEmpty()) {
+    // If not using layer lists, animation state changes will require
+    // rebuilding property trees to track them.
+    if (!IsUsingLayerLists())
+      property_trees_.needs_rebuild = true;
+
+    // A commit is required to push animation changes to the compositor.
+    SetNeedsCommit();
+  }
 }
 
 int LayerTreeHost::ScheduleMicroBenchmark(

diff --git a/cc/trees/layer_tree_host_unittest_animation.cc b/cc/trees/layer_tree_host_unittest_animation.cc
index 6a1ffbf..47a27fd 100644
--- a/cc/trees/layer_tree_host_unittest_animation.cc
+++ b/cc/trees/layer_tree_host_unittest_animation.cc

@@ -28,6 +28,7 @@
 #include "cc/test/layer_tree_test.h"
 #include "cc/trees/effect_node.h"
 #include "cc/trees/layer_tree_impl.h"
+#include "cc/trees/target_property.h"
 #include "cc/trees/transform_node.h"
 #include "components/viz/common/quads/compositor_frame.h"
 
@@ -1207,6 +1208,109 @@
 
 MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestScrollOffsetAnimationRemoval);
 
+// Verifies that the state of a scroll animation is tracked correctly on the
+// main and compositor thread and that the KeyframeModel is removed when
+// the scroll animation completes. This is a regression test for
+// https://crbug.com/962346 and demonstrates the necessity of
+// LayerTreeHost::AnimateLayers for https://crbug.com/762717.
+class LayerTreeHostAnimationTestScrollOffsetAnimationCompletion
+    : public LayerTreeHostAnimationTest {
+ public:
+  LayerTreeHostAnimationTestScrollOffsetAnimationCompletion()
+      : final_position_(80.0, 180.0) {}
+
+  void SetupTree() override {
+    LayerTreeHostAnimationTest::SetupTree();
+
+    scroll_layer_ = FakePictureLayer::Create(&client_);
+    scroll_layer_->SetScrollable(gfx::Size(100, 100));
+    scroll_layer_->SetBounds(gfx::Size(10000, 10000));
+    client_.set_bounds(scroll_layer_->bounds());
+    scroll_layer_->SetScrollOffset(gfx::ScrollOffset(100.0, 200.0));
+    layer_tree_host()->root_layer()->AddChild(scroll_layer_);
+
+    std::unique_ptr<ScrollOffsetAnimationCurve> curve(
+        ScrollOffsetAnimationCurve::Create(
+            final_position_,
+            CubicBezierTimingFunction::CreatePreset(
+                CubicBezierTimingFunction::EaseType::EASE_IN_OUT)));
+    std::unique_ptr<KeyframeModel> keyframe_model(KeyframeModel::Create(
+        std::move(curve), 1, 0, TargetProperty::SCROLL_OFFSET));
+    keyframe_model->set_needs_synchronized_start_time(true);
+
+    AttachAnimationsToTimeline();
+    animation_child_->AttachElement(scroll_layer_->element_id());
+    animation_child_->AddKeyframeModel(std::move(keyframe_model));
+  }
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void BeginMainFrame(const viz::BeginFrameArgs& args) override {
+    KeyframeModel* keyframe_model =
+        animation_child_->GetKeyframeModel(TargetProperty::SCROLL_OFFSET);
+    switch (layer_tree_host()->SourceFrameNumber()) {
+      case 0:
+        EXPECT_EQ(scroll_layer_->CurrentScrollOffset().x(), 100);
+        EXPECT_EQ(scroll_layer_->CurrentScrollOffset().y(), 200);
+        EXPECT_EQ(KeyframeModel::RunState::WAITING_FOR_TARGET_AVAILABILITY,
+                  keyframe_model->run_state());
+        break;
+      case 1:
+        EXPECT_EQ(KeyframeModel::RunState::RUNNING,
+                  keyframe_model->run_state());
+        break;
+      default:
+        break;
+    }
+  }
+
+  void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override {
+    if (host_impl->sync_tree()->source_frame_number() == 0) {
+      GetImplTimelineAndAnimationByID(*host_impl);
+      return;
+    }
+    KeyframeModel* keyframe_model =
+        animation_child_impl_->GetKeyframeModel(TargetProperty::SCROLL_OFFSET);
+    if (!keyframe_model || keyframe_model->run_state() ==
+                               KeyframeModel::RunState::WAITING_FOR_DELETION)
+      EndTest();
+  }
+
+  void DidFinishImplFrameOnThread(LayerTreeHostImpl* host_impl) override {
+    if (!animation_child_impl_)
+      return;
+    if (KeyframeModel* keyframe_model = animation_child_impl_->GetKeyframeModel(
+            TargetProperty::SCROLL_OFFSET)) {
+      if (keyframe_model->run_state() == KeyframeModel::RunState::RUNNING) {
+        ran_animation_ = true;
+      }
+    }
+  }
+
+  void AfterTest() override {
+    // The animation should have run for some frames.
+    EXPECT_TRUE(ran_animation_);
+
+    // The finished KeyframeModel should have been removed from both the
+    // main and impl side animations.
+    EXPECT_EQ(nullptr, animation_child_->GetKeyframeModel(
+                           TargetProperty::SCROLL_OFFSET));
+    EXPECT_EQ(nullptr, animation_child_impl_->GetKeyframeModel(
+                           TargetProperty::SCROLL_OFFSET));
+
+    // The scroll should have been completed.
+    EXPECT_EQ(final_position_, scroll_layer_->CurrentScrollOffset());
+  }
+
+ private:
+  FakeContentLayerClient client_;
+  scoped_refptr<FakePictureLayer> scroll_layer_;
+  const gfx::ScrollOffset final_position_;
+  bool ran_animation_ = false;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestScrollOffsetAnimationCompletion);
+
 // When animations are simultaneously added to an existing layer and to a new
 // layer, they should start at the same time, even when there's already a
 // running animation on the existing layer.
@@ -1699,6 +1803,8 @@
       case 2:
         KeyframeModel* keyframe_model =
             animation_->GetKeyframeModel(TargetProperty::TRANSFORM);
+        EXPECT_EQ(KeyframeModel::RunState::RUNNING,
+                  keyframe_model->run_state());
         animation_->RemoveKeyframeModel(keyframe_model->id());
         break;
     }
@@ -1717,6 +1823,36 @@
       case 2:
         // The animation is removed/stopped.
         EXPECT_FALSE(child->screen_space_transform_is_animating());
+        break;
+      case 3:
+        break;
+      default:
+        NOTREACHED();
+    }
+  }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    GetImplTimelineAndAnimationByID(*host_impl);
+    switch (host_impl->active_tree()->source_frame_number()) {
+      case 0:
+        // No animation yet.
+        break;
+      case 1:
+        // Animation is starting.
+        EXPECT_EQ(KeyframeModel::RunState::STARTING,
+                  animation_impl_->GetKeyframeModel(TargetProperty::TRANSFORM)
+                      ->run_state());
+        break;
+      case 2:
+        // After activation, the KeyframeModel should be waiting for deletion.
+        EXPECT_EQ(KeyframeModel::RunState::WAITING_FOR_DELETION,
+                  animation_impl_->GetKeyframeModel(TargetProperty::TRANSFORM)
+                      ->run_state());
+        break;
+      case 3:
+        // The animation KeyframeModel is cleaned up.
+        EXPECT_EQ(nullptr,
+                  animation_impl_->GetKeyframeModel(TargetProperty::TRANSFORM));
         EndTest();
         break;
       default:
@@ -1735,9 +1871,9 @@
         EXPECT_TRUE(child->screen_space_transform_is_animating());
         break;
       case 2:
+      case 3:
         // The animation is removed/stopped.
         EXPECT_FALSE(child->screen_space_transform_is_animating());
-        EndTest();
         break;
       default:
         NOTREACHED();
@@ -2321,12 +2457,9 @@
   }
 
   void UpdateLayerTreeHost() override {
-    if (layer_tree_host()->SourceFrameNumber() == 1) {
-      EXPECT_FALSE(layer_tree_host()->property_trees()->needs_rebuild);
+    if (layer_tree_host()->SourceFrameNumber() == 1)
       AddAnimatedTransformToAnimation(animation_child_.get(), 1.0, 5, 5);
-    }
-
-    EXPECT_TRUE(layer_tree_host()->property_trees()->needs_rebuild);
+    EXPECT_TRUE(layer_tree_host()->proxy()->CommitRequested());
   }
 
   void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override {

diff --git a/cc/trees/proxy_main.cc b/cc/trees/proxy_main.cc
index b1f5684..20299713 100644
--- a/cc/trees/proxy_main.cc
+++ b/cc/trees/proxy_main.cc

@@ -215,8 +215,8 @@
   // what this does.
   layer_tree_host_->BeginMainFrame(begin_main_frame_state->begin_frame_args);
 
-  // Updates cc animations on the main-thread. This appears to be entirely
-  // duplicated by work done in LayerTreeHost::BeginMainFrame. crbug.com/762717.
+  // Updates cc animations on the main-thread. This is necessary in order
+  // to track animation states such that they are cleaned up properly.
   layer_tree_host_->AnimateLayers(
       begin_main_frame_state->begin_frame_args.frame_time);