[animation-trigger] Ensure first frame reflects triggered animation [chromium/src : main]

0 views

Skip to first unread message

David Awogbemila (Gerrit)

unread,

Jan 8, 2026, 5:32:09 PM (3 days ago) Jan 8

to Kevin Ellis, Chromium LUCI CQ, Menard, Alexis, David Bokan, chromium...@chromium.org, Olga Gerchikov, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

Attention needed from Kevin Ellis

David Awogbemila voted Commit-Queue+1

Commit-Queue

Open in Gerrit

Related details

Attention is currently required from:

Kevin Ellis

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Kevin Ellis (Gerrit)

unread,

Jan 9, 2026, 12:31:39 PM (2 days ago) Jan 9

to David Awogbemila, Chromium LUCI CQ, Menard, Alexis, David Bokan, chromium...@chromium.org, Olga Gerchikov, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org

Attention needed from David Awogbemila

Kevin Ellis added 2 comments

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

Line 2873, Patchset 6 (Latest): if (timeline_->IsMonotonicallyIncreasing()) {

Kevin Ellis . unresolved

Worth adding a comment here. Historically, OnValidateSnapshot was for scroll timelines, and now we have separate logic for scroll triggering with a monotonic timeline (though most of the logic is consistent).

Documentation for this method in the header file only mentions scroll timelines. Suggesting that we update the comment in the header file, and add a note here where we have the branching logic.

File third_party/blink/renderer/core/animation/scroll_snapshot_timeline.cc

Line 218, Patchset 6 (Latest): animations.erase(animation);

Kevin Ellis . unresolved

We are effectively forcing the hashset to be copied just so we can remove elements from the map. AnimationTimeline::GetAnimations returns a const ref.

An SDA animation will be doing extra work here. because of the potential for animation triggers. Instead can we do the following:

```
const HeapHashSet<WeakMember<Animation>>& animations = GetAnimations();
if (Runtime...) {
  ...
  for (auto& [animation, behaviors] : trigger-BehaviorMap()) {
    DCHECK(...);
    // Avoid superfluous snapshot validation, by skipping the call if it will 
    // be invoked in the loop below. 
    if (!animations.Contains(animation)) {
      animation->OnValidateSnapshot(true);
    }
  }
}
```