Revert "Remove KeyframeModel ids from animation clients"

This reverts commit e73a825.

Reason for revert: Reverting this patch as it regressed ui animation on ChromeOS. Problem has been located. Fix it in a follow up patch.

Original change's description:
> Remove KeyframeModel ids from animation clients
> 
> Currently removing/pausing/aborting keyframe models to cc::Animation
> requires the client to provide the keyframe model id. e.g.
> Animation::RemoveKeyframeModel(id).
> 
> Actually in reality we never remove/abort individual keyframe model of
> an animation but do them all together. This patch removes the ids from
> the clients so that they don't need to track the keyframe models.
> 
> This patch is to clear barriers of introducing Group Effects.
> 
> The non trivial changes on ui animation:
> 
> Current logic: LayerAnimationSequence owns n LayerAnimationElement which
> could be either threaded (opacity, transform) or non threaded. Each of
> the threaded LayerAnimationElements has a corresponding
> cc::KeyframeModel which is added / removed on the LayerAnimationElement
> level. e.g. element->AddKeyframeModel(), element->RemoveKeyframeModel().
> 
> New logic: Given that threaded keyframe models can only be removed all
> together, we move the removal logic to the LayerAnimationSequence level.
> e.g. sequence->RemoveKeyframeModels(). The *add* logic is still on the
> element level because adding individual keyframe model for each element
> is allowed.
> Note that a sequence can contain both threaded and non threaded elements
> simultaneously. e.g. [threaded, non threaded, threaded]. To make sure
> that we don't remove the keyframe model from the second threaded element
> upon finishing the first threaded element, we need to finish the whole
> sequence to proceed. Also note that the sequence may be repeated
> infinitely and every time a threaded element is started we create a
> cc::KeyframeModel for it. That being said, we need to make sure that the
> existing keyframe models are removed before the sequence gets repeated.
> 
> Bug: 810003
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: Ia7dd8dab07a374da82f1619d011a23c31421295b
> Reviewed-on: https://chromium-review.googlesource.com/1102831
> Commit-Queue: Yi Gu <yigu@chromium.org>
> Reviewed-by: Majid Valipour <majidvp@chromium.org>
> Reviewed-by: Ali Juma <ajuma@chromium.org>
> Reviewed-by: Ian Vollick <vollick@chromium.org>
> Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#571658}

TBR=vollick@chromium.org,ajuma@chromium.org,majidvp@chromium.org,yigu@chromium.org,smcgruer@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 810003
Change-Id: I4e7bdc75ad5ad06a6f0079d19a2b867d207a518a
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:linux_vr;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1126361
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572652}

cc/animation/animation.cc

@@ -205,30 +205,35 @@ void Animation::AddKeyframeModelForKeyframeEffect(
       ->AddKeyframeModel(std::move(keyframe_model));
 }
 
