Closed
Bug 1040543
Opened 10 years ago
Closed 10 years ago
Split AnimationPlayer and Animation into separate objects
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(12 files, 11 obsolete files)
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 |
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 | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 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•10 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•10 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 15•10 years ago
|
||
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•10 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 | ||
Comment 18•10 years ago
|
||
Attachment #8464517 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462397 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8464518 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462398 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8464520 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462399 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8464521 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462401 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8464522 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462400 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8464523 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462402 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8464524 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462403 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8464525 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462404 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8464526 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462405 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8464527 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462406 -
Attachment is obsolete: true
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8464528 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8462407 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
Comment on attachment 8464518 [details] [diff] [review]
part 2 - Rename ElementAnimationCollection to AnimationPlayerCollection
r=me
Attachment #8464518 -
Flags: review?(bzbarsky) → review+
Comment 32•10 years ago
|
||
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 33•10 years ago
|
||
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 34•10 years ago
|
||
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 35•10 years ago
|
||
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 36•10 years ago
|
||
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 37•10 years ago
|
||
Comment on attachment 8464527 [details] [diff] [review]
part 10 - Move mIsLastNotification from AnimationPlayer to Animation
r=me
Attachment #8464527 -
Flags: review?(bzbarsky) → review+
Comment 38•10 years ago
|
||
Comment on attachment 8464526 [details] [diff] [review]
part 9 - Move IsFinishedTransition from AnimationPlayer to Animation
r=me
Attachment #8464526 -
Flags: review?(bzbarsky) → review+
Comment 39•10 years ago
|
||
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•10 years ago
|
||
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 41•10 years ago
|
||
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•10 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•10 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.
Assignee | ||
Comment 44•10 years ago
|
||
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•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•