Bug 207629

Created attachment 390515 [details] Patch

Created attachment 390516 [details] Patch

Comment on attachment 390516 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390516&action=review > Source/WebCore/animation/AnimationEventBase.h:56 > + RefPtr<WebAnimation> m_animation; This tingles my "ref cycle" spidey sense. > Source/WebCore/animation/DeclarativeAnimation.cpp:355 > + Optional<Seconds> timelineTime = timeline() ? timeline()->currentTime() : WTF::nullopt; auto > Source/WebCore/animation/DeclarativeAnimation.cpp:364 > + if (is<CSSAnimation>(this)) { > + auto event = AnimationEvent::create(eventType, downcast<CSSAnimation>(this)->animationName(), time, pseudoId, timelineTime, this); > + event->setTarget(m_owningElement); > + m_eventQueue->enqueueEvent(WTFMove(event)); > + } else if (is<CSSTransition>(this)) { > + auto event = TransitionEvent::create(eventType, downcast<CSSTransition>(this)->transitionProperty(), time, pseudoId, timelineTime, this); > + event->setTarget(m_owningElement); > + m_eventQueue->enqueueEvent(WTFMove(event)); > + } This is what virtual functions are for. Why the is<>(this) here?

(In reply to Simon Fraser (smfr) from comment #3) > Comment on attachment 390516 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390516&action=review > > > Source/WebCore/animation/AnimationEventBase.h:56 > > + RefPtr<WebAnimation> m_animation; > > This tingles my "ref cycle" spidey sense. So, for a DeclarativeAnimation, as it stands, events will be enqueued on a queue owned by a DeclarativeAnimation, and those events will maintain a strong reference to the DeclarativeAnimation until dispatched. This is a temporary cycle. When we will dispatch those events on the DocumentTimeline's queue, there will be a temporary cycle as well, DocumentTimeline > AnimationEventBase > WebAnimation > DocumentTimeline. > > Source/WebCore/animation/DeclarativeAnimation.cpp:355 > > + Optional<Seconds> timelineTime = timeline() ? timeline()->currentTime() : WTF::nullopt; > > auto Will fix. > > Source/WebCore/animation/DeclarativeAnimation.cpp:364 > > + if (is<CSSAnimation>(this)) { > > + auto event = AnimationEvent::create(eventType, downcast<CSSAnimation>(this)->animationName(), time, pseudoId, timelineTime, this); > > + event->setTarget(m_owningElement); > > + m_eventQueue->enqueueEvent(WTFMove(event)); > > + } else if (is<CSSTransition>(this)) { > > + auto event = TransitionEvent::create(eventType, downcast<CSSTransition>(this)->transitionProperty(), time, pseudoId, timelineTime, this); > > + event->setTarget(m_owningElement); > > + m_eventQueue->enqueueEvent(WTFMove(event)); > > + } > > This is what virtual functions are for. Why the is<>(this) here? Right, I'll add a new virtual function on DeclarativeAnimation to generate the right event.

Created attachment 390748 [details] Patch

Created attachment 390753 [details] Patch

Created attachment 390754 [details] Patch

<rdar://problem/59456167>

Comment on attachment 390754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390754&action=review very nice > Source/WebCore/animation/CSSAnimation.h:52 > + Ref<AnimationEventBase> createEvent(const AtomString& eventType, double elapsedTime, const String& pseudoId, Optional<Seconds> timelineTime) final; Maybe a comment somewhere to mention how elapsedTime and timelineTime differ.

(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 390754 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390754&action=review > > very nice > > > Source/WebCore/animation/CSSAnimation.h:52 > > + Ref<AnimationEventBase> createEvent(const AtomString& eventType, double elapsedTime, const String& pseudoId, Optional<Seconds> timelineTime) final; > > Maybe a comment somewhere to mention how elapsedTime and timelineTime differ. Sure, adding this in DeclarativeAnimation.h where the virtual method is specified: // elapsedTime is the animation's current time at the time the event is added and is exposed through the DOM API, timelineTime is the animations' // timeline current time and is not exposed through the DOM API but used by the DocumentTimeline for sorting events before dispatch.