Current ElementAnimation serves as both an Animation and an AnimationPlayer. In order to implement more of the API, particularly playback control, we should split off separate objects for these.

These are just some WIP patches--they build but break animations. I'll debug them next week but I wanted to get some early feedback on whether you agree with splitting up the objects like this. This is currently blocking David Z's work on bug 1033114 (which DevTools folks, and maybe Gaia folks too, are waiting on) so I'm really keen to get something landed before I go away mid-next week. Any feedback to help get this ready would be much appreciated. I'll probably ask Boris to do the reviewing since there's a fair bit of WebIDL changes there but I'd appreciate your thoughts too David.

Comment on attachment 8462410 [details] [diff] [review] Roll-up patch of parts 1~11 Boris, does splitting up ElementAnimation into two objects, AnimationPlayer and Animation seem reasonable to you? As opposed to, say, having one object and allocating a tear-off object when necessary? Note that in future I expect we'll implement the Web Animations API properly such that authors can individually construct these objects and knit them together (i.e. myNewPlayer.source = new Animation(...) etc.) so I thought it makes sense to make them independent objects from the start. Personally I don't really like that fact that Web Animations has these separate AnimationPlayer objects but I lost that argument and it's in the spec now. (I wrote a blog with some background: http://brian.sol1.net/svg/2013/07/25/players-wanted-the-pause-and-seek-game/)

If David and Boris agree with this general approach, I plan to tidy up these patches first-thing next week and put up for review then.

Comment on attachment 8462410 [details] [diff] [review] Roll-up patch of parts 1~11 I think having the separate objects is fine.

Comment on attachment 8462410 [details] [diff] [review] Roll-up patch of parts 1~11 So I don't feel like I'm in a good position to evaluate whether the extra complexity is worth it. If you think it is, then I'm ok with it. But we can also push back on pieces of the spec that are too complex by not implementing them; given that (as I understand it) we only have 2 of the 4 browser engines working on Web Animations so far, it's entirely possible that doing so could increase the eventual chance of 4-engine interoperability. But it could also reduce it. It also feels a little weird to have the implementation of CSS concepts living in dom/. Then again, that's probably better than adding an extra layer to avoid doing that. I guess it's not clear to me what other issues you wanted feedback on -- I probably should have asked to clarify that sooner rather than stalling on getting back to you.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #16) > Comment on attachment 8462410 [details] [diff] [review] > Roll-up patch of parts 1~11 > > So I don't feel like I'm in a good position to evaluate whether the extra > complexity is worth it. If you think it is, then I'm ok with it. I think it's ok. The only people I've heard express concern about it are Anne and Dean Jackson. Separating dynamic playback state from a static description of an animation is probably not a bad architectural distinction anyway. > But we > can also push back on pieces of the spec that are too complex by not > implementing them; given that (as I understand it) we only have 2 of the 4 > browser engines working on Web Animations so far, it's entirely possible > that doing so could increase the eventual chance of 4-engine > interoperability. But it could also reduce it. I think we're more likely to frustrate the other current implementer and end up in a worse situation. Microsoft haven't expressed any concern about complexity and list the API as "under consideration" (http://status.modern.ie/webanimationsjavascriptapi). Apparently there has been some work from Apple on this but I believe it has stalled due to limited resources not due to the complexity of the spec. > It also feels a little weird to have the implementation of CSS concepts > living in dom/. Then again, that's probably better than adding an extra > layer to avoid doing that. I think so although we may end up adding CSS subclasses of some of these objects at some point. > I guess it's not clear to me what other issues you wanted feedback on -- I > probably should have asked to clarify that sooner rather than stalling on > getting back to you. I just wanted to make sure you weren't opposed to the idea altogether and also to check you didn't have major refactorings to these classes waiting to land that this would interfere with.

Comment on attachment 8464517 [details] [diff] [review] part 1 - Move/Rename ElementAnimations to dom::AnimationPlayer It would have been nice to preserve blame here by doing an hg cp and then deleting the now-irrelevant code instead of copy/pasting large blobs of code. Would still be preferred if you're willing to do that. It would also have made reviewing this way simpler. :( r=me

Comment on attachment 8464518 [details] [diff] [review] part 2 - Rename ElementAnimationCollection to AnimationPlayerCollection r=me

Comment on attachment 8464520 [details] [diff] [review] part 3 - Add Animation interface >+#include "Animation.h" mozilla/dom/Animation.h, please. >+ * http://dev.w3.org/fxtf/web-animations/#the-animationtimeline-interface Should be http://dev.w3.org/fxtf/web-animations/#the-animation-interface >+ [Pure] readonly attribute Animation? source; Per spec this should be an AnimationNode, and be writable. Can you please add a comment explaining the deviation, ideally with links to the relevant bugs? r=me