-void Animation::RemoveKeyframeModelsForKeyframeEffect(
+void Animation::PauseKeyframeModelForKeyframeEffect(
+    int keyframe_model_id,
+    double time_offset,
     KeyframeEffectId keyframe_effect_id) {
   DCHECK(GetKeyframeEffectById(keyframe_effect_id));
-  GetKeyframeEffectById(keyframe_effect_id)->RemoveKeyframeModels();
+  GetKeyframeEffectById(keyframe_effect_id)
+      ->PauseKeyframeModel(keyframe_model_id, time_offset);
 }
 
-void Animation::PauseKeyframeEffect(double time_offset,
-                                    KeyframeEffectId keyframe_effect_id) {
+void Animation::RemoveKeyframeModelForKeyframeEffect(
+    int keyframe_model_id,
+    KeyframeEffectId keyframe_effect_id) {
   DCHECK(GetKeyframeEffectById(keyframe_effect_id));
   GetKeyframeEffectById(keyframe_effect_id)
-      ->Pause(base::TimeDelta::FromSecondsD(time_offset));
+      ->RemoveKeyframeModel(keyframe_model_id);
 }
 
-void Animation::AbortKeyframeEffect(KeyframeEffectId keyframe_effect_id) {
+void Animation::AbortKeyframeModelForKeyframeEffect(
+    int keyframe_model_id,
+    KeyframeEffectId keyframe_effect_id) {
   DCHECK(GetKeyframeEffectById(keyframe_effect_id));
-  GetKeyframeEffectById(keyframe_effect_id)->Abort();
+  GetKeyframeEffectById(keyframe_effect_id)
+      ->AbortKeyframeModel(keyframe_model_id);
 }
 
-void Animation::AbortKeyframeModelsWithProperty(
-    TargetProperty::Type target_property,
-    bool needs_completion) {
+void Animation::AbortKeyframeModels(TargetProperty::Type target_property,
+                                    bool needs_completion) {
   for (auto& keyframe_effect : keyframe_effects_)
-    keyframe_effect->AbortKeyframeModelsWithProperty(target_property,
-                                                     needs_completion);
+    keyframe_effect->AbortKeyframeModels(target_property, needs_completion);
 }
 
 void Animation::PushPropertiesTo(Animation* animation_impl) {

cc/animation/animation.h

@@ -83,13 +83,16 @@ class CC_ANIMATION_EXPORT Animation : public base::RefCounted<Animation> {
   void AddKeyframeModelForKeyframeEffect(
       std::unique_ptr<KeyframeModel> keyframe_model,
       KeyframeEffectId keyframe_effect_id);
-  void RemoveKeyframeModelsForKeyframeEffect(
+  void PauseKeyframeModelForKeyframeEffect(int keyframe_model_id,
+                                           double time_offset,
+                                           KeyframeEffectId keyframe_effect_id);
+  void RemoveKeyframeModelForKeyframeEffect(
+      int keyframe_model_id,
       KeyframeEffectId keyframe_effect_id);
-  void PauseKeyframeEffect(double time_offset,
-                           KeyframeEffectId keyframe_effect_id);
-  void AbortKeyframeEffect(KeyframeEffectId keyframe_effect_id);
-  void AbortKeyframeModelsWithProperty(TargetProperty::Type target_property,
-                                       bool needs_completion);
+  void AbortKeyframeModelForKeyframeEffect(int keyframe_model_id,
+                                           KeyframeEffectId keyframe_effect_id);
+  void AbortKeyframeModels(TargetProperty::Type target_property,
+                           bool needs_completion);
 
   virtual void PushPropertiesTo(Animation* animation_impl);
 

cc/animation/animation_unittest.cc

@@ -367,6 +367,8 @@ TEST_F(AnimationTest, AddRemoveAnimationToNonAttachedAnimation) {
   const float start_opacity = .7f;
   const float end_opacity = .3f;
 
+  const int filter_id = AddAnimatedFilterToAnimation(
+      animation_.get(), duration, 0.1f, 0.9f, keyframe_effect_id_);
   AddOpacityTransitionToAnimation(animation_.get(), duration, start_opacity,
                                   end_opacity, false, keyframe_effect_id_);
 
@@ -380,11 +382,18 @@ TEST_F(AnimationTest, AddRemoveAnimationToNonAttachedAnimation) {
                    ->needs_push_properties());
   EXPECT_FALSE(animation_->GetKeyframeEffectById(keyframe_effect_id_)
                    ->element_animations());
+  animation_->RemoveKeyframeModelForKeyframeEffect(filter_id,
+                                                   keyframe_effect_id_);
+  EXPECT_FALSE(animation_->GetKeyframeEffectById(keyframe_effect_id_)
+                   ->needs_push_properties());
 
   animation_->AttachElementForKeyframeEffect(element_id_, keyframe_effect_id_);
 
   EXPECT_TRUE(animation_->GetKeyframeEffectById(keyframe_effect_id_)
                   ->element_animations());
+  EXPECT_FALSE(animation_->GetKeyframeEffectById(keyframe_effect_id_)
+                   ->element_animations()
+                   ->HasAnyAnimationTargetingProperty(TargetProperty::FILTER));
   EXPECT_TRUE(animation_->GetKeyframeEffectById(keyframe_effect_id_)
                   ->element_animations()
                   ->HasAnyAnimationTargetingProperty(TargetProperty::OPACITY));
@@ -398,6 +407,11 @@ TEST_F(AnimationTest, AddRemoveAnimationToNonAttachedAnimation) {
   EXPECT_FALSE(client_impl_.IsPropertyMutated(
       element_id_, ElementListType::ACTIVE, TargetProperty::OPACITY));
 
+  EXPECT_FALSE(client_.IsPropertyMutated(element_id_, ElementListType::ACTIVE,
+                                         TargetProperty::FILTER));
+  EXPECT_FALSE(client_impl_.IsPropertyMutated(
+      element_id_, ElementListType::ACTIVE, TargetProperty::FILTER));
+
   host_impl_->ActivateAnimations();
 
   base::TimeTicks time;
@@ -413,6 +427,11 @@ TEST_F(AnimationTest, AddRemoveAnimationToNonAttachedAnimation) {
       element_id_, ElementListType::ACTIVE, end_opacity);
   client_impl_.ExpectOpacityPropertyMutated(
       element_id_, ElementListType::PENDING, end_opacity);
+
+  EXPECT_FALSE(client_.IsPropertyMutated(element_id_, ElementListType::ACTIVE,
+                                         TargetProperty::FILTER));
+  EXPECT_FALSE(client_impl_.IsPropertyMutated(
+      element_id_, ElementListType::ACTIVE, TargetProperty::FILTER));
 }
 
 TEST_F(AnimationTest, AddRemoveAnimationCausesSetNeedsCommit) {
@@ -427,17 +446,19 @@ TEST_F(AnimationTest, AddRemoveAnimationCausesSetNeedsCommit) {
 
   EXPECT_FALSE(client_.mutators_need_commit());
 
-  AddOpacityTransitionToAnimation(animation_.get(), 1., .7f, .3f, false,
-                                  keyframe_effect_id_);
+  const int keyframe_model_id = AddOpacityTransitionToAnimation(
+      animation_.get(), 1., .7f, .3f, false, keyframe_effect_id_);
 
   EXPECT_TRUE(client_.mutators_need_commit());
   client_.set_mutators_need_commit(false);
 
-  animation_->PauseKeyframeEffect(1., keyframe_effect_id_);
+  animation_->PauseKeyframeModelForKeyframeEffect(keyframe_model_id, 1.,
+                                                  keyframe_effect_id_);
   EXPECT_TRUE(client_.mutators_need_commit());
   client_.set_mutators_need_commit(false);
 
-  animation_->RemoveKeyframeModelsForKeyframeEffect(keyframe_effect_id_);
+  animation_->RemoveKeyframeModelForKeyframeEffect(keyframe_model_id,
+                                                   keyframe_effect_id_);
   EXPECT_TRUE(client_.mutators_need_commit());
   client_.set_mutators_need_commit(false);
 }

cc/animation/element_animations_unittest.cc

@@ -615,7 +615,7 @@ TEST_F(ElementAnimationsTest, SyncPause) {
 
   // Pause the animation at the middle of the second range so the offset
   // delays animation until the middle of the third range.
-  animation_->PauseKeyframeEffect(1.5);
+  animation_->PauseKeyframeModel(keyframe_model_id, 1.5);
   EXPECT_EQ(KeyframeModel::PAUSED, animation_->keyframe_effect()
                                        ->GetKeyframeModelById(keyframe_model_id)
                                        ->run_state());
@@ -1103,8 +1103,7 @@ TEST_F(ElementAnimationsTest, ScrollOffsetRemovalClearsScrollDelta) {
 
   auto events = CreateEventsForTesting();
 
-  // Removing keyframe models with target SCROLL_OFFSET leads to
-  // scroll_offset_animation_was_interrupted being set to true.
+  // First test the 1-argument version of RemoveKeyframeModel.
   gfx::ScrollOffset target_value(300.f, 200.f);
   std::unique_ptr<ScrollOffsetAnimationCurve> curve(
       ScrollOffsetAnimationCurve::Create(
@@ -1123,7 +1122,7 @@ TEST_F(ElementAnimationsTest, ScrollOffsetRemovalClearsScrollDelta) {
   EXPECT_FALSE(animation_impl_->keyframe_effect()
                    ->scroll_offset_animation_was_interrupted());
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
   EXPECT_TRUE(
       animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
 
@@ -1137,7 +1136,36 @@ TEST_F(ElementAnimationsTest, ScrollOffsetRemovalClearsScrollDelta) {
   EXPECT_FALSE(animation_impl_->keyframe_effect()
                    ->scroll_offset_animation_was_interrupted());
 
-  // Check that removing non-scroll-offset keyframe models does not cause
+  // Now, test the 2-argument version of RemoveKeyframeModel.
+  curve = ScrollOffsetAnimationCurve::Create(
+      target_value, CubicBezierTimingFunction::CreatePreset(
+                        CubicBezierTimingFunction::EaseType::EASE_IN_OUT));
+  keyframe_model = KeyframeModel::Create(std::move(curve), keyframe_model_id, 0,
+                                         TargetProperty::SCROLL_OFFSET);
+  keyframe_model->set_needs_synchronized_start_time(true);
+  animation_->AddKeyframeModel(std::move(keyframe_model));
+  PushProperties();
+  animation_impl_->ActivateKeyframeEffects();
+  EXPECT_FALSE(
+      animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
+  EXPECT_FALSE(animation_impl_->keyframe_effect()
+                   ->scroll_offset_animation_was_interrupted());
+
+  animation_->RemoveKeyframeModel(keyframe_model_id);
+  EXPECT_TRUE(
+      animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
+
+  PushProperties();
+  EXPECT_TRUE(animation_impl_->keyframe_effect()
+                  ->scroll_offset_animation_was_interrupted());
+  EXPECT_FALSE(
+      animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
+
+  animation_impl_->ActivateKeyframeEffects();
+  EXPECT_FALSE(animation_impl_->keyframe_effect()
+                   ->scroll_offset_animation_was_interrupted());
+
+  // Check that removing non-scroll-offset animations does not cause
   // scroll_offset_animation_was_interrupted() to get set.
   keyframe_model_id =
       AddAnimatedTransformToAnimation(animation_.get(), 1.0, 1, 2);
@@ -1148,7 +1176,7 @@ TEST_F(ElementAnimationsTest, ScrollOffsetRemovalClearsScrollDelta) {
   EXPECT_FALSE(animation_impl_->keyframe_effect()
                    ->scroll_offset_animation_was_interrupted());
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
   EXPECT_FALSE(
       animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
 
@@ -1171,7 +1199,7 @@ TEST_F(ElementAnimationsTest, ScrollOffsetRemovalClearsScrollDelta) {
   EXPECT_FALSE(animation_impl_->keyframe_effect()
                    ->scroll_offset_animation_was_interrupted());
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
   EXPECT_FALSE(
       animation_->keyframe_effect()->scroll_offset_animation_was_interrupted());
 
@@ -1389,7 +1417,7 @@ TEST_F(ElementAnimationsTest, Interrupt) {
   std::unique_ptr<KeyframeModel> to_add(CreateKeyframeModel(
       std::unique_ptr<AnimationCurve>(new FakeFloatTransition(1.0, 1.f, 0.5f)),
       2, TargetProperty::OPACITY));
-  animation_->AbortKeyframeModelsWithProperty(TargetProperty::OPACITY, false);
+  animation_->AbortKeyframeModels(TargetProperty::OPACITY, false);
   animation_->AddKeyframeModel(std::move(to_add));
 
   // Since the previous animation was aborted, the new animation should start
@@ -1802,9 +1830,9 @@ TEST_F(ElementAnimationsTest, InactiveObserverGetsTicked) {
             client_impl_.GetOpacity(element_id_, ElementListType::ACTIVE));
 }
 
-// Tests that AbortKeyframeModelsWithProperty aborts all animations targeting
-// the specified property.
-TEST_F(ElementAnimationsTest, AbortKeyframeModelsWithProperty) {
+// Tests that AbortKeyframeModels aborts all animations targeting the
+// specified property.
+TEST_F(ElementAnimationsTest, AbortKeyframeModels) {
   CreateTestLayer(false, false);
   AttachTimelineAnimationLayer();
 
@@ -1847,7 +1875,7 @@ TEST_F(ElementAnimationsTest, AbortKeyframeModelsWithProperty) {
       KeyframeModel::RUNNING,
       animation_->keyframe_effect()->GetKeyframeModelById(5)->run_state());
 
-  animation_->AbortKeyframeModelsWithProperty(TargetProperty::TRANSFORM, false);
+  animation_->AbortKeyframeModels(TargetProperty::TRANSFORM, false);
 
   // Only un-finished TRANSFORM animations should have been aborted.
   EXPECT_EQ(
@@ -1885,7 +1913,7 @@ TEST_F(ElementAnimationsTest, MainThreadAbortedAnimationGetsDeleted) {
       keyframe_model_id));
   EXPECT_FALSE(host_->needs_push_properties());
 
-  animation_->AbortKeyframeModelsWithProperty(TargetProperty::OPACITY, false);
+  animation_->AbortKeyframeModels(TargetProperty::OPACITY, false);
   EXPECT_EQ(KeyframeModel::ABORTED,
             animation_->GetKeyframeModel(TargetProperty::OPACITY)->run_state());
   EXPECT_TRUE(host_->needs_push_properties());
@@ -1927,8 +1955,7 @@ TEST_F(ElementAnimationsTest, ImplThreadAbortedAnimationGetsDeleted) {
   EXPECT_TRUE(animation_impl_->keyframe_effect()->GetKeyframeModelById(
       keyframe_model_id));
 
-  animation_impl_->AbortKeyframeModelsWithProperty(TargetProperty::OPACITY,
-                                                   false);
+  animation_impl_->AbortKeyframeModels(TargetProperty::OPACITY, false);
   EXPECT_EQ(
       KeyframeModel::ABORTED,
       animation_impl_->GetKeyframeModel(TargetProperty::OPACITY)->run_state());
@@ -1999,8 +2026,7 @@ TEST_F(ElementAnimationsTest, ImplThreadTakeoverAnimationGetsDeleted) {
   EXPECT_TRUE(animation_impl_->keyframe_effect()->GetKeyframeModelById(
       keyframe_model_id));
 
-  animation_impl_->AbortKeyframeModelsWithProperty(
-      TargetProperty::SCROLL_OFFSET, true);
+  animation_impl_->AbortKeyframeModels(TargetProperty::SCROLL_OFFSET, true);
   EXPECT_TRUE(host_impl_->needs_push_properties());
   EXPECT_EQ(KeyframeModel::ABORTED_BUT_NEEDS_COMPLETION,
             animation_impl_->GetKeyframeModel(TargetProperty::SCROLL_OFFSET)
@@ -2126,8 +2152,7 @@ TEST_F(ElementAnimationsTest, FinishedAndAbortedEventsForGroup) {
   EXPECT_EQ(AnimationEvent::STARTED, events->events_[0].type);
   EXPECT_EQ(AnimationEvent::STARTED, events->events_[1].type);
 
-  animation_impl_->AbortKeyframeModelsWithProperty(TargetProperty::OPACITY,
-                                                   false);
+  animation_impl_->AbortKeyframeModels(TargetProperty::OPACITY, false);
 
   events = CreateEventsForTesting();
   animation_impl_->Tick(kInitialTickTime + TimeDelta::FromMilliseconds(1000));
@@ -2274,7 +2299,7 @@ TEST_F(ElementAnimationsTest, AnimationStartScale) {
   curve2->AddKeyframe(TransformKeyframe::Create(
       base::TimeDelta::FromSecondsD(1.0), operations3, nullptr));
 
-  animation_impl_->RemoveKeyframeModels();
+  animation_impl_->RemoveKeyframeModel(1);
   keyframe_model =
       KeyframeModel::Create(std::move(curve2), 2, 2, TargetProperty::TRANSFORM);
 
@@ -2799,7 +2824,8 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenTransformAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
+  animation_->RemoveKeyframeModel(animation2_id);
   EXPECT_FALSE(client_.GetHasPotentialTransformAnimation(
       element_id_, ElementListType::ACTIVE));
   EXPECT_FALSE(client_.GetTransformIsCurrentlyAnimating(
@@ -2851,8 +2877,7 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenTransformAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_impl_->AbortKeyframeModelsWithProperty(TargetProperty::TRANSFORM,
-                                                   false);
+  animation_impl_->AbortKeyframeModels(TargetProperty::TRANSFORM, false);
   EXPECT_FALSE(client_impl_.GetHasPotentialTransformAnimation(
       element_id_, ElementListType::PENDING));
   EXPECT_FALSE(client_impl_.GetTransformIsCurrentlyAnimating(
@@ -3014,7 +3039,7 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenOpacityAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
   EXPECT_FALSE(client_.GetHasPotentialOpacityAnimation(
       element_id_, ElementListType::ACTIVE));
   EXPECT_FALSE(client_.GetOpacityIsCurrentlyAnimating(element_id_,
@@ -3066,8 +3091,7 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenOpacityAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_impl_->AbortKeyframeModelsWithProperty(TargetProperty::OPACITY,
-                                                   false);
+  animation_impl_->AbortKeyframeModels(TargetProperty::OPACITY, false);
   EXPECT_FALSE(client_impl_.GetHasPotentialOpacityAnimation(
       element_id_, ElementListType::PENDING));
   EXPECT_FALSE(client_impl_.GetOpacityIsCurrentlyAnimating(
@@ -3228,7 +3252,7 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenFilterAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(keyframe_model_id);
   EXPECT_FALSE(client_.GetHasPotentialFilterAnimation(element_id_,
                                                       ElementListType::ACTIVE));
   EXPECT_FALSE(client_.GetFilterIsCurrentlyAnimating(element_id_,
@@ -3280,8 +3304,7 @@ TEST_F(ElementAnimationsTest, ObserverNotifiedWhenFilterAnimationChanges) {
   animation_->keyframe_effect()->NotifyKeyframeModelStarted(events->events_[0]);
   events->events_.clear();
 
-  animation_impl_->AbortKeyframeModelsWithProperty(TargetProperty::FILTER,
-                                                   false);
+  animation_impl_->AbortKeyframeModels(TargetProperty::FILTER, false);
   EXPECT_FALSE(client_impl_.GetHasPotentialFilterAnimation(
       element_id_, ElementListType::PENDING));
   EXPECT_FALSE(client_impl_.GetFilterIsCurrentlyAnimating(
@@ -3385,7 +3408,8 @@ TEST_F(ElementAnimationsTest, PushedDeletedAnimationWaitsForActivation) {
                   ->affects_active_elements());
 
   // Delete the animation on the main-thread animations.
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(
+      animation_->GetKeyframeModel(TargetProperty::OPACITY)->id());
   PushProperties();
 
   // The animation should no longer affect pending elements.
@@ -3444,7 +3468,8 @@ TEST_F(ElementAnimationsTest, StartAnimationsAffectingDifferentObservers) {
 
   // Remove the first animation from the main-thread animations, and add a
   // new animation affecting the same property.
-  animation_->RemoveKeyframeModels();
+  animation_->RemoveKeyframeModel(
+      animation_->GetKeyframeModel(TargetProperty::OPACITY)->id());
   const int second_keyframe_model_id =
       AddOpacityTransitionToAnimation(animation_.get(), 1, 1.f, 0.5f, true);
   PushProperties();