Split AnimationPlayer and Animation into separate objects

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 11 obsolete attachments)

135.45 KB, patch
dbaron
: feedback+
bzbarsky
: feedback+
Details | Diff | Splinter Review
76.20 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
54.31 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
42.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
31.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
48.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.65 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
16.68 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 998245
Assignee

Comment 3

5 years ago
Assignee

Comment 12

5 years ago
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.
Attachment #8462410 - Flags: feedback?(dbaron)
Assignee

Comment 13

5 years ago
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/)
Attachment #8462410 - Flags: feedback?(bzbarsky)
Assignee

Comment 14

5 years ago
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.
Attachment #8462410 - Flags: feedback?(bzbarsky) → feedback+
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.
Attachment #8462410 - Flags: feedback?(dbaron) → feedback+
Assignee

Comment 17

5 years ago
(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.
Assignee

Updated

5 years ago
Blocks: 1045993
Assignee

Updated

5 years ago
Blocks: 1045994
Assignee

Updated

5 years ago
Attachment #8462397 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462398 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
Attachment #8464520 - Flags: review?(bzbarsky)
Assignee

Updated

5 years ago
Attachment #8462399 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462401 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462400 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462402 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462403 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462404 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462405 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462406 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8462407 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Blocks: 927349
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
Attachment #8464517 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464518 [details] [diff] [review]
part 2 - Rename ElementAnimationCollection to AnimationPlayerCollection

r=me
Attachment #8464518 - Flags: review?(bzbarsky) → review+
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
Attachment #8464520 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464521 [details] [diff] [review]
part 4 - Create Animation objects and set on AnimationPlayer

r=me
Attachment #8464521 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464522 [details] [diff] [review]
part 5 - Pass down time from AnimationPlayer to Animation

r=me
Attachment #8464522 - Flags: review?(bzbarsky) → review+
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
Attachment #8464523 - Flags: review?(bzbarsky) → review+
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
Attachment #8464524 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464527 [details] [diff] [review]
part 10 - Move mIsLastNotification from AnimationPlayer to Animation

r=me
Attachment #8464527 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464526 [details] [diff] [review]
part 9 - Move IsFinishedTransition from AnimationPlayer to Animation

r=me
Attachment #8464526 - Flags: review?(bzbarsky) → review+
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
Attachment #8464525 - Flags: review?(bzbarsky) → review+
Comment on attachment 8464528 [details] [diff] [review]
part 11 - Make ElementPropertyTransition inherit from Animation instead of AnimationPlayer

r=me
Attachment #8464528 - Flags: review?(bzbarsky) → review+
Assignee

Comment 42

5 years ago
(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.
Assignee

Comment 43

5 years ago
(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.
Depends on: 1052147
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.