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.
<rdar://problem/35970103>
Created attachment 328998 [details] Patch
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 { }
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.
Created attachment 329011 [details] Patch
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<>?
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.
Created attachment 329022 [details] Patch
Created attachment 329023 [details] Patch
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 };
(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.
Created attachment 329033 [details] Patch
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.
Created attachment 329090 [details] Patch
(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.
Comment on attachment 329090 [details] Patch r=me
Committed r225790: <https://trac.webkit.org/changeset/225790>