From e3805f29fed78bac51e5ee5315719a4208f1ff10 Mon Sep 17 00:00:00 2001 From: Kevin Ellis Date: Fri, 11 Feb 2022 01:36:04 +0000 Subject: [PATCH] Code health cleanup: replacing animations. Animation::Update performed a synchronous processing of the finish microtask to ensure that finished events where dispatched ahead of replace events. This step does not align with the spec. Instead we should be queuing the replace event. Microtasks will be processed in the correct order. Spec link: https://www.w3.org/TR/web-animations-1/#timelines Change-Id: Ibe7753e792fb6cf905bbe6815a080a8cc51c2803 (cherry picked from commit d4fb69ff0fe343fe8a171014785a88eabfe2b1c2) Bug: 1290858 Change-Id: Ibe7753e792fb6cf905bbe6815a080a8cc51c2803 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3414765 Reviewed-by: Mustaq Ahmed Commit-Queue: Kevin Ellis Cr-Original-Commit-Position: refs/heads/main@{#964223} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3453925 Reviewed-by: Adrian Taylor Commit-Queue: Krishna Govind Cr-Commit-Position: refs/branch-heads/4758@{#1134} Cr-Branched-From: 4a2cf4baf90326df19c3ee70ff987960d59a386e-refs/heads/main@{#950365} --- third_party/blink/renderer/core/animation/animation.cc | 4 ---- .../blink/renderer/core/animation/document_animations.cc | 8 ++++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/third_party/blink/renderer/core/animation/animation.cc b/third_party/blink/renderer/core/animation/animation.cc index 18bfff861f6b17..63c6e37019de65 100644 --- a/third_party/blink/renderer/core/animation/animation.cc +++ b/third_party/blink/renderer/core/animation/animation.cc @@ -2320,10 +2320,6 @@ bool Animation::Update(TimingUpdateReason reason) { if (reason == kTimingUpdateForAnimationFrame) { if (idle || CalculateAnimationPlayState() == kFinished) { - // TODO(crbug.com/1029348): Per spec, we should have a microtask - // checkpoint right after the update cycle. Once this is fixed we should - // no longer need to force a synchronous resolution here. - AsyncFinishMicrotask(); finished_ = true; } } diff --git a/third_party/blink/renderer/core/animation/document_animations.cc b/third_party/blink/renderer/core/animation/document_animations.cc index 32cfdff88acf1e..1eb731c5fc91be 100644 --- a/third_party/blink/renderer/core/animation/document_animations.cc +++ b/third_party/blink/renderer/core/animation/document_animations.cc @@ -49,6 +49,7 @@ #include "third_party/blink/renderer/core/page/page_animator.h" #include "third_party/blink/renderer/core/style/computed_style.h" #include "third_party/blink/renderer/platform/bindings/microtask.h" +#include "third_party/blink/renderer/platform/heap/persistent.h" namespace blink { @@ -260,10 +261,13 @@ void DocumentAnimations::RemoveReplacedAnimations( // 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. + // dispatched in composite order. Queue as a microtask so that the finished + // event is dispatched ahead of the remove event. for (auto it = animations_to_remove.rbegin(); it != animations_to_remove.rend(); it++) { - (*it)->RemoveReplacedAnimation(); + Animation* animation = *it; + Microtask::EnqueueMicrotask(WTF::Bind(&Animation::RemoveReplacedAnimation, + WrapWeakPersistent(animation))); } }