[webkit-reviews] review granted: [Bug 207364] [Web Animations] Ensure CSS Transition and CSS Animation events are queued, sorted and dispatched by their timeline : [Attachment 390760] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 207364: [Web Animations] Ensure CSS Transition and CSS Animation events are
queued, sorted and dispatched by their timeline
https://bugs.webkit.org/show_bug.cgi?id=207364

Attachment 390760: Patch

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




--- Comment #12 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 390760
  --> https://bugs.webkit.org/attachment.cgi?id=390760
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:394
> +    // enqueueAnimationEvent() calls scheduleAnimationResolution() to ensure
that the "update animations and send events"
> +    // procedure is run and enqueued events are dispatched in the next
frame. However, events that are enqueued while
> +    // this procedure is running should not schedule animation resolution
until the event queue has been cleared.
> +    m_shouldScheduleAnimationResolutionForNewPendingEvents = false;

This is a bit confusing because I'd expect scheduleAnimationResolution() to
queue up something that is going to happen later, which makes me wonder why
calling it in this block is bad. What's the timing difference between calling
scheduleAnimationResolution() here, and calling it later via
scheduleNextTick()?

> Source/WebCore/animation/DocumentTimeline.cpp:728
> +    String eventType = event.type().string();

eventType is unused

> Source/WebCore/animation/WebAnimation.cpp:1206
> +    return pending() || playState() == PlayState::Running ||
m_hasScheduledEventsDuringTick;

Why does the WebAnimation have to stick around until the event is dispatched?
Does the event need to reference back to the WebAnimation? Or do we need to be
able to unschedule the event sometimes?

> LayoutTests/imported/w3c/ChangeLog:16
> +	   However, in order to not regress our WPT score, the issue of "style
change" events will be addressed in a follow-up patch.

We should file GitHub issues on tests we think aren't cross-browser compatible.