[webkit-reviews] review granted: [Bug 182100] [Web Animations] Expose the reverse() method : [Attachment 332269] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 182100: [Web Animations] Expose the reverse() method
https://bugs.webkit.org/show_bug.cgi?id=182100

Attachment 332269: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 332269
  --> https://bugs.webkit.org/attachment.cgi?id=332269
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.