Bug 180657 – [Web Animations] Enqueue and dispatch animation events

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

We added support for the AnimationPlaybackEvent interface in https://bugs.webkit.org/show_bug.cgi?id=180647. The next step towards dispatching the required event is to add a facility to enqueue and dispatch events as specified.

[Radar WebKit Bug Importer]

<rdar://problem/35970103>

[Antoine Quint]

Created attachment 328998 [details]
Patch

[Dean Jackson]

Comment on attachment 328998 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:232
> +    if (m_timeline && m_timeline->isDocumentTimeline())
> +        downcast<DocumentTimeline>(*m_timeline).enqueueAnimationPlaybackEvent(WTFMove(event));
> +    // Otherwise, queue a task to dispatch event at animation. The task source for this task is the DOM manipulation task source.
> +    else {

This indentation is weird. Put the comment inside the { }

[Chris Dumez]

Comment on attachment 328998 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.h:85
> +    ScriptExecutionContext* m_scriptExecutionContext;

What makes sure m_scriptExecutionContext is still alive when you use it? Such classes usually subclass ContextDestructionObserver or ActiveDOMObject so they are keep track of when the scriptExecutionContext is getting destroyed. In this case, since you fire events, I believe you want ActiveDOMObject and you want to make sure you hold an ActiveDOMObject::PendingActivity while you can still fire events. You also need to make sure you never fire events *after* ActiveDOMObject::stop() has been called.

[Antoine Quint]

Created attachment 329011 [details]
Patch

[Chris Dumez]

Comment on attachment 329011 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        dispatched along with DOM manipulations. In our implementation MutationObserver uses a

MutationOberver is supposed to use Microtasks as per :https://dom.spec.whatwg.org/#queueing-a-mutation-record:
- Queue a mutation observer compound microtask.

The Web Animation spec merely says:
Otherwise, queue a task to dispatch finishEvent at animation. The task source for this task is the DOM manipulation task source.

This looks to me like regular event scheduling, not a micro task. Why do we need to use a MicroTask? The DOM spec does not say that MutationObserver should schedule a task with DOM manipulation task source so it is not obvious to me that your explanation here is correct.

> Source/WebCore/animation/DocumentTimeline.cpp:222
> +static inline bool compareAnimationPlaybackEvents(const RefPtr<WebCore::AnimationPlaybackEvent> lhs, const RefPtr<WebCore::AnimationPlaybackEvent> rhs)

Shouldn't these be references?

> Source/WebCore/animation/DocumentTimeline.cpp:240
> +    auto pendingAnimationEvents = m_pendingAnimationEvents;

= WTFMove(m_pendingAnimationEvents) ? Then you don't need to clear() on the next line.

> Source/WebCore/animation/WebAnimation.cpp:244
> +    setPendingActivity(this);

Nowadays, we prefer calling makePendingActivity() and capture the pendingActivity in a lambda, as this is less error-prone.

> Source/WebCore/animation/WebAnimation.cpp:254
> +    unsetPendingActivity(this);

I don't think you want to do this here as this make cause |this| to get destroyed.

> Source/WebCore/animation/WebAnimation.cpp:322
> +    removeAllEventListeners();

You probably want to clear m_pendingAnimationEvents too. Note that Events ref their target so you have a cycle otherwise (WebAnimation refs its events via m_pendingAnimationEvents and the events ref the WebAnimation via their target).

> Source/WebCore/animation/WebAnimation.h:80
> +    WebAnimation(Document&);

explicit?

> Source/WebCore/animation/WebAnimation.h:90
> +    Vector<RefPtr<AnimationPlaybackEvent>> m_pendingAnimationEvents;

Can this be a Vector of Ref<>?

[Chris Dumez]

Comment on attachment 329011 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:54
>  {

You need to call suspendIfNeeded() in this constructor or you'll hit assertions.

[Antoine Quint]

Created attachment 329022 [details]
Patch

[Antoine Quint]

Created attachment 329023 [details]
Patch

[Chris Dumez]

Comment on attachment 329023 [details]
Patch

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

> Source/WebCore/animation/DocumentTimeline.cpp:222
> +static inline bool compareAnimationPlaybackEvents(const RefPtr<WebCore::AnimationPlaybackEvent> lhs, const RefPtr<WebCore::AnimationPlaybackEvent> rhs)

Why isn't this?
static inline bool compareAnimationPlaybackEvents(const RefPtr<WebCore::AnimationPlaybackEvent>& lhs, const RefPtr<WebCore::AnimationPlaybackEvent>& rhs);

Seems like it would do some refcounting churn otherwise.

> Source/WebCore/animation/DocumentTimeline.h:84
> +    Vector<RefPtr<AnimationPlaybackEvent>> m_pendingAnimationEvents;

Can this be a Vector of Ref<> ?

> Source/WebCore/animation/WebAnimation.cpp:225
> +    if (m_timeline && m_timeline->isDocumentTimeline()) {

We prefer if(is<DocumentTimeline>(m_timeline)) {

> Source/WebCore/animation/WebAnimation.cpp:235
> +        if (m_pendingAnimationEvents.size() == 1) {

Is it intentional that if you queue several events then you are going the spin the run loop only once and then fire all the events at once? Doesn't the spec mandate that each event is a scheduled task?

> Source/WebCore/animation/WebAnimation.cpp:237
> +            callOnMainThread([this, pendingActivity = WTFMove(pendingActivity)]() {

We usually just do [this, pendingActivity = makePendingActivity(*this)] here to avoid the unnecessary local.

> Source/WebCore/animation/WebAnimation.h:88
> +    Vector<RefPtr<AnimationPlaybackEvent>> m_pendingAnimationEvents;

Can this be a Vector of Ref<>?

> Source/WebCore/animation/WebAnimation.h:89
> +    bool m_isStopped;

bool m_isStopped { false };

[Antoine Quint]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 329023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329023&action=review
> 
> > Source/WebCore/animation/WebAnimation.cpp:235
> > +        if (m_pendingAnimationEvents.size() == 1) {
> 
> Is it intentional that if you queue several events then you are going the
> spin the run loop only once and then fire all the events at once? Doesn't
> the spec mandate that each event is a scheduled task?

You're right. They only should be dispatched all at once, and sorted, when queued on the document.

[Antoine Quint]

Created attachment 329033 [details]
Patch

[Chris Dumez]

Comment on attachment 329033 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:233
> +        m_pendingAnimationEvents.append(WTFMove(event));

It looks odd that you queue events in this m_pendingAnimationEvents but schedule a task for every event being appended. Then, when the task is executed, it fires all events queued since last event loop iteration. We may want to get rid of m_pendingAnimationEvents and capture the event in the lambda instead.

[Antoine Quint]

Created attachment 329090 [details]
Patch

[Antoine Quint]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 329033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329033&action=review
> 
> > Source/WebCore/animation/WebAnimation.cpp:233
> > +        m_pendingAnimationEvents.append(WTFMove(event));
> 
> It looks odd that you queue events in this m_pendingAnimationEvents but
> schedule a task for every event being appended. Then, when the task is
> executed, it fires all events queued since last event loop iteration. We may
> want to get rid of m_pendingAnimationEvents and capture the event in the
> lambda instead.

Of course, I was too focused on adopting makePendingActivity() that I forgot to update the logic. The new patch dispatches each event individually with a dedicated task.

[Chris Dumez]

Comment on attachment 329090 [details]
Patch

r=me

[Antoine Quint]

Committed r225790: <https://trac.webkit.org/changeset/225790>