Bug 207629 – [Web Animations] Make all animation event types inherit from the same base class

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

[Web Animations] Make all animation event types inherit from the same base class

[Antoine Quint]

Created attachment 390515 [details]
Patch

[Antoine Quint]

Created attachment 390516 [details]
Patch

[Simon Fraser (smfr)]

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?

[Antoine Quint]

(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.

[Antoine Quint]

Created attachment 390748 [details]
Patch

[Antoine Quint]

Created attachment 390753 [details]
Patch

[Antoine Quint]

Created attachment 390754 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/59456167>

[Simon Fraser (smfr)]

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.

[Antoine Quint]

(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.

[Antoine Quint]

https://bugs.webkit.org/show_bug.cgi?id=207364