Bug 182100 - [Web Animations] Expose the reverse() method
Summary: [Web Animations] Expose the reverse() method
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: 2018-01-25 07:13 PST by Antoine Quint
Modified: 2018-01-25 12:23 PST (History)
7 users (show)

See Also:


Attachments
Patch (14.53 KB, patch)
2018-01-25 08:21 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (6.15 KB, patch)
2018-01-25 10:44 PST, Antoine Quint
graouts: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-01-25 07:13:14 PST
Animations support a reverse() method to play animations backwards.
Comment 1 Radar WebKit Bug Importer 2018-01-25 07:13:32 PST
<rdar://problem/36867117>
Comment 2 Antoine Quint 2018-01-25 08:21:00 PST
Created attachment 332269 [details]
Patch
Comment 3 Dean Jackson 2018-01-25 09:32:57 PST
Comment on attachment 332269 [details]
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:296
>  void WebAnimation::setPlaybackRate(double newPlaybackRate)
>  {
> +    setPlaybackRate(newPlaybackRate, Silently::No);
> +}
> +
> +void WebAnimation::setPlaybackRate(double newPlaybackRate, Silently silently)

Why isn't this a single method with a default parameter?

> Source/WebCore/animation/WebAnimation.cpp:304
> +    // The procedure to set the animation playback rate of an animation, animation to new playback rate is as follows.

animation, animation huh?

> Source/WebCore/animation/WebAnimation.cpp:305
> +    // The procedure to silently set the animation playback rate of animation, animation to new playback rate is identical

huh again?

> Source/WebCore/animation/WebAnimation.cpp:306
> +    // to the above procedure except that rather than invoking the procedure to set the current time in the final step,

procedure procedure procedure

> Source/WebCore/animation/WebAnimation.cpp:775
> +    // The procedure to reverse an animation of animation animation is as follows:

animation animation

> Source/WebCore/animation/WebAnimation.cpp:796
> +        return playResult.releaseException();

Are you supposed to throw the exception here? I wonder why you restore the forward rate before throwing.

> Source/WebCore/animation/WebAnimation.h:118
> +    void setPlaybackRate(double, Silently);

See note above.
Comment 4 Antoine Quint 2018-01-25 10:24:10 PST
(In reply to Dean Jackson from comment #3)
> Comment on attachment 332269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332269&action=review
> 
> > Source/WebCore/animation/WebAnimation.cpp:296
> >  void WebAnimation::setPlaybackRate(double newPlaybackRate)
> >  {
> > +    setPlaybackRate(newPlaybackRate, Silently::No);
> > +}
> > +
> > +void WebAnimation::setPlaybackRate(double newPlaybackRate, Silently silently)
> 
> Why isn't this a single method with a default parameter?

Because the single parameter method is a method that is exposed via bindings. But maybe that's OK?

> > Source/WebCore/animation/WebAnimation.cpp:304
> > +    // The procedure to set the animation playback rate of an animation, animation to new playback rate is as follows.
> 
> animation, animation huh?

I should put quote around the second "animation", it's in italics in the spec to clarify it is the name of the animation that is being referred to.

> > Source/WebCore/animation/WebAnimation.cpp:796
> > +        return playResult.releaseException();
> 
> Are you supposed to throw the exception here? I wonder why you restore the
> forward rate before throwing.

I will raise an issue to clarify, but the WPT tests expect the exception to be forwarded. The comments explain that we need to reset if calling play() raises an exception.
Comment 5 Antoine Quint 2018-01-25 10:44:11 PST
Created attachment 332287 [details]
Patch for landing
Comment 6 Antoine Quint 2018-01-25 10:54:50 PST
(In reply to Antoine Quint from comment #4)
> (In reply to Dean Jackson from comment #3)
> > Comment on attachment 332269 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=332269&action=review
> > 
> > > Source/WebCore/animation/WebAnimation.cpp:296
> > >  void WebAnimation::setPlaybackRate(double newPlaybackRate)
> > >  {
> > > +    setPlaybackRate(newPlaybackRate, Silently::No);
> > > +}
> > > +
> > > +void WebAnimation::setPlaybackRate(double newPlaybackRate, Silently silently)
> > 
> > Why isn't this a single method with a default parameter?
> 
> Because the single parameter method is a method that is exposed via
> bindings. But maybe that's OK?

It is OK! I fixed in the landing patch.

> > > Source/WebCore/animation/WebAnimation.cpp:796
> > > +        return playResult.releaseException();
> > 
> > Are you supposed to throw the exception here? I wonder why you restore the
> > forward rate before throwing.
> 
> I will raise an issue to clarify, but the WPT tests expect the exception to
> be forwarded. The comments explain that we need to reset if calling play()
> raises an exception.

See https://github.com/w3c/csswg-drafts/issues/2226.
Comment 7 Antoine Quint 2018-01-25 12:23:50 PST
Committed r227623: <https://trac.webkit.org/changeset/227623>