Bug 178526 - [Web Animations] Provide basic timeline and animation interfaces
Summary: [Web Animations] Provide basic timeline and animation interfaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-19 11:42 PDT by Antoine Quint
Modified: 2017-10-31 06:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (52.98 KB, patch)
2017-10-19 11:45 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.90 MB, application/zip)
2017-10-19 13:31 PDT, Build Bot
no flags Details
Patch (115.70 KB, patch)
2017-10-20 00:59 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (670.28 KB, patch)
2017-10-20 01:21 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (113.89 KB, patch)
2017-10-20 02:02 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (113.06 KB, patch)
2017-10-20 04:58 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (113.00 KB, patch)
2017-10-20 05:00 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2017-10-19 11:42:57 PDT
[Web Animations] Provide basic timeline and animation interfaces
Comment 1 Antoine Quint 2017-10-19 11:45:23 PDT
Created attachment 324255 [details]
Patch
Comment 2 Dean Jackson 2017-10-19 11:54:28 PDT
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?
Comment 3 Dean Jackson 2017-10-19 11:55:08 PDT
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.
Comment 4 Build Bot 2017-10-19 13:31:23 PDT
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
Comment 5 Build Bot 2017-10-19 13:31:24 PDT
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
Comment 6 Antoine Quint 2017-10-19 14:19:00 PDT
(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.
Comment 7 Antoine Quint 2017-10-19 14:19:27 PDT
(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.
Comment 8 Antoine Quint 2017-10-20 00:59:59 PDT
Created attachment 324369 [details]
Patch
Comment 9 Antoine Quint 2017-10-20 01:21:41 PDT
Created attachment 324372 [details]
Patch
Comment 10 Antoine Quint 2017-10-20 02:02:34 PDT
Created attachment 324377 [details]
Patch
Comment 11 Sam Weinig 2017-10-20 04:48:03 PDT
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.
Comment 12 Antoine Quint 2017-10-20 04:58:00 PDT
Created attachment 324387 [details]
Patch
Comment 13 Antoine Quint 2017-10-20 05:00:11 PDT
Created attachment 324388 [details]
Patch
Comment 14 Antoine Quint 2017-10-20 05:00:46 PDT
(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!
Comment 15 WebKit Commit Bot 2017-10-20 11:41:27 PDT
Comment on attachment 324388 [details]
Patch

Clearing flags on attachment: 324388

Committed r223779: <https://trac.webkit.org/changeset/223779>
Comment 16 WebKit Commit Bot 2017-10-20 11:41:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-10-31 06:40:34 PDT
<rdar://problem/35271071>