Bug 207629 - [Web Animations] Make all animation event types inherit from the same base class
Summary: [Web Animations] Make all animation event types inherit from the same base class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks: 207364
  Show dependency treegraph
 
Reported: 2020-02-12 07:14 PST by Antoine Quint
Modified: 2020-02-14 08:37 PST (History)
9 users (show)

See Also:


Attachments
Patch (27.80 KB, patch)
2020-02-12 07:23 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (27.80 KB, patch)
2020-02-12 07:28 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (31.95 KB, patch)
2020-02-14 03:27 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (32.07 KB, patch)
2020-02-14 03:38 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (32.00 KB, patch)
2020-02-14 04:10 PST, Antoine Quint
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-02-12 07:14:34 PST
[Web Animations] Make all animation event types inherit from the same base class
Comment 1 Antoine Quint 2020-02-12 07:23:15 PST
Created attachment 390515 [details]
Patch
Comment 2 Antoine Quint 2020-02-12 07:28:16 PST
Created attachment 390516 [details]
Patch
Comment 3 Simon Fraser (smfr) 2020-02-12 09:46:21 PST
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?
Comment 4 Antoine Quint 2020-02-13 02:03:40 PST
(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.
Comment 5 Antoine Quint 2020-02-14 03:27:13 PST
Created attachment 390748 [details]
Patch
Comment 6 Antoine Quint 2020-02-14 03:38:48 PST
Created attachment 390753 [details]
Patch
Comment 7 Antoine Quint 2020-02-14 04:10:04 PST
Created attachment 390754 [details]
Patch
Comment 8 Radar WebKit Bug Importer 2020-02-14 04:49:01 PST
<rdar://problem/59456167>
Comment 9 Simon Fraser (smfr) 2020-02-14 08:26:50 PST
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.
Comment 10 Antoine Quint 2020-02-14 08:30:37 PST
(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.
Comment 11 Antoine Quint 2020-02-14 08:37:01 PST
https://bugs.webkit.org/show_bug.cgi?id=207364