1040543 - Split AnimationPlayer and Animation into separate objects

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Move/Rename ElementAnimations to dom::AnimationPlayer

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Rename ElementAnimationCollection to AnimationPlayerCollection

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add Animation interface

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Pass down time from AnimationPlayer to Animation

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Create Animation objects outside AnimationPlayer and then set them

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Rename mAnimations to mPlayers and likewise for similar local variables

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Move keyframe properties from AnimationPlayer to Animation

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Move timing parameters from AnimationPlayer to Animation

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Move IsFinishedTransition from AnimationPlayer to Animation

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 10 - Move mIsLastNotification from AnimationPlayer to Animation

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 11 - Make ElementPropertyTransition inherit from Animation instead of AnimationPlayer

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Roll-up patch of parts 1~11

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 13

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8462410 [details] [diff] [review]
Roll-up patch of parts 1~11

I think having the separate objects is fine.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Move/Rename ElementAnimations to dom::AnimationPlayer

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Rename ElementAnimationCollection to AnimationPlayerCollection

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Add Animation interface

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Create Animation objects and set on AnimationPlayer

Comment 22

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Pass down time from AnimationPlayer to Animation

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Rename mAnimations to mPlayers and likewise for similar local variables

Comment 24

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Move keyframe properties from AnimationPlayer to Animation

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Move timing parameters from AnimationPlayer to Animation

Comment 26

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Move IsFinishedTransition from AnimationPlayer to Animation

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 10 - Move mIsLastNotification from AnimationPlayer to Animation

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 11 - Make ElementPropertyTransition inherit from Animation instead of AnimationPlayer

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://tbpl.mozilla.org/?tree=Try&rev=990193f9a3a2

Comment 30

[Boris Zbarsky [:bzbarsky]]

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 31

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 32

[Boris Zbarsky [:bzbarsky]]

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 33

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 34

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 35

[Boris Zbarsky [:bzbarsky]]

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 36

[Boris Zbarsky [:bzbarsky]]

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 37

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 39

[Boris Zbarsky [:bzbarsky]]

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 40

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 43

[Brian Birtles (:birtles, Feb 26~ parental 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.

Comment 44

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 45

[Nigel Babu [:nigelb]]

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