Bug 180647 - [Web Animations] Implement AnimationPlaybackEvent and AnimationPlaybackEventInit
Summary: [Web Animations] Implement AnimationPlaybackEvent and AnimationPlaybackEventInit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-11 06:19 PST by Antoine Quint
Modified: 2017-12-11 11:19 PST (History)
2 users (show)

See Also:


Attachments
Patch (24.18 KB, patch)
2017-12-11 06:29 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (24.21 KB, patch)
2017-12-11 08:00 PST, Antoine Quint
dino: 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 2017-12-11 06:19:26 PST
Before we can get started implementing more of the APIs of Animation, we need to implement the animation class that animations can dispatch.
Comment 1 Radar WebKit Bug Importer 2017-12-11 06:19:59 PST
<rdar://problem/35966325>
Comment 2 Antoine Quint 2017-12-11 06:29:48 PST
Created attachment 328965 [details]
Patch
Comment 3 Antoine Quint 2017-12-11 08:00:56 PST
Created attachment 328973 [details]
Patch
Comment 4 Dean Jackson 2017-12-11 09:53:14 PST
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.
Comment 5 Antoine Quint 2017-12-11 09:58:27 PST
(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.
Comment 6 Antoine Quint 2017-12-11 10:02:45 PST
Committed r225745: <https://trac.webkit.org/changeset/225745>
Comment 7 Antoine Quint 2017-12-11 10:57:03 PST
Committed r225749: <https://trac.webkit.org/changeset/225749>
Comment 8 Antoine Quint 2017-12-11 11:19:42 PST
Committed r225750: <https://trac.webkit.org/changeset/225750>