Bug 182100 – [Web Animations] Expose the reverse() method

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

[Antoine Quint]

Animations support a reverse() method to play animations backwards.

[Radar WebKit Bug Importer]

<rdar://problem/36867117>

[Antoine Quint]

Created attachment 332269 [details]
Patch

[Dean Jackson]

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.

[Antoine Quint]

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

[Antoine Quint]

Created attachment 332287 [details]
Patch for landing

[Antoine Quint]

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

[Antoine Quint]

Committed r227623: <https://trac.webkit.org/changeset/227623>