Bug 178526

Created attachment 324255 [details] Patch

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

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

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

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

Created attachment 324369 [details] Patch

Created attachment 324372 [details] Patch

Created attachment 324377 [details] Patch

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.

Created attachment 324387 [details] Patch

Created attachment 324388 [details] Patch

(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 on attachment 324388 [details] Patch Clearing flags on attachment: 324388 Committed r223779: <https://trac.webkit.org/changeset/223779>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35271071>