[animation-trigger] Avoid uncancelable side-effect in ResolveStyle [chromium/src : main]
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud added 5 comments![]( "Open in Gerrit")
we shouldn't introduced uncancelable side-effects in ResolveStyle.nit: introduce
// Elements which declare named animation triggers, i.e. timeline-trigger.
HeapHashSet<Member<Element>> elements_with_pending_trigger_updates_;Why not add a bool on `CSSAnimationUpdate` (`needs_trigger_update`, or something) instead? All other pending updates are described there.
You could then apply the trigger updates during `MaybeApplyPendingUpdate` as normal?
void StyleResolver::ApplyTriggerData(StyleResolverState& state) {Why does this not happen in `StyleResolver::ApplyAnimatedStyle`? Every other `CSSAnimationData::CalculateFooUpdate` already happens there.
const ComputedStyle* old_style = state.GetElement().GetComputedStyle();This is not the correct old style. (Use `state.OldStyle()`.)
PostStyleUpdateScope::CurrentAnimationData()->SetPendingTriggerUpdate(
state.GetElement(), state.AnimationUpdate());It's unfortunate to add a second place where this update object it set. The existing call to `SetAnimationUpdateIfNeeded` should be enough? The `CSSAnimationUpdate` we set here will also be overwritten by `SetAnimationUpdateIfNeeded`.
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 3 comments![]( "Open in Gerrit")
Haven't made any changes. Looking to explore partial re-purposing of `CSSAnimationUpdate`.
we shouldn't introduced uncancelable side-effects in ResolveStyle.David Awogbemilanit: introduce
Done
// Elements which declare named animation triggers, i.e. timeline-trigger.
HeapHashSet<Member<Element>> elements_with_pending_trigger_updates_;Why not add a bool on `CSSAnimationUpdate` (`needs_trigger_update`, or something) instead? All other pending updates are described there.
You could then apply the trigger updates during `MaybeApplyPendingUpdate` as normal?
I can look into this. I got the sense that `CSSAnimationUpdate` was trying to track changes to the `animation` property whereas the element declaring `timeline-trigger` might have no animation at all. Still I it might be possible to use `CSSAnimationUpdate` in a way that makes sense so I'll explore that.
(I guess this is relevant to your other comments)
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")
// Elements which declare named animation triggers, i.e. timeline-trigger.
HeapHashSet<Member<Element>> elements_with_pending_trigger_updates_;David AwogbemilaWhy not add a bool on `CSSAnimationUpdate` (`needs_trigger_update`, or something) instead? All other pending updates are described there.
You could then apply the trigger updates during `MaybeApplyPendingUpdate` as normal?
I can look into this. I got the sense that `CSSAnimationUpdate` was trying to track changes to the `animation` property whereas the element declaring `timeline-trigger` might have no animation at all. Still I it might be possible to use `CSSAnimationUpdate` in a way that makes sense so I'll explore that.
(I guess this is relevant to your other comments)
Right. At least it's current (pre-trigger) purposes is to capture any change to the "stateful animations machinery" that results from a style resolution. That could be transitions, animations, or timelines.
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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
const ScopedCSSName* GetScopedName() const { return name_.Get(); }nit: Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters. Please rename 'GetScopedName' to 'ScopedName' (or 'ScopedCSSName') to follow the bare word convention for getters, as it does not conflict with the type name.
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 abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
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 4 comments![]( "Open in Gerrit")
// Elements which declare named animation triggers, i.e. timeline-trigger.
HeapHashSet<Member<Element>> elements_with_pending_trigger_updates_;David AwogbemilaWhy not add a bool on `CSSAnimationUpdate` (`needs_trigger_update`, or something) instead? All other pending updates are described there.
You could then apply the trigger updates during `MaybeApplyPendingUpdate` as normal?
Anders Hartvoll RuudI can look into this. I got the sense that `CSSAnimationUpdate` was trying to track changes to the `animation` property whereas the element declaring `timeline-trigger` might have no animation at all. Still I it might be possible to use `CSSAnimationUpdate` in a way that makes sense so I'll explore that.
(I guess this is relevant to your other comments)
Right. At least it's current (pre-trigger) purposes is to capture any change to the "stateful animations machinery" that results from a style resolution. That could be transitions, animations, or timelines.
Done
void StyleResolver::ApplyTriggerData(StyleResolverState& state) {David AwogbemilaWhy does this not happen in `StyleResolver::ApplyAnimatedStyle`? Every other `CSSAnimationData::CalculateFooUpdate` already happens there.
Done
const ComputedStyle* old_style = state.GetElement().GetComputedStyle();David AwogbemilaThis is not the correct old style. (Use `state.OldStyle()`.)
Acknowledged
PostStyleUpdateScope::CurrentAnimationData()->SetPendingTriggerUpdate(
state.GetElement(), state.AnimationUpdate());David AwogbemilaIt's unfortunate to add a second place where this update object it set. The existing call to `SetAnimationUpdateIfNeeded` should be enough? The `CSSAnimationUpdate` we set here will also be overwritten by `SetAnimationUpdateIfNeeded`.
Acknowledged
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
That's true, but it's not a good place to _calculate_ those changes. We'll ultimately need to split up the "calculate what to do" part of `UpdateNamedTriggers` and the "apply what we previously calculated" part of `UpdateNamedTriggers`, perform the former during `ResolveStyle`, and perform the latter during `MaybeApplyPendingUpdate`.
So the question is: Is this CL valuable as a step towards that? I would only land this if the answer to that question is "yes". Otherwise, we can probably live with the side-effects for a bit?
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Awogbemila
| Commit-Queue | +1 |
That's true, but it's not a good place to _calculate_ those changes. We'll ultimately need to split up the "calculate what to do" part of `UpdateNamedTriggers` and the "apply what we previously calculated" part of `UpdateNamedTriggers`, perform the former during `ResolveStyle`, and perform the latter during `MaybeApplyPendingUpdate`.
So the question is: Is this CL valuable as a step towards that? I would only land this if the answer to that question is "yes". Otherwise, we can probably live with the side-effects for a bit?
Yeah I believe it is a step towards that. I've now incorporated some of the [changes](https://chromium-review.googlesource.com/c/chromium/src/+/7255899/6..7) (between ps6 & 7) that I had initially deferred to crrev.com/c/7237813. This makes it easier to see that `UpdateNamedTriggers` now does less work.
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. |