Bug 178526 – [Web Animations] Provide basic timeline and animation interfaces

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

[Antoine Quint]

[Web Animations] Provide basic timeline and animation interfaces

[Antoine Quint]

Created attachment 324255 [details]
Patch

[Dean Jackson]

Comment on attachment 324255 [details]
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?

[Dean Jackson]

Comment on attachment 324255 [details]
Patch

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

> LayoutTests/webanimations/animation-creation-basic.html:30
> +
> +debug("");
> +

Remove.

> LayoutTests/webanimations/document-timeline.html:27
> +
> +debug("");
> +

Remove.

[Build Bot]

Comment on attachment 324255 [details]
Patch

Attachment 324255 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4922365

New failing tests:
webarchive/adopt-attribute-styled-node-webarchive.html

[Build Bot]

Created attachment 324275 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Antoine Quint]

(In reply to Dean Jackson from comment #2)
> Comment on attachment 324255 [details]
> 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.

We need the custom JS because AnimationTimeline is subclassed, yes. If we don't provide one, only AnimationTimeline JS wrapper objects are created for DocumentTimeline objects.

> > 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?

Correct, it should. I will remove this code for now and add this to a follow-up patch when we can dump the contents of a timeline.

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

OK, I'll remove the build-time flag.

> > Source/WebCore/animation/WebAnimation.idl:29
> > +    InterfaceName=Animation,
> 
> Why are you calling it Animation rather than WebAnimation?

There already is an Animation class in platform/animation.

> > 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?

There are but I figured that before looking into which platform test to use for this, we could have some homegrown ones. I think WPT ones will be more useful once we have more code to test. At this stage, we want to get the internals started.

[Antoine Quint]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 324255 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324255&action=review
> 
> > LayoutTests/webanimations/animation-creation-basic.html:30
> > +
> > +debug("");
> > +
> 
> Remove.
> 
> > LayoutTests/webanimations/document-timeline.html:27
> > +
> > +debug("");
> > +
> 
> Remove.

I think the test output is much better with blank lines.

[Antoine Quint]

Created attachment 324369 [details]
Patch

[Antoine Quint]

Created attachment 324372 [details]
Patch

[Antoine Quint]

Created attachment 324377 [details]
Patch

[Sam Weinig]

Comment on attachment 324377 [details]
Patch

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

> Source/WebCore/animation/AnimationTimeline.idl:29
> +    ImplementationLacksVTable

I think you can remove this, as your AnimationTimeline has a vtable.

> Source/WebCore/animation/DocumentTimeline.idl:28
> +    ImplementationLacksVTable

Same as above.

[Antoine Quint]

Created attachment 324387 [details]
Patch

[Antoine Quint]

Created attachment 324388 [details]
Patch

[Antoine Quint]

(In reply to Sam Weinig from comment #11)
> Comment on attachment 324377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324377&action=review
> 
> > Source/WebCore/animation/AnimationTimeline.idl:29
> > +    ImplementationLacksVTable
> 
> I think you can remove this, as your AnimationTimeline has a vtable.
> 
> > Source/WebCore/animation/DocumentTimeline.idl:28
> > +    ImplementationLacksVTable
> 
> Same as above.

Indeed, thanks Sam!

[WebKit Commit Bot]

Comment on attachment 324388 [details]
Patch

Clearing flags on attachment: 324388

Committed r223779: <https://trac.webkit.org/changeset/223779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35271071>