Comment on attachment 8464521 [details] [diff] [review] part 4 - Create Animation objects and set on AnimationPlayer r=me

Comment on attachment 8464522 [details] [diff] [review] part 5 - Pass down time from AnimationPlayer to Animation r=me

Comment on attachment 8464523 [details] [diff] [review] part 6 - Rename mAnimations to mPlayers and likewise for similar local variables >+ AnimationPlayer* player, Layer* aLayer, While we're here, aPlayer would be consistent with the other arguments r=me

Comment on attachment 8464524 [details] [diff] [review] part 7 - Move keyframe properties from AnimationPlayer to Animation >These properties are returns from the Animation by a couple of Properties() s/returns/returned/ >methods that provide direct access to the member variable. In future the it is s/the it/it/ >+++ b/layout/base/nsDisplayList.cpp >+ AnimationProperty* property = &anim->Properties()[propIdx]; Ideally we'd use AnimationProperty& on the LHS here... followup is OK for that. r=me

Comment on attachment 8464527 [details] [diff] [review] part 10 - Move mIsLastNotification from AnimationPlayer to Animation r=me

Comment on attachment 8464526 [details] [diff] [review] part 9 - Move IsFinishedTransition from AnimationPlayer to Animation r=me

Comment on attachment 8464526 [details] [diff] [review] part 9 - Move IsFinishedTransition from AnimationPlayer to Animation Er, actually, maybe SetIsFinishedTransition instead of SetFinishedTransition? I guess you just kept the old name....

Comment on attachment 8464525 [details] [diff] [review] part 8 - Move timing parameters from AnimationPlayer to Animation r=me

Comment on attachment 8464528 [details] [diff] [review] part 11 - Make ElementPropertyTransition inherit from Animation instead of AnimationPlayer r=me

(In reply to On vacation Aug 5-18. Please do not request review. from comment #30) > Comment on attachment 8464517 [details] [diff] [review] > part 1 - Move/Rename ElementAnimations to dom::AnimationPlayer > > It would have been nice to preserve blame here by doing an hg cp and then > deleting the now-irrelevant code instead of copy/pasting large blobs of > code. Would still be preferred if you're willing to do that. It would also > have made reviewing this way simpler. :( Thanks for the reviews. Sorry about that. Yes, I was looking for a way of doing that but didn't think of using hg cp. I'll make that change but it will take a little longer to land since I'm also on leave.

(In reply to On vacation Aug 5-18. Please do not request review. from comment #32) > Comment on attachment 8464520 [details] [diff] [review] > part 3 - Add Animation interface > >+ [Pure] readonly attribute Animation? source; > > Per spec this should be an AnimationNode, and be writable. Can you please > add a comment explaining the deviation, ideally with links to the relevant > bugs? That's simply because I'm implementing this piecemeal. I haven't introduced the AnimationNode interface yet since we don't plan to implement the only other subinterface, AnimationGroup, in the short-term and because some people are saying we should mark it [NoInterfaceObject], in which case there'd be no observable change by dropping it as far as I can tell. Also, I haven't implemented the writable behavior yet since that's quite a bit of work. I'll file bugs on those missing features and add comments.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b03265860af9 m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/414eb2e95057 https://hg.mozilla.org/integration/mozilla-inbound/rev/daca3c0d2c85 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc995232033e https://hg.mozilla.org/integration/mozilla-inbound/rev/a50e04b3a699 https://hg.mozilla.org/integration/mozilla-inbound/rev/86f0c54582ff https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0e15df4168 https://hg.mozilla.org/integration/mozilla-inbound/rev/34cb6ae672bd https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d0f91ff4c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a12123bef3f https://hg.mozilla.org/integration/mozilla-inbound/rev/851652501f18 https://hg.mozilla.org/integration/mozilla-inbound/rev/4234319389c3

https://hg.mozilla.org/mozilla-central/rev/414eb2e95057 https://hg.mozilla.org/mozilla-central/rev/daca3c0d2c85 https://hg.mozilla.org/mozilla-central/rev/cc995232033e https://hg.mozilla.org/mozilla-central/rev/a50e04b3a699 https://hg.mozilla.org/mozilla-central/rev/86f0c54582ff https://hg.mozilla.org/mozilla-central/rev/8d0e15df4168 https://hg.mozilla.org/mozilla-central/rev/34cb6ae672bd https://hg.mozilla.org/mozilla-central/rev/d6d0f91ff4c4 https://hg.mozilla.org/mozilla-central/rev/3a12123bef3f https://hg.mozilla.org/mozilla-central/rev/851652501f18 https://hg.mozilla.org/mozilla-central/rev/4234319389c3