Bug 180647 – [Web Animations] Implement AnimationPlaybackEvent and AnimationPlaybackEventInit

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

Before we can get started implementing more of the APIs of Animation, we need to implement the animation class that animations can dispatch.

[Radar WebKit Bug Importer]

<rdar://problem/35966325>

[Antoine Quint]

Created attachment 328965 [details]
Patch

[Antoine Quint]

Created attachment 328973 [details]
Patch

[Dean Jackson]

Comment on attachment 328973 [details]
Patch

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

> Source/WebCore/animation/AnimationPlaybackEvent.cpp:46
> +    : Event(type, true, false)

What is the middle 'true' here? I guess it is a different constructor to the one you're using above?

> Source/WebCore/animation/AnimationPlaybackEvent.cpp:58
> +    if (m_currentTime)
> +        return m_currentTime->milliseconds();
> +    return std::nullopt;

We normally do these the other way around.

if (!m_currentTime)
  return std::nullopt;

return actualvalue;

> Source/WebCore/animation/AnimationPlaybackEvent.cpp:71
> +EventInterface AnimationPlaybackEvent::eventInterface() const
> +{
> +    return AnimationPlaybackEventInterfaceType;
> +}

This can be inline in the header file.

> Source/WebCore/animation/AnimationPlaybackEvent.h:40
> +    static Ref<AnimationPlaybackEvent> create(const AtomicString& type, const AnimationPlaybackEventInit& initializer, IsTrusted isTrusted = IsTrusted::No)

Not your issue, but I wish it was Trusted::NotTrusted, although Trusted::Trusted would look weird.

[Antoine Quint]

(In reply to Dean Jackson from comment #4)
> Comment on attachment 328973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328973&action=review
> 
> > Source/WebCore/animation/AnimationPlaybackEvent.cpp:46
> > +    : Event(type, true, false)
> 
> What is the middle 'true' here? I guess it is a different constructor to the
> one you're using above?

Yes, one is with the initialiser, this one is Event(const AtomicString& type, bool canBubble, bool cancelable). 

> > Source/WebCore/animation/AnimationPlaybackEvent.cpp:58
> > +    if (m_currentTime)
> > +        return m_currentTime->milliseconds();
> > +    return std::nullopt;
> 
> We normally do these the other way around.
> 
> if (!m_currentTime)
>   return std::nullopt;
> 
> return actualvalue;

Will fix.

> > Source/WebCore/animation/AnimationPlaybackEvent.cpp:71
> > +EventInterface AnimationPlaybackEvent::eventInterface() const
> > +{
> > +    return AnimationPlaybackEventInterfaceType;
> > +}
> 
> This can be inline in the header file.

Will fix.

[Antoine Quint]

Committed r225745: <https://trac.webkit.org/changeset/225745>

[Antoine Quint]

Committed r225749: <https://trac.webkit.org/changeset/225749>

[Antoine Quint]

Committed r225750: <https://trac.webkit.org/changeset/225750>