[webkit-reviews] review granted: [Bug 180830] [Web Animations] Implement the cancel() method on Animation : [Attachment 329387] 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 180830: [Web Animations] Implement the cancel() method on Animation
https://bugs.webkit.org/show_bug.cgi?id=180830

Attachment 329387: Patch

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




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

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

> Source/WebCore/animation/WebAnimation.cpp:342
> +	   // 2. Reject the current finished promise with a DOMException named
"AbortError".
> +	   m_finishedPromise.reject(Exception { AbortError });

Are you confirming that all these changes are covered by existing tests? I
don't see any changes in the expected results that are looking for this. If
not, you should be writing new tests.

> Source/WebCore/animation/WebAnimation.cpp:356
> +	   // 4. Create an AnimationPlaybackEvent, cancelEvent.
> +	   // 5. Set cancelEvent's type attribute to cancel.
> +	   // 6. Set cancelEvent's currentTime to null.
> +	   // 7. Let timeline time be the current time of the timeline with
which animation is associated. If animation is not associated with an
> +	   //	 active timeline, let timeline time be n unresolved time value.
> +	   // 8. Set cancelEvent's timelineTime to timeline time. If timeline
time is unresolved, set it to null.
> +	   // 9. If animation has a document for timing, then append
cancelEvent to its document for timing's pending animation event queue along
> +	   //	 with its target, animation. If animation is associated with an
active timeline that defines a procedure to convert timeline times
> +	   //	 to origin-relative time, let the scheduled event time be the
result of applying that procedure to timeline time. Otherwise, the
> +	   //	 scheduled event time is an unresolved time value.

Do you have bugs for all these missing steps?