[animation-trigger] Update attachments when trigger-scope changes [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 3 comments![]( "Open in Gerrit")
RelevantTriggerAttachmentMap& relevant_attachments);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)_
RelevantTriggerAttachmentMap& relevant_attachments) {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)_
// obsolete add should be removed. However, don't call removeAnimation justnit: 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)_
Related details
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. |
David Awogbemila (Gerrit)
David Awogbemila added 3 comments![]( "Open in Gerrit")
RelevantTriggerAttachmentMap& relevant_attachments);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)_
Done
RelevantTriggerAttachmentMap& relevant_attachments) {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)_
Done
// obsolete add should be removed. However, don't call removeAnimation justnit: 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)_
Done
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
if (existing_trigger_attachments || trigger_attachments) {
element.GetDocument()
.GetDocumentAnimations()
.AddPendingTriggerAttachmentUpdate(animation);
}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.
Related details
- David Awogbemila
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. |
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
if (existing_trigger_attachments || trigger_attachments) {
element.GetDocument()
.GetDocumentAnimations()
.AddPendingTriggerAttachmentUpdate(animation);
}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.
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.
Related details
- Anders Hartvoll Ruud
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 1 comment![]( "Open in Gerrit")
if (existing_trigger_attachments || trigger_attachments) {
element.GetDocument()
.GetDocumentAnimations()
.AddPendingTriggerAttachmentUpdate(animation);
}David AwogbemilaWe 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.
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.
We still need to queue those changes on `CSSAnimationUpdate`. We need to be able to easily change our minds about all the effects of `CalculateAnimationUpdate`.
Related details
- David Awogbemila
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. |
David Awogbemila (Gerrit)
David Awogbemila added 1 comment![]( "Open in Gerrit")
if (existing_trigger_attachments || trigger_attachments) {
element.GetDocument()
.GetDocumentAnimations()
.AddPendingTriggerAttachmentUpdate(animation);
}David AwogbemilaWe 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.
Anders Hartvoll RuudI 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.
We still need to queue those changes on `CSSAnimationUpdate`. We need to be able to easily change our minds about all the effects of `CalculateAnimationUpdate`.
Done. An earlier cl in the chain, crrev.com/c/7265794, now takes care of tracking trigger attachment changes by passing through `MaybeApplyPendingUpdates`.
(Apologies for the web of cls :))
Related details
- Anders Hartvoll Ruud
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |