[animation-trigger] Make TimelineTrigger support multiple timelines [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
AnimationTimeline* GetTimelineInternal() {nit: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTimelineInternal' to 'TimelineInternal' as there is no naming conflict with a type. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
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)_
AnimationTimeline* GetTimelineInternal() { return timeline_.Get(); }nit: Precede setters with the word “Set”; use bare words for getters. Consider renaming 'GetTimelineInternal' to 'TimelineInternal' as there is no naming conflict with a type. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
TimelineTrigger(const Member<TimelineTriggerRanges>& ranges,nit: Prefer raw pointers for function parameters. `Member<T>` is intended for class member variables. Use `TimelineTriggerRanges*` instead of `const Member<TimelineTriggerRanges>&`.
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)_
TimelineTrigger::TimelineTrigger(const Member<TimelineTriggerRanges>& ranges,nit: Prefer raw pointers for function parameters. `Member<T>` is intended for class member variables. Use `TimelineTriggerRanges*` instead of `const Member<TimelineTriggerRanges>&`.
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
TimelineTrigger(const Member<TimelineTriggerRanges>& ranges,nit: Prefer raw pointers for function parameters. `Member<T>` is intended for class member variables. Use `TimelineTriggerRanges*` instead of `const Member<TimelineTriggerRanges>&`.
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; will consider renaming in future patch.
TimelineTrigger::TimelineTrigger(const Member<TimelineTriggerRanges>& ranges,nit: Prefer raw pointers for function parameters. `Member<T>` is intended for class member variables. Use `TimelineTriggerRanges*` instead of `const Member<TimelineTriggerRanges>&`.
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. Will fix.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack added 2 comments![]( "Open in Gerrit")
(I'll delete this from the description: timeline_trigger_range.cc is empty in this patch because I left the implementation of TimelineTriggerRange in timeline_trigger.cc as that may make it easier to see what code is actually changing; I had originally separated the implementation in https://chromium-review.googlesource.com/c/chromium/src/+/7365245)Thanks, please move the implementation over for landing. You can always leave a note like this as a file comment on the file.
] interface TimelineTriggerRanges {We tend to not pluralist type names - maybe call this TimelineTriggerRangeList similar to NodeList.
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. |
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
getter TimelineTriggerRange item(unsigned long index);Also add `iterable<TimelineTriggerRange>;` as we should be able to iterate through the ranges in this type.
David Awogbemila (Gerrit)
David Awogbemila added 4 comments![]( "Open in Gerrit")
(I'll delete this from the description: timeline_trigger_range.cc is empty in this patch because I left the implementation of TimelineTriggerRange in timeline_trigger.cc as that may make it easier to see what code is actually changing; I had originally separated the implementation in https://chromium-review.googlesource.com/c/chromium/src/+/7365245)Thanks, please move the implementation over for landing. You can always leave a note like this as a file comment on the file.
Ack. Will do.
We tend to not pluralist type names - maybe call this TimelineTriggerRangeList similar to NodeList.
Sounds good. Done.
Also add `iterable<TimelineTriggerRange>;` as we should be able to iterate through the ranges in this type.
Done
namespace blink {} // namespace blinkFor ease of review, the implementation of
TimelineTriggerRange is left in timeline_trigger.cc as it is mostly a copy
of code that was previously implemented in TimelineTrigger. The code will
be moved here before landing.
Related details
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Flack (Gerrit)
Robert Flack voted Code-Review+1![]( "Open in Gerrit")
- David Awogbemila
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![]( "Open in Gerrit")
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/57110.
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. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[animation-trigger] Make TimelineTrigger support multiple timelines
We got a resolution[1] to future-proof the TimelineTrigger interface
towards supporting multiple timelines. This cl makes this change.
(I'll delete this from the description: timeline_trigger_range.cc is empty in this patch because I left the implementation of TimelineTriggerRange in timeline_trigger.cc as that may make it easier to see what code is actually changing; I had originally separated the implementation in https://chromium-review.googlesource.com/c/chromium/src/+/7365245)
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/core/animation/BUILD.gn
- M third_party/blink/renderer/core/animation/animation_test.cc
- M third_party/blink/renderer/core/animation/animation_trigger_test.cc
- M third_party/blink/renderer/core/animation/css/css_animations.cc
- M third_party/blink/renderer/core/animation/css/css_animations_test.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.cc
- M third_party/blink/renderer/core/animation/timeline_trigger.h
- M third_party/blink/renderer/core/animation/timeline_trigger.idl
- A third_party/blink/renderer/core/animation/timeline_trigger_range.cc
- A third_party/blink/renderer/core/animation/timeline_trigger_range.h
- A third_party/blink/renderer/core/animation/timeline_trigger_range_list.cc
- A third_party/blink/renderer/core/animation/timeline_trigger_range_list.h
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-addAnimation.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-disarmed-by-apis.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-multiple-animations.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-multiple-triggers.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/animation-trigger-range-px-roundtrip.tentative.html
- M third_party/blink/web_tests/external/wpt/scroll-animations/animation-trigger/support/support.js
- M third_party/blink/web_tests/external/wpt/web-animations/interfaces/TimelineTrigger/constructor.html
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Robert Flack, +1 by David Awogbemila
| 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")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57110
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |