Bug 54151

Can't wait.

Created attachment 81890 [details] Work in progress. Not for review. Work in progress. Creates the DOM objects for the animations, exposed as a WebKitAnimationList of WebKitAnimations. These are plumbed through to a KeyframeAnimation on the element, but only duration is hooked up right now. DOMWindow has a method for getting the animations on a particular element.

BTW - I think if/when this lands, we can kill the pause API in DRT and simply do everything via the new API. This will allow us to go through multiple steps etc, and will handle a time before the delay, etc. So I've blocked the DRT bug on this.

<rdar://problem/8980852>

Created attachment 83423 [details] Work in Progress. Not for review. Not for review. This is against a fairly old checkout, but it implements the API for software animations. You can play/pause and scrub to any moment in the duration of an animation. The usage is something like this: var animations = window.getAnimationsForElement(someElement); for (var i = 0; i < animations.length; i++) { animations[i].pause(); animations[i].elapsedTime = 10; } I need to get HW animations to work now. Step 1 will be to implement a way that HW animations can be instantly converted to software.

Created attachment 84828 [details] Another WIP patch Everything animationy works. There is a crasher in AnimationBase destructor. Once that's fixed, write test cases, make sure we're not killing transitions, and then split this out into smaller patches. There are some easy bug fixes in here.

Created attachment 85251 [details] Part 1 Patch This is the first patch for this bug. It adds the new IDLs and implementations, but read-only for now. You can select animations and query their properties. You cannot yet scrub or play/pause. New test case to exercise the API. I'm willing to bet that I managed to screw up at least one of the non-Mac platform build files.

Comment on attachment 85251 [details] Part 1 Patch style fail!

