[webkit-reviews] review granted: [Bug 178988] [Web Animations] Expose the currentTime property on Animation : [Attachment 325287] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 29 12:20:18 PDT 2017


Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 178988: [Web Animations] Expose the currentTime property on Animation
https://bugs.webkit.org/show_bug.cgi?id=178988

Attachment 325287: Patch

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




--- Comment #2 from Dean Jackson <dino at apple.com> ---
Comment on attachment 325287
  --> https://bugs.webkit.org/attachment.cgi?id=325287
Patch

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

> Source/WebCore/animation/WebAnimation.cpp:94
> +    if (time == std::nullopt)
> +	   return std::nullopt;
> +    return time->value();

If you want to test for the optional having a value you can just do

if (!time)
  return std::nullopt;

But in this case there is a helper to make it even easier for you:

return time.value_or(std::nullopt);

> Source/WebCore/animation/WebAnimation.cpp:100
> +    if (currentTime == std::nullopt)
> +	   return Exception { TypeError };

if (!currentTime)

> Source/WebCore/animation/WebAnimation.cpp:110
> +    if (!m_timeline || m_startTime == std::nullopt)
> +	   return std::nullopt;

if (!m_timeline || !m_startTime)

> Source/WebCore/animation/WebAnimation.cpp:113
> +    if (timelineTime == std::nullopt)

if (!timelineTime)

> Source/WebCore/animation/WebAnimation.cpp:129
> +    std::optional<Seconds> timelineTime = m_timeline->currentTime();

auto timelineTime = m_timeline->currentTime();

> Source/WebCore/animation/WebAnimation.cpp:130
> +    if (timelineTime == std::nullopt) {

if (!timelineTime)

> LayoutTests/ChangeLog:11
> +	   Add a new test that checks that the currentTime property is set
> +	   correctly based on the startTime value and the document timeline
> +	   currentTime, and that setting the property may raise an exception
> +	   and otherwise update the animation startTime.

If this is tested by the existing WPT tests, then you'd be better off importing
them with expected results that show a lot of failures. Then, as you implement,
you'll slowly change those failures into passes.

Then again, since you need to go through Internals, maybe there is no way to
test this using WPT.


More information about the webkit-reviews mailing list