[webkit-reviews] review granted: [Bug 202190] [Web Animations] Implement replaced animations : [Attachment 380002] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 202190: [Web Animations] Implement replaced animations
https://bugs.webkit.org/show_bug.cgi?id=202190

Attachment 380002: Patch

https://bugs.webkit.org/attachment.cgi?id=380002&action=review




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 380002
  --> https://bugs.webkit.org/attachment.cgi?id=380002
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380002&action=review

> Source/WebCore/animation/AnimationTimeline.cpp:195
> +	       Vector<RefPtr<WebAnimation>> sortedWebAnimations;

Initialize the vector with the size of webAnimations.size()

> Source/WebCore/animation/DocumentTimeline.cpp:450
> +    auto animations = animationsForElement(*target,
AnimationTimeline::Ordering::Sorted);
> +    for (auto& animationWithHigherCompositeOrder :
WTF::makeReversedRange(animations)) {

If you always access them in this reversed manner, why not have
Ordering::SortedDescending or something like that, to avoid sorting then
reversing?

> Source/WebCore/animation/DocumentTimeline.cpp:476
> +    for (const auto& animation : m_allAnimations) {

Do you need the const here?

> Source/WebCore/animation/DocumentTimeline.cpp:480
> +	       animation->setReplaceState(WebAnimation::ReplaceState::Removed);

Also, this looks like a non-const method?

> Source/WebCore/animation/DocumentTimeline.cpp:488
> +	       // 6. If animation has a document for timing, then append
removeEvent to its document for timing's pending animation
> +	       //    event queue along with its target, animation. For the
scheduled event time, use the result of applying the procedure
> +	       //    to convert timeline time to origin-relative time to the
current time of the timeline with which animation is associated.
> +	       //    Otherwise, queue a task to dispatch removeEvent at
animation. The task source for this task is the DOM manipulation task source.

It's not clear you're doing this. What does queue a task to dispatch
removeEvent at animation mean?