Fix handling of replaceable animations with multiple timelines.

When removing a replaced animation, it is important to consider all
timelines.  Previously, we made the decision on a per timeline basis,
resulting in failing to flag some animations as replaceable.

Bug: 10293484

Change-Id: I8256f2eb69f710ba1ec04b095446bd30c1b64a97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401505Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805499}

third_party/blink/renderer/core/animation/animation_timeline.cc

@@ -117,18 +117,11 @@ void AnimationTimeline::ServiceAnimations(TimingUpdateReason reason) {
   // Explicitly free the backing store to avoid memory regressions.
   // TODO(bikineev): Revisit when young generation is done.
   animations.clear();
-
-  if (RuntimeEnabledFeatures::WebAnimationsAPIEnabled() &&
-      reason == kTimingUpdateForAnimationFrame) {
-    RemoveReplacedAnimations();
-  }
 }
 
 // https://drafts.csswg.org/web-animations-1/#removing-replaced-animations
-void AnimationTimeline::RemoveReplacedAnimations() {
-  // Group replaceable animations by target element.
-  HeapHashMap<Member<Element>, Member<HeapVector<Member<Animation>>>>
-      replaceable_animations;
+void AnimationTimeline::getReplaceableAnimations(
+    AnimationTimeline::ReplaceableAnimationsMap* replaceable_animations_map) {
   for (Animation* animation : animations_) {
     // Initial conditions for removal:
     // * has an associated animation effect whose effect target is a descendant
@@ -142,62 +135,13 @@ void AnimationTimeline::RemoveReplacedAnimations() {
     if (target->GetDocument() != animation->GetDocument())
       continue;
 
-    auto inserted = replaceable_animations.insert(target, nullptr);
+    auto inserted = replaceable_animations_map->insert(target, nullptr);
     if (inserted.is_new_entry) {
       inserted.stored_value->value =
           MakeGarbageCollected<HeapVector<Member<Animation>>>();
     }
     inserted.stored_value->value->push_back(animation);
   }
-
-  HeapVector<Member<Animation>> animations_to_remove;
-  for (auto& elem_it : replaceable_animations) {
-    HeapVector<Member<Animation>>* animations = elem_it.value;
-
-    // Only elements with multiple animations in the replaceable state need to
-    // be checked.
-    if (animations->size() == 1)
-      continue;
-
-    // By processing in decreasing order by priority, we can perform a single
-    // pass for discovery of replaced properties.
-    std::sort(animations->begin(), animations->end(), CompareAnimations);
-    PropertyHandleSet replaced_properties;
-    for (auto anim_it = animations->rbegin(); anim_it != animations->rend();
-         anim_it++) {
-      // Remaining conditions for removal:
-      // * has a replace state of active,  and
-      // * for which there exists for each target property of every animation
-      //   effect associated with animation, an animation effect associated with
-      //   a replaceable animation with a higher composite order than animation
-      //   that includes the same target property.
-
-      // Only active animations can be removed. We still need to go through
-      // the process of iterating over properties if not removable to update
-      // the set of properties being replaced.
-      bool replace = (*anim_it)->ReplaceStateActive();
-      PropertyHandleSet animation_properties =
-          To<KeyframeEffect>((*anim_it)->effect())->Model()->Properties();
-      for (const auto& property : animation_properties) {
-        auto inserted = replaced_properties.insert(property);
-        if (inserted.is_new_entry) {
-          // Top-most compositor order animation affecting this property.
-          replace = false;
-        }
-      }
-      if (replace)
-        animations_to_remove.push_back(*anim_it);
-    }
-  }
-
-  // The list of animations for removal is constructed in reverse composite
-  // ordering for efficiency. Flip the ordering to ensure that events are
-  // dispatched in composite order.
-  // TODO(crbug.com/981905): Add test for ordering once onremove is implemented.
-  for (auto it = animations_to_remove.rbegin();
-       it != animations_to_remove.rend(); it++) {
-    (*it)->RemoveReplacedAnimation();
-  }
 }
 
 void AnimationTimeline::SetOutdatedAnimation(Animation* animation) {

third_party/blink/renderer/core/animation/animation_timeline.h

@@ -94,11 +94,15 @@ class CORE_EXPORT AnimationTimeline : public ScriptWrappable {
 
   void MarkAnimationsCompositorPending(bool source_changed = false);
 
+  using ReplaceableAnimationsMap =
+      HeapHashMap<Member<Element>, Member<HeapVector<Member<Animation>>>>;
+  void getReplaceableAnimations(
+      ReplaceableAnimationsMap* replaceable_animation_set);
+
   void Trace(Visitor*) const override;
 
  protected:
   virtual PhaseAndTime CurrentPhaseAndTime() = 0;
-  void RemoveReplacedAnimations();
 
   Member<Document> document_;
   unsigned outdated_animation_count_;

third_party/blink/renderer/core/animation/document_animations.cc

@@ -73,14 +73,22 @@ void DocumentAnimations::AddTimeline(AnimationTimeline& timeline) {
 }
 
 void DocumentAnimations::UpdateAnimationTimingForAnimationFrame() {
+  // https://drafts.csswg.org/web-animations-1/#timelines.
+
+  // 1. Update the current time of all timelines associated with doc passing now
+  //    as the timestamp.
   UpdateAnimationTiming(*document_, timelines_, kTimingUpdateForAnimationFrame);
 
-  // Perform a microtask checkpoint per step 3 of
-  // https://drafts.csswg.org/web-animations-1/#timelines. This is to
-  // ensure that any microtasks queued up as a result of resolving or
-  // rejecting Promise objects as part of updating timelines run their
-  // callbacks prior to dispatching animation events and generating
-  // the next main frame.
+  // 2. Remove replaced animations for doc.
+  ReplaceableAnimationsMap replaceable_animations_map;
+  for (auto& timeline : timelines_)
+    timeline->getReplaceableAnimations(&replaceable_animations_map);
+  RemoveReplacedAnimations(&replaceable_animations_map);
+
+  // 3. Perform a microtask checkpoint
+  // This is to ensure that any microtasks queued up as a result of resolving or
+  // rejecting Promise objects as part of updating timelines run their callbacks
+  // prior to dispatching animation events and generating the next main frame.
   Microtask::PerformCheckpoint(V8PerIsolateData::MainThreadIsolate());
 }
 
@@ -181,4 +189,56 @@ void DocumentAnimations::GetAnimationsTargetingTreeScope(
     }
   }
 }
+
+void DocumentAnimations::RemoveReplacedAnimations(
+    DocumentAnimations::ReplaceableAnimationsMap* replaceable_animations_map) {
+  HeapVector<Member<Animation>> animations_to_remove;
+  for (auto& elem_it : *replaceable_animations_map) {
+    HeapVector<Member<Animation>>* animations = elem_it.value;
+
+    // Only elements with multiple animations in the replaceable state need to
+    // be checked.
+    if (animations->size() == 1)
+      continue;
+
+    // By processing in decreasing order by priority, we can perform a single
+    // pass for discovery of replaced properties.
+    std::sort(animations->begin(), animations->end(), CompareAnimations);
+    PropertyHandleSet replaced_properties;
+    for (auto anim_it = animations->rbegin(); anim_it != animations->rend();
+         anim_it++) {
+      // Remaining conditions for removal:
+      // * has a replace state of active,  and
+      // * for which there exists for each target property of every animation
+      //   effect associated with animation, an animation effect associated with
+      //   a replaceable animation with a higher composite order than animation
+      //   that includes the same target property.
+
+      // Only active animations can be removed. We still need to go through
+      // the process of iterating over properties if not removable to update
+      // the set of properties being replaced.
+      bool replace = (*anim_it)->ReplaceStateActive();
+      PropertyHandleSet animation_properties =
+          To<KeyframeEffect>((*anim_it)->effect())->Model()->Properties();
+      for (const auto& property : animation_properties) {
+        auto inserted = replaced_properties.insert(property);
+        if (inserted.is_new_entry) {
+          // Top-most compositor order animation affecting this property.
+          replace = false;
+        }
+      }
+      if (replace)
+        animations_to_remove.push_back(*anim_it);
+    }
+  }
+
+  // The list of animations for removal is constructed in reverse composite
+  // ordering for efficiency. Flip the ordering to ensure that events are
+  // dispatched in composite order.
+  for (auto it = animations_to_remove.rbegin();
+       it != animations_to_remove.rend(); it++) {
+    (*it)->RemoveReplacedAnimation();
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/core/animation/document_animations.h

@@ -75,6 +75,11 @@ class CORE_EXPORT DocumentAnimations final
   uint64_t current_transition_generation_;
   void Trace(Visitor*) const;
 
+ protected:
+  using ReplaceableAnimationsMap =
+      HeapHashMap<Member<Element>, Member<HeapVector<Member<Animation>>>>;
+  void RemoveReplacedAnimations(ReplaceableAnimationsMap*);
+
  private:
   Member<Document> document_;
   HeapHashSet<WeakMember<AnimationTimeline>> timelines_;

third_party/blink/web_tests/external/wpt/web-animations/timing-model/timelines/update-and-send-events-replacement-expected.txt

@@ -9,8 +9,8 @@ PASS Removes an animation after updating the fill mode of another animation
 PASS Removes an animation after updating its fill mode
 PASS Removes an animation after updating another animation's effect to one with different timing
 PASS Removes an animation after updating its effect to one with different timing
-FAIL Removes an animation after updating another animation's timeline assert_equals: expected "removed" but got "active"
-FAIL Removes an animation after updating its timeline assert_equals: expected "removed" but got "active"
+PASS Removes an animation after updating another animation's timeline
+PASS Removes an animation after updating its timeline
 PASS Removes an animation after updating another animation's effect's properties
 PASS Removes an animation after updating its effect's properties
 PASS Removes an animation after updating another animation's effect to one with different properties
@@ -36,7 +36,7 @@ PASS Does NOT remove an animation after making a redundant change to another ani
 PASS Does NOT remove an animation after making a redundant change to its timeline
 PASS Does NOT remove an animation after making a redundant change to another animation's effect's properties
 PASS Does NOT remove an animation after making a redundant change to its effect's properties
-FAIL Updates ALL timelines before checking for replacement assert_equals: expected "removed" but got "active"
+PASS Updates ALL timelines before checking for replacement
 PASS Dispatches remove events after finish events
 PASS Fires remove event before requestAnimationFrame
 PASS Queues all remove events before running them