Attachment 85251 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/page/animation/WebKitAnimation.cpp:95: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Source/WebCore/page/animation/WebKitAnimationList.cpp:60: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/page/animation/WebKitAnimationList.cpp:74: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/page/animation/WebKitAnimationList.cpp:78: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/page/animation/AnimationBase.h:175: The parameter name "time" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/page/animation/WebKitAnimation.h:36: Missing space before { [whitespace/braces] [5] Source/WebCore/page/animation/WebKitAnimation.h:79: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 9 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 85255 [details] Step 1 Patch Same comments as last time. This just has style updates. The style error in the v8 custom interface can be ignored. There is no header file for this.

Attachment 85255 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 85251 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8122043

Comment on attachment 85255 [details] Step 1 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85255&action=review Generally looks good! > LayoutTests/animations/animation-api-1.html:65 > +var a_anims = a.webkitGetAnimations(); a_anims is fugly. > Source/WebCore/dom/Element.h:348 > + PassRefPtr<WebKitAnimationList> webkitGetAnimations(); Method should be const. You don't need the prefix to propagate into the function names that are not exposed through bindings (but I guess this one does). > Source/WebCore/page/animation/AnimationBase.cpp:811 > + // Remove ourselves from the wait lists if we still have a renderer. I think you should flip the comment around. When does it not have a renderer? > Source/WebCore/page/animation/AnimationBase.h:192 > + Animation* animation() { return m_animation.get(); } const. Return const Animation*? > Source/WebCore/page/animation/AnimationController.cpp:47 > +// FIXME: Why isn't this set to 60fps or something? File a bug? > Source/WebCore/rendering/RenderObject.h:754 > + PassRefPtr<WebKitAnimationList> animations(); Not sure why you need to add this method to RenderObject. Why not just get the list via AnimationController?

Attachment 85255 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8118630

View in context: https://bugs.webkit.org/attachment.cgi?id=85255&action=review Overall looks good. Some comments we might want to talk over before committing... > LayoutTests/ChangeLog:21 > + Since you can now pause and set the animation to a particular time, seems like the perfect opportunity to do some pixel tests? > Source/WebCore/page/animation/AnimationBase.cpp:1406 > + // TODO I think these are better as FIXME: something > Source/WebCore/page/animation/AnimationBase.h:163 > It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses? > Source/WebCore/page/animation/AnimationBase.h:169 > + void pause(); How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified? > Source/WebCore/page/animation/AnimationController.cpp:47 > +// FIXME: Why isn't this set to 60fps or something? Did you open a bug to deal with this at some point? > Source/WebCore/page/animation/CompositeAnimation.cpp:581 > + Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations? > Source/WebCore/page/animation/WebKitAnimation.cpp:1 > +/* This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something? > Source/WebCore/page/animation/WebKitAnimationList.cpp:1 > +/* Same comment about where to put this file. It's really a DOM node, so maybe it belongs somewhere else?

Comment on attachment 85255 [details] Step 1 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85255&action=review >> Source/WebCore/page/animation/AnimationController.cpp:47 >> +// FIXME: Why isn't this set to 60fps or something? > > File a bug? I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time. Since you're here, why not just bump it to 60fps? I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes).

Hey and since you are poking at animations, do any of y'all wanna look at the patch on bug 51952? :P

(In reply to comment #15) > > > LayoutTests/ChangeLog:21 > > + > > Since you can now pause and set the animation to a particular time, seems like the perfect opportunity to do some pixel tests? Nope - I can't pause yet. That's step/patch 2. > > > Source/WebCore/page/animation/AnimationBase.cpp:1406 > > + // TODO > > I think these are better as FIXME: something OK > > > Source/WebCore/page/animation/AnimationBase.h:163 > > > > It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses? You might be able to pause transitions (eventually). The reason they are on AnimationBase is that Step 2 dives deep into the heart of the animation state machine, and for that it needs class access. > > > Source/WebCore/page/animation/AnimationBase.h:169 > > + void pause(); > > How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified? This pause() comes externally (from JS). I think it will be more clear in Patch 2. > > > Source/WebCore/page/animation/AnimationController.cpp:47 > > +// FIXME: Why isn't this set to 60fps or something? > > Did you open a bug to deal with this at some point? Nope, but I will. > > > Source/WebCore/page/animation/CompositeAnimation.cpp:581 > > + > > Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations? I'm not sure what operation you're talking about, but probably the one where you get the list of animations. I don't think it will ever be called outside the JS API. It is a bit expensive to create a list each time, but given that animations can disappear we can't really have it live. My feeling in general is that once you start using this API you're going to suffer in performance. Hardware animations will jump to software. But that's ok because you're probably an authoring tool of some sort, or have a good reason to start manipulating things. > > > Source/WebCore/page/animation/WebKitAnimation.cpp:1 > > +/* > > This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? It's got this name because that is what makes sense externally. I can't think of a better name to expose to the bindings. > Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something? Good point. The implementation probably could live in page. > > > Source/WebCore/page/animation/WebKitAnimationList.cpp:1 > > +/* > > Same comment about where to put this file. It's really a DOM node, so maybe it belongs somewhere else? page? It isn't strictly a DOM node - it doesn't exist in the document. Thanks!

(In reply to comment #16) > I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time. Since you're here, why not just bump it to 60fps? I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes). I'll do that, thanks! James, while you're watching, I can't work out why Chromium builds are not generating V8WebKitAnimation.h. Do you have any idea? I added a new section for page/animation/%.idl in Android.derived.v8bindings.mk

(In reply to comment #19) > (In reply to comment #16) > > > I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time. Since you're here, why not just bump it to 60fps? I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes). > > I'll do that, thanks! > > James, while you're watching, I can't work out why Chromium builds are not generating V8WebKitAnimation.h. Do you have any idea? > > I added a new section for page/animation/%.idl in Android.derived.v8bindings.mk Chromium doesn't use Android.derived.v8bindings.mk (confusing, isn't it) I think you need to add the new IDLs to the webcore_bindings_idl_files list in http://trac.webkit.org/export/80677/trunk/Source/WebCore/WebCore.gypi

(In reply to comment #16) > I asked hyatt why this was set to 25ms and he said he just picked a number that seemed good at the time. Since you're here, why not just bump it to 60fps? I tried that locally and no tests failed, but it didn't seem to make any huge difference in animation quality either (at least to my eyes). Probably because of bug 47447.

(In reply to comment #18) > > > > > > Source/WebCore/page/animation/AnimationBase.h:163 > > > > > > > It seems odd that this API is in AnimationBase since KeyframeAnimations are the only things you can use this API on. Although, if you could unify some of the API, maybe this can be applicable to the other "pause" uses? > > You might be able to pause transitions (eventually). The reason they are on AnimationBase is that Step 2 dives deep into the heart of the animation state machine, and for that it needs class access. Right. That makes sense. > > > > > > Source/WebCore/page/animation/AnimationBase.h:169 > > > + void pause(); > > > > How are these different from pauseAnimation/startAnimation and all that glop? Can the be unified? > > This pause() comes externally (from JS). I think it will be more clear in Patch 2. But right now we have suspend/resume to stop all animations/transitions, the pauseAPI (which your patch will hopefully allow us to rip out), the play-state property, and your new API. Seems like it could all be rolled into one as far as AnimationBase is concerned. That would make the state machine simpler. Then the higher level logic (CompositeAnimation and AnimationController) can use that single API as needed. But maybe that's a big scary change at this point. So maybe we just defer it until later (meaning "never" :-) > > > > > Source/WebCore/page/animation/CompositeAnimation.cpp:581 > > > + > > > > Is this possibly expensive operation in any fast paths? Or is it just used when the author asks for animations? > > I'm not sure what operation you're talking about, but probably the one where you get the list of animations. I don't think it will ever be called outside the JS API. It is a bit expensive to create a list each time, but given that animations can disappear we can't really have it live. > > My feeling in general is that once you start using this API you're going to suffer in performance. Hardware animations will jump to software. But that's ok because you're probably an authoring tool of some sort, or have a good reason to start manipulating things. Yeah, that's fine. As long as this never gets called from any style change logic. If it's just for your new API, it can be slow as hell and the author will never notice. > > > > > > Source/WebCore/page/animation/WebKitAnimation.cpp:1 > > > +/* > > > > This is a confusing name. How is it different from AnimationBase, ImplicitAnimation, CompositeAnimation, etc.? > > It's got this name because that is what makes sense externally. I can't think of a better name to expose to the bindings. > > > Yes, I know many of the names are confusing, but let's not add to it. Maybe it would be better for this object to be in dom or html or something? > > Good point. The implementation probably could live in page. My only push back is that the AnimationList is returned from a call on Element, so having it in 'dom' might make sense. But moving it out to page with all the other node'y things with idl's is probably fine.

Attachment 85255 [details] did not build on win: Build output: http://queues.webkit.org/results/8114888

Created attachment 85410 [details] Review changes Changes from review. Hoping that I've fixed build errors.

Comment on attachment 85410 [details] Review changes Setting to r? I assume that's what kicks off the bots. The patch already has an r+.

Attachment 85410 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 42 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 85410 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8141093

Attachment 85410 [details] did not build on win: Build output: http://queues.webkit.org/results/8131105

Created attachment 85435 [details] Build fixes for Windows and Cr-Linux Some moronic errors were breaking the build.

Comment on attachment 85435 [details] Build fixes for Windows and Cr-Linux No review needed

Attachment 85435 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 85435 [details] did not build on win: Build output: http://queues.webkit.org/results/8139146

Created attachment 85459 [details] Updated patch No review needed. - It turns out the style error that I thought was a mistake was telling me I'd made a typo (Webkit -> WebKit) in the v8 bindings :) - Use numeric_limit<> rather than INFINITY - Make sure the animations are ordered as they are returned (test was occasionally failing) - Test also checks for null now.

Attachment 85459 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1 Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 2 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Part 1 landed. http://trac.webkit.org/changeset/80846 Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/animations/animation-api-1-expected.txt A LayoutTests/animations/animation-api-1.html M LayoutTests/fast/dom/Window/window-properties-expected.txt M LayoutTests/fast/dom/Window/window-property-descriptors-expected.txt M LayoutTests/fast/dom/prototype-inheritance-2-expected.txt M LayoutTests/fast/dom/prototype-inheritance-expected.txt M LayoutTests/fast/js/global-constructors-expected.txt M Source/WebCore/Android.derived.jscbindings.mk M Source/WebCore/Android.derived.v8bindings.mk M Source/WebCore/Android.mk M Source/WebCore/Android.v8bindings.mk M Source/WebCore/CMakeLists.txt M Source/WebCore/ChangeLog M Source/WebCore/CodeGenerators.pri M Source/WebCore/DerivedSources.cpp M Source/WebCore/DerivedSources.make M Source/WebCore/GNUmakefile.am M Source/WebCore/WebCore.gypi M Source/WebCore/WebCore.pro M Source/WebCore/WebCore.vcproj/WebCore.vcproj M Source/WebCore/WebCore.xcodeproj/project.pbxproj M Source/WebCore/bindings/js/JSBindingsAllInOne.cpp A Source/WebCore/bindings/js/JSWebKitAnimationCustom.cpp A Source/WebCore/bindings/js/JSWebKitAnimationListCustom.cpp A Source/WebCore/bindings/v8/custom/V8WebKitAnimationCustom.cpp M Source/WebCore/dom/Element.cpp M Source/WebCore/dom/Element.h M Source/WebCore/dom/Element.idl M Source/WebCore/page/DOMWindow.idl A Source/WebCore/page/WebKitAnimation.cpp A Source/WebCore/page/WebKitAnimation.h A Source/WebCore/page/WebKitAnimation.idl A Source/WebCore/page/WebKitAnimationList.cpp A Source/WebCore/page/WebKitAnimationList.h A Source/WebCore/page/WebKitAnimationList.idl M Source/WebCore/page/animation/AnimationBase.cpp M Source/WebCore/page/animation/AnimationBase.h M Source/WebCore/page/animation/AnimationController.cpp M Source/WebCore/page/animation/AnimationController.h M Source/WebCore/page/animation/AnimationControllerPrivate.h M Source/WebCore/page/animation/CompositeAnimation.cpp M Source/WebCore/page/animation/CompositeAnimation.h Committed r80846

http://trac.webkit.org/changeset/80846 might have broken Leopard Intel Release (Build)

It looks like this might be causing bug 56325.

Comment on attachment 85255 [details] Step 1 Patch Cleared Simon Fraser's review+ from obsolete attachment 85255 [details] so that this bug does not appear in http://webkit.org/pending-commit.

*** Bug 26850 has been marked as a duplicate of this bug. ***

How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P). Thanks, Paul

(In reply to comment #40) > How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P). It's stalled on me making a proposal to public-fx so that other vendors can comment :( I've been busy with other things. If someone wants to start that ball rolling, I'd be most appreciative.

(In reply to comment #41) > (In reply to comment #40) > > How much work still needs to be done to finish this feature? The web community is eargerly awaiting it (read: me :P). > > It's stalled on me making a proposal to public-fx so that other vendors can comment :( I've been busy with other things. > > If someone wants to start that ball rolling, I'd be most appreciative. Hey Dean, thanks for the update. I'm actually happy to help if you can get me a gist of the API. Just sent you an email, let's connect and make it happen.

How is this going? Is it done on some other bug or it has been declined?

There has been work on an Animation API in W3C. I've stalled on this bug until things are more clear there.

We now support Web Animations and each Animation object can have its currentTime property manipulated, including CSS Animations and CSS Transitions.