[webkit-reviews] review granted: [Bug 178526] [Web Animations] Provide basic timeline and animation interfaces : [Attachment 324255] 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 178526: [Web Animations] Provide basic timeline and animation interfaces
https://bugs.webkit.org/show_bug.cgi?id=178526

Attachment 324255: Patch

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




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

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

> Source/WebCore/animation/AnimationTimeline.idl:29
> +    CustomToJSObject,

Why do you need a custom toJS? There is only a DocumentTimeline at the moment
right? I guess you need this for inheritance, even if no one creates one of
these yet.

> Source/WebCore/animation/WebAnimation.cpp:48
> +    if (m_timeline)
> +	   m_timeline->removeAnimation(*this);

Where do you ever add the animation to the timeline? Shouldn't the constructor
above do that?

> Source/WebCore/animation/WebAnimation.idl:28
> +    Conditional=WEB_ANIMATIONS,
> +    EnabledAtRuntime=WebAnimations,

I wonder if we need both. I think maybe just the runtime feature is enough. I
expect we are planning to ship this everywhere, so no real need to compile it
out.

> Source/WebCore/animation/WebAnimation.idl:29
> +    InterfaceName=Animation,

Why are you calling it Animation rather than WebAnimation?

> LayoutTests/ChangeLog:14
> +	   Basic test coverage to check that we are exposing a DocumentTimeline
instance on
> +	   the Document and that we can construct Animations, optionally
associated with a timeline.
> +
> +	   * webanimations/animation-creation-basic-expected.txt: Added.
> +	   * webanimations/animation-creation-basic.html: Added.
> +	   * webanimations/document-timeline-expected.txt: Added.
> +	   * webanimations/document-timeline.html: Added.

Aren't there Web Platform Tests for any of this?