[webkit-reviews] review granted: [Bug 178526] [Web Animations] Provide basic timeline and animation interfaces : [Attachment 324255] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 19 11:54:28 PDT 2017
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?
More information about the webkit-reviews
mailing list