[animation-trigger] Update attachments when trigger-scope changes [chromium/src : main]

0 views

Skip to first unread message

AI Code Reviewer (Gerrit)

unread,

Dec 8, 2025, 4:17:48 PM (yesterday) Dec 8

to David Awogbemila, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Olga Gerchikov, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

AI Code Reviewer added 3 comments

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

Line 105, Patchset 1 (Latest): RelevantTriggerAttachmentMap& relevant_attachments);

AI Code Reviewer . unresolved

The 'relevant_attachments' parameter is not modified in this function. Prefer passing it by const reference (const RelevantTriggerAttachmentMap&) to indicate this and ensure const-correctness.

To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason

_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_

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

Line 146, Patchset 1 (Latest): RelevantTriggerAttachmentMap& relevant_attachments) {

AI Code Reviewer . unresolved

The 'relevant_attachments' parameter is not modified in this function. Prefer passing it by const reference (const RelevantTriggerAttachmentMap&) to indicate this and ensure const-correctness.

Line 161, Patchset 1 (Latest): // obsolete add should be removed. However, don't call removeAnimation just

AI Code Reviewer . unresolved

nit: Typo in comment. "obsolete add should be removed" -> "obsolete and should be removed".

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Awogbemila (Gerrit)

unread,

Dec 8, 2025, 7:28:08 PM (yesterday) Dec 8

to Anders Hartvoll Ruud, AI Code Reviewer, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Olga Gerchikov, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from Anders Hartvoll Ruud

David Awogbemila added 3 comments

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

Line 105, Patchset 1: RelevantTriggerAttachmentMap& relevant_attachments);

AI Code Reviewer . resolved

The 'relevant_attachments' parameter is not modified in this function. Prefer passing it by const reference (const RelevantTriggerAttachmentMap&) to indicate this and ensure const-correctness.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason

_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_

David Awogbemila

Done

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

Line 146, Patchset 1: RelevantTriggerAttachmentMap& relevant_attachments) {

AI Code Reviewer . resolved

The 'relevant_attachments' parameter is not modified in this function. Prefer passing it by const reference (const RelevantTriggerAttachmentMap&) to indicate this and ensure const-correctness.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason

_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_

David Awogbemila

Done

Line 161, Patchset 1: // obsolete add should be removed. However, don't call removeAnimation just

AI Code Reviewer . resolved

nit: Typo in comment. "obsolete add should be removed" -> "obsolete and should be removed".
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason

_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_

David Awogbemila

Done

Open in Gerrit

Related details

Attention is currently required from:

Anders Hartvoll Ruud

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

Anders Hartvoll Ruud (Gerrit)

unread,

7:48 AM (15 hours ago) 7:48 AM

to David Awogbemila, AI Code Reviewer, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Olga Gerchikov, blink-revie...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from David Awogbemila

Anders Hartvoll Ruud added 1 comment

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

Line 1999, Patchset 5 (Latest):

            if (existing_trigger_attachments || trigger_attachments) {
              element.GetDocument()
                  .GetDocumentAnimations()
                  .AddPendingTriggerAttachmentUpdate(animation);
            }

Anders Hartvoll Ruud . unresolved

We cannot modify `DocumentAnimations` here. You would need to queue up any changes to `update`, then apply those onto the "wider world" during `MaybeApplyPendingUpdate`. This is because `update` may discarded as a whole, e.g. if we're in one of several style resolution rounds for container queries.

Open in Gerrit

Related details

Attention is currently required from:

David Awogbemila

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Awogbemila (Gerrit)

unread,

10:37 AM (12 hours ago) 10:37 AM

Attention needed from Anders Hartvoll Ruud

David Awogbemila added 1 comment

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

Line 1999, Patchset 5 (Latest):

            if (existing_trigger_attachments || trigger_attachments) {
              element.GetDocument()
                  .GetDocumentAnimations()
                  .AddPendingTriggerAttachmentUpdate(animation);
            }

Anders Hartvoll Ruud . unresolved

We cannot modify `DocumentAnimations` here. You would need to queue up any changes to `update`, then apply those onto the "wider world" during `MaybeApplyPendingUpdate`. This is because `update` may discarded as a whole, e.g. if we're in one of several style resolution rounds for container queries.

David Awogbemila

I think this is effectively queuing the animation up for an update that will eventually be applied [at the same time](https://source.chromium.org/chromium/chromium/src/+/87c86aa78e1ab82adbf838ac4f31dba0d7996363:third_party/blink/renderer/core/css/post_style_update_scope.cc;l=82-88;bpv=0;bpt=0) as `MaybeApplyPendingUpdate`.

I think this function is trying to queue the animation up for `MaybeApplyPendingUpdate` if there is a change on the `animation` or `animation-trigger` style. If there is [no change](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/animation/css/css_animations.cc;l=1996-2002?q=calculateanimationupdate&ss=chromium%2Fchromium%2Fsrc) to `animation` or `animation-trigger` , it won't be queued for `MaybeApplyPendingUpdate`, but there might have been a change to `trigger-scope` *elsewhere* in the DOM that requires us to reconfigure the attachments. If it is a noop, the update attachments code should see that.

Open in Gerrit

Related details

Attention is currently required from:

Anders Hartvoll Ruud

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages