[animation-trigger] Enforce single trigger per animation [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
const HeapHashSet<WeakMember<AnimationTrigger>>& GetTriggers();nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTriggers' to 'Triggers'.
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)_
nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTriggers' to 'Triggers'.
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Awogbemila
| Commit-Queue | +1 |
2 comments
const HeapHashSet<WeakMember<AnimationTrigger>>& GetTriggers();nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTriggers' to 'Triggers'.
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)_
Acknowledged. Pre-existing function.
nit: Blink Style Guide: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTriggers' to 'Triggers'.
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)_
Acknowledged. Pre-existing function.
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 4 comments![]( "Open in Gerrit")
// TODO(crbug.com/474398437): Support multiple triggers per animation.Please also link the CSSWG issue where we resolved to only support a single trigger for now. Currently it looks like a WIP feature rather than a limitation in accordance with the spec.
test_for_n_triggers(3);I'm confused here. Looks like we are supporting multiple triggers here, whereas the CL for for enforcing a single trigger.
test_invalid_value('animation-trigger', '--abc play --abc play');Can we add a note here pointing to the issue indicating that we intent to support multiple triggers in the future, but for now are limited to one.
test_computed_value('animation-trigger', '--abc play --abc play');Should we keep these with updated expectations rather than remove them. Perhaps with an added comment about supporting multiple triggers in the future.
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 4 comments![]( "Open in Gerrit")
// TODO(crbug.com/474398437): Support multiple triggers per animation.Please also link the CSSWG issue where we resolved to only support a single trigger for now. Currently it looks like a WIP feature rather than a limitation in accordance with the spec.
Done
test_for_n_triggers(3);I'm confused here. Looks like we are supporting multiple triggers here, whereas the CL for for enforcing a single trigger.
This test is using multiple animations (separated by commas) but still only a single trigger (also separated by commas) per animation. (Multiple triggers attached to a single animation would be separated by spaces)
test_invalid_value('animation-trigger', '--abc play --abc play');Can we add a note here pointing to the issue indicating that we intent to support multiple triggers in the future, but for now are limited to one.
Done
test_computed_value('animation-trigger', '--abc play --abc play');Should we keep these with updated expectations rather than remove them. Perhaps with an added comment about supporting multiple triggers in the future.
Yeah they are modified to `test_invalid_value` cases below.
Related details
- Kevin Ellis
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. |
Kevin Ellis (Gerrit)
Kevin Ellis voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kevin Ellis
| Code-Review | +1 |
3 comments
test_for_n_triggers(3);David AwogbemilaI'm confused here. Looks like we are supporting multiple triggers here, whereas the CL for for enforcing a single trigger.
This test is using multiple animations (separated by commas) but still only a single trigger (also separated by commas) per animation. (Multiple triggers attached to a single animation would be separated by spaces)
Optional nits:
- s/single/single-animation s/multiple/multiple-animations.
- Add comment to test_for_n_triggers to clarify that each trigger is for a separate animation.
test_computed_value('animation-trigger', '--abc play --abc play');David AwogbemilaShould we keep these with updated expectations rather than remove them. Perhaps with an added comment about supporting multiple triggers in the future.
Yeah they are modified to `test_invalid_value` cases below.
- 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
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
test_for_n_triggers(3);David AwogbemilaI'm confused here. Looks like we are supporting multiple triggers here, whereas the CL for for enforcing a single trigger.
Kevin EllisThis test is using multiple animations (separated by commas) but still only a single trigger (also separated by commas) per animation. (Multiple triggers attached to a single animation would be separated by spaces)
Optional nits:
- s/single/single-animation s/multiple/multiple-animations.
- Add comment to test_for_n_triggers to clarify that each trigger is for a separate animation.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/57166.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Awogbemila (Gerrit)
David Awogbemila voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |