Closed
Bug 1150810
Opened 10 years ago
Closed 9 years ago
Implement AnimationTimeline.getAnimations()
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(18 files)
39.27 KB,
patch
|
Details | Diff | Splinter Review | |
3.87 KB,
patch
|
Details | Diff | Splinter Review | |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
40 bytes,
text/x-review-board-request
|
jwatt
:
review+
|
Details |
While we could probably do something fairly simple that just walks the document and collects up the animations I'd like to use this as a chance to do some of the refactoring we'll need to support script-generated animations (which we'd like to do in Q3 or Q4). Some possible refactorings include: * Rename AnimationTimeline to DocumentTimeline to reflect recent changes in the spec * Make the timeline the point where we register with the refresh driver. We don't want to have to introduce yet another type of manager (in addition to nsAnimationManager and nsTransitionManager) in order to support script-generated animations so we want to move as much code out of those managers as possible. The times reported by the timeline are taken from the refresh driver so it makes most sense to register with the refresh driver at this point. * Introduce some way of making animations subscribe to ticks from their timeline. This should hopefully allow us to eventually have animations that aren't associated with an element. * Introduce a pseudo element class? Or just some opaque type to represent the target of animations targeting a pseudo element? * Move event queueing to CSSAnimation/CSSTransition? We still need to keep the list of animations on an element so we can restyle a given element applying the appropriate ordering.
Assignee | ||
Comment 1•9 years ago
|
||
This is as far as I've got so far. The main pieces missing are: * Making the dtor for AnimationCollection always cancel animations (so that document.timeline.getAnimations() no longer returns the animations from a removed element; and because we'll eventually want to dispatch animationcancel/transitioncancel in this case) is triggering a warning on EventStateManager.cpp about a mismatch between the passed-in frame and content. Perhaps the element has already been disassociated from its frame by this point. I need to look into this anyway. * I haven't implemented sorting yet although I've drawn up a sketch of how it should work: https://github.com/w3c/web-animations/issues/62. I'll go through this will Google next week and see what they think. * There are still more tests to write although most of them should be fairly straight forward and I've outlined what they are already. * There's a call to SetTimeline(nullptr) in the unlink method for Animation which probably isn't necessary but I need to verify this.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Note that most of the refactoring outlined in comment 1 is happening under bug 1151731 although the renaming already happened in bug 1152171.
Assignee | ||
Updated•9 years ago
|
Summary: Implement DocumentTimeline.getAnimations() → Implement AnimationTimeline.getAnimations()
Assignee | ||
Comment 3•9 years ago
|
||
As part of this bug, I'm trying to make sure we cancel animations on an element when it is unbound from the tree. We already delete the property that contains the animations when we unbind the element here: http://mxr.mozilla.org/mozilla-central/source/dom/base/Element.cpp?rev=733947acb2f7#1727 However, when we do this, we don't call cancel on the animations stored in that property. That only happens when we explicitly destroy the animation collection using this method: http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.h?rev=e09370e4a895#266 It doesn't happen when we simply call the property's destructor. I'd like to change this behavior so that we cancel events in the property's destructor, i.e. here: http://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.cpp?rev=b1385ccee5f2#741 The reason I want to do this is to give removed animations a chance to remove themselves from the timeline they're registered against so that document.timeline.getAnimations() doesn't return them. Furthermore, presumably we should be creating removed animation observer records in this case. (Also, in the future when we come to implement the animationcancel and transitioncancel events, we'll likely want to cancel on the animations when we unbind the element so we can queue those events.) Anyway, after moving the cancelling to the property destructor I start hitting this warning: http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp?rev=f40854954ae0#485 Specifically, what happens is: 1. Load http://jsfiddle.net/gkvL1rtu/ (most of which is unrelated except that it removes an element with a running animation from the document) 2. A moment later I get a series of failed warnings with a stack like the following: xul.dll!mozilla::EventStateManager::PreHandleEvent(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, nsIFrame * aTargetFrame, nsIContent * aTargetContent, nsEventStatus * aStatus) Line 490 C++ xul.dll!PresShell::HandleEventInternal(mozilla::WidgetEvent * aEvent, nsEventStatus * aStatus) Line 8154 C++ xul.dll!PresShell::HandlePositionedEvent(nsIFrame * aTargetFrame, mozilla::WidgetGUIEvent * aEvent, nsEventStatus * aEventStatus) Line 8010 C++ xul.dll!PresShell::HandleEvent(nsIFrame * aFrame, mozilla::WidgetGUIEvent * aEvent, bool aDontRetargetEvents, nsEventStatus * aEventStatus, nsIContent * * aTargetContent) Line 7797 C++ xul.dll!nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent * aEvent, nsView * aView, nsEventStatus * aStatus) Line 787 C++ xul.dll!nsView::HandleEvent(mozilla::WidgetGUIEvent * aEvent, bool aUseAttachedEvents) Line 1095 C++ xul.dll!nsWindow::DispatchEvent(mozilla::WidgetGUIEvent * event, nsEventStatus & aStatus) Line 3639 C++ xul.dll!nsBaseWidget::DispatchInputEvent(mozilla::WidgetInputEvent * aEvent) Line 1071 C++ xul.dll!nsWindow::ClientMarginHitTestPoint(int mx, int my) Line 5609 C++ xul.dll!nsWindow::ProcessMessage(unsigned int msg, unsigned int & wParam, long & lParam, long * aRetValue) Line 4677 C++ xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd, unsigned int msg, unsigned int wParam, long lParam) Line 4361 C++ xul.dll!CallWindowProcCrashProtected(long (HWND__ *, unsigned int, unsigned int, long) * aWndProc, HWND__ * aHWnd, unsigned int aMsg, unsigned int aWParam, long aLParam) Line 35 C++ xul.dll!nsWindow::WindowProc(HWND__ * hWnd, unsigned int msg, unsigned int wParam, long lParam) Line 4313 C++ (The event in this case is WM_NCHITTEST) If I cause the call to Animation::CancelFromStyle to no longer trigger 'nsNodeUtils::AnimationRemoved(this)' then the warning no longer fires. That call to AnimationRemoved is here: http://mxr.mozilla.org/mozilla-central/source/dom/animation/Animation.cpp?rev=70c5890d9577#450 It seems like somehow we end up with an animation removed mutation record associated with a frame and later there's a mismatch between that frame and the content that gets associated with it? Olli, can you give me any pointers about what might be going on here?
Flags: needinfo?(bugs)
Comment 5•9 years ago
|
||
So are we dispatching events to that removed-from-dom element via presshell? Or what triggers that warning? What is the event there and what causes the condition to fail? >It seems like somehow we end up with an animation removed mutation record associated with a frame mutation records are associated with nodes, not frames > and later there's a mismatch between that frame and the content that gets associated with it? I don't think that is the case. But what are the events causing the warnings?
Flags: needinfo?(bugs)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5) > So are we dispatching events to that removed-from-dom element via presshell? > Or what triggers that warning? What is the event there and what causes the > condition to fail? It's a WM_NCHITTEST windows event in this case. Since it's a bunch of || conditions, all of them are failing. That is, we've got a target frame with content but the frame's content and parent content don't match the passed-in target content. > > and later there's a mismatch between that frame and the content that gets associated with it? > I don't think that is the case. But what are the events causing the warnings? It seems to always be WM_NCHITTEST. The particular frames / content that end up getting passed to PreHandleEvent, however, vary each time (presumably depending on which frame matches the hit-testing). Currently I'm seeing an nsTextFrame target frame and a HTMLSpanElement target content but yesterday I recall seeing a nsTextNode target content. I'll keep digging into it. Currently, I'm wondering if somehow the mCurrentEventFrame and mCurrentEventContent members of the presshell are getting out of sync, but if you have any suggestions of where to look that would be a great help!
Assignee | ||
Comment 7•9 years ago
|
||
It looks like what gets passed to PreHandleEvent looks like the following target content: HTMLSpanElement target frame -> content: nsTextNode -> parent: nsXMLElement -> parent: HTMLSpanElement === target content -> parent: <a>, <li>, (shared-list), <div>, <div>, <form>, <body>, (shared-elem), doc So the warning fails because it only looks up one level of to the frame's content's parent (if it looked up to the grandparent it would be ok).
Assignee | ||
Comment 8•9 years ago
|
||
A couple more things I noticed: * the HTMLSpanElement here has mFirstChild == nullptr so it seems like the <span> has disowned its children * we can also get WM_MOUSEMOVE However, I've managed to reproduce this issue in a regular trunk build with no patches applied so it doesn't appear to be related to animation observers at all. I'll file a separate bug for it.
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #8) > I'll file a separate bug for it. That would be bug 1168688.
Assignee | ||
Comment 10•9 years ago
|
||
David, I've been trying to spec the order in which animations get returned from AnimationTimeline.getAnimations() as well as Element.getAnimations() but I'm a bit unsure about transitions. My proposal is that for transitions it's something like: 1. Sort by when the transition was generated (we can probably use the animation generation in the restyle manager for this?). 2. For transitions generated by the same style change event, sort by document order (with further spec text clarifying the position of :before and :after pseudo-elements). 3. For transitions generated in the same style change event and for the same target (pseudo-)element, sort by the name of the property being transitioned. Step 3 seems pretty arbitrary but I can't think of anything better (and I'd rather not let this ordering be implementation-dependent). We've been talking about exposing transitions as CSSTransition objects (a subclass of Animation) that include a 'transitionProperty' property so we'll have to define the exact string for that property (e.g. no shorthands, no escapes etc.). Assuming we have that defined, the sorting could be based on the Unicode codepoints of the transitionProperty string such that '-moz-outline-radius-topright' sorts before 'background-color'. Does that make sense? And if so, do you have any suggestions about how to efficiently sort the transitions? (Initially we're only going to sort on calls to getAnimation() so sorting by string on each call might be acceptable. However, in future, assuming we implement this part, it will be possible to mutate a CSSTransition object such that it animates properties other than just the transitionProperty (the transitionProperty itself wouldn't change however). In that case we'd need to determine the priority of transitions during a regular sample in case one or more of the mutated CSSTransitions targets the same property. Perhaps we could just have a static hashmap from nsCSSProperty to sort order?) There are some more details here: https://github.com/w3c/web-animations/issues/62
Flags: needinfo?(dbaron)
Assignee | ||
Updated•9 years ago
|
Component: DOM → DOM: Animation
Assignee | ||
Comment 11•9 years ago
|
||
Moved sorting behavior to bug 1171817 so it can land separately (and because it turned into a lot of work).
Flags: needinfo?(dbaron)
Assignee | ||
Comment 12•9 years ago
|
||
Bug 1150810 part 1 - Move DocumentTimeline methods up to AnimationTimeline This is not strictly necessary yet but we will want to implement methods like GetAnimations() on the base class, AnimationTimeline, so we may as well do that now rather than adding that code to DocumentTimeline and moving it later.
Attachment #8622161 -
Flags: review?(jwatt)
Assignee | ||
Comment 13•9 years ago
|
||
Bug 1150810 part 2 - Replace references to DocumentTimeline with AnimationTimeline This is needed not only for supporting other kinds of timelines, but also for when we come to implement SetTimeline(AnimationTimeline* aTimeline).
Attachment #8622162 -
Flags: review?(jwatt)
Assignee | ||
Comment 14•9 years ago
|
||
Bug 1150810 part 3 - Make IsPossiblyOrphanedPendingAnimation return true when there is no rendered doc This appears to be an existing bug.
Attachment #8622163 -
Flags: review?(jwatt)
Assignee | ||
Comment 15•9 years ago
|
||
Bug 1150810 part 4 - Store global on Animation The connection between an Animation and an AnimationTimeline is optional. That is, it is possible to have an Animation without an AnimationTimeline. Until now we have often just assumed the timeline will be set but eventually we need to support the possibility of the timeline being null. Indeed, later in this patch series we will set the timeline out-of-band (i.e. not in the constructor) using SetTimeline which opens up the possibility that timeline will be null for a period of time. This patch paves the way for having an optional timeline by storing the global used for, e.g. creating promises, on the Animation object itself.
Attachment #8622164 -
Flags: review?(bugs)
Assignee | ||
Comment 16•9 years ago
|
||
Bug 1150810 part 5 - Handle Timeline() returning null
Attachment #8622165 -
Flags: review?(jwatt)
Assignee | ||
Comment 17•9 years ago
|
||
Bug 1150810 part 6 - Rename Timeline() to GetTimeline() This is in anticipation of adding Animation::SetTimeline(). Once we set the timeline out of band, there's a chance that getting it could return null so this patch makes the WebIDL and method name reflect that.
Attachment #8622166 -
Flags: review?(bugs)
Assignee | ||
Comment 18•9 years ago
|
||
Bug 1150810 part 7 - Add Animation::SetTimeline
Attachment #8622167 -
Flags: review?(jwatt)
Assignee | ||
Comment 19•9 years ago
|
||
Bug 1150810 part 8 - Add AnimationTimeline::AddAnimations/RemoveAnimations
Attachment #8622168 -
Flags: review?(jwatt)
Assignee | ||
Comment 20•9 years ago
|
||
Bug 1150810 part 9 - Add relevant animations to timeline We only store relevant animations on the timeline. Relevant animations are any animations that are running or yet to run ("current animations") or which have finished but are still applying a fill mode ("in effect animations"). AnimationTimeline.getAnimations() only ever returns relevant animations so this is the minimum set we need to keep track of. Keeping track of any more than this would prevent us from garbage-collecting any no longer relevant animations since we keep a strong reference to this animations. The reason we keep a strong reference is that if an animation is attached to a timeline, even if there are no references to it from script or markup it needs to be kept alive in order to dispatch events or resolve promises. An irrelevant animation however is not going to do either of these things without outside intervention so we don't need to keep it alive.
Attachment #8622169 -
Flags: review?(jwatt)
Assignee | ||
Comment 21•9 years ago
|
||
Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations This patch also removed the (commented-out) play() method from the AnimationTimeline.webidl since it has been removed from the spec.
Attachment #8622170 -
Flags: review?(jwatt)
Assignee | ||
Comment 22•9 years ago
|
||
Bug 1150810 part 11 - Add some assertions to AnimationTimeline::GetAnimations
Attachment #8622171 -
Flags: review?(jwatt)
Assignee | ||
Comment 23•9 years ago
|
||
Bug 1150810 part 12 - Flush styles in AnimationTimeline::GetAnimations()
Attachment #8622172 -
Flags: review?(jwatt)
Assignee | ||
Comment 24•9 years ago
|
||
Bug 1150810 part 13 - Sort the result of AnimationTimeline::GetAnimations
Attachment #8622173 -
Flags: review?(jwatt)
Assignee | ||
Comment 25•9 years ago
|
||
Bug 1150810 part 14 - Don't return animations targetting pseudo-elements
Attachment #8622174 -
Flags: review?(jwatt)
Assignee | ||
Comment 26•9 years ago
|
||
Bug 1150810 part 15 - Add a comment about need to store more than just relevant animations We'll likely address this as part of bug 1151731 when we sample animations from their timeline.
Attachment #8622175 -
Flags: review?(jwatt)
Assignee | ||
Comment 27•9 years ago
|
||
Bug 1150810 part 16 - Add tests for AnimationTimeline.getAnimations()
Attachment #8622176 -
Flags: review?(jwatt)
Assignee | ||
Comment 29•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d30c838669
Comment 30•9 years ago
|
||
Comment on attachment 8622164 [details] MozReview Request: Bug 1150810 part 4 - Store global on Animation; r=smaug, jwatt I would prefer if null check for mGlobal was added before Promise::Create(mGlobal, aRv);, since ::Create crashes on debug builds at least of mGlobal is null. (and currently we have the null check in CreatePromise(AnimationTimeline* aTimeline, ErrorResult& aRv)) So if (!mReady) { -> if (!mReady && mGlobal) { etc.
Attachment #8622164 -
Flags: review?(bugs) → review+
Updated•9 years ago
|
Attachment #8622166 -
Flags: review?(bugs) → review+
![]() |
||
Updated•9 years ago
|
Attachment #8622161 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 31•9 years ago
|
||
Comment on attachment 8622161 [details] MozReview Request: Bug 1150810 part 1 - Move DocumentTimeline methods up to AnimationTimeline; r=jwatt https://reviewboard.mozilla.org/r/11175/#review9833 ::: dom/animation/AnimationTimeline.h:51 (Diff revision 1) > + * Returns true if the times returned by GetCurrentTime() are convertible > + * to and from wallclock-based TimeStamp (e.g. from TimeStamp::Now()) values > + * using ToTimelineTime() and ToTimeStamp(). Can you expand on this comment to note that under normal circumstances this will return true, and not test control as a specific circumstance when it will not?
![]() |
||
Comment 32•9 years ago
|
||
Comment on attachment 8622162 [details] MozReview Request: Bug 1150810 part 2 - Replace references to DocumentTimeline with AnimationTimeline; r=jwatt https://reviewboard.mozilla.org/r/11177/#review9837 Ship It!
Attachment #8622162 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 33•9 years ago
|
||
Comment on attachment 8622163 [details] MozReview Request: Bug 1150810 part 3 - Make IsPossiblyOrphanedPendingAnimation return true when there is no rendered doc; r=jwatt https://reviewboard.mozilla.org/r/11179/#review9839 Ship It!
Attachment #8622163 -
Flags: review?(jwatt) → review+
![]() |
||
Updated•9 years ago
|
Attachment #8622164 -
Flags: review+
![]() |
||
Comment 34•9 years ago
|
||
Comment on attachment 8622164 [details] MozReview Request: Bug 1150810 part 4 - Store global on Animation; r=smaug, jwatt https://reviewboard.mozilla.org/r/11181/#review9841 Ship It!
![]() |
||
Comment 35•9 years ago
|
||
Comment on attachment 8622165 [details] MozReview Request: Bug 1150810 part 5 - Handle Timeline() returning null; r=jwatt https://reviewboard.mozilla.org/r/11183/#review9843 Ship It!
Attachment #8622165 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 36•9 years ago
|
||
Comment on attachment 8622167 [details] MozReview Request: Bug 1150810 part 7 - Add Animation::SetTimeline; r=jwatt https://reviewboard.mozilla.org/r/11187/#review9845 Ship It!
Attachment #8622167 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 37•9 years ago
|
||
Comment on attachment 8622168 [details] MozReview Request: Bug 1150810 part 8 - Add AnimationTimeline::AddAnimations/RemoveAnimations; r=jwatt https://reviewboard.mozilla.org/r/11189/#review9847 Ship It!
Attachment #8622168 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 38•9 years ago
|
||
Comment on attachment 8622169 [details] MozReview Request: Bug 1150810 part 9 - Add relevant animations to timeline; r=jwatt https://reviewboard.mozilla.org/r/11191/#review9849 ::: dom/animation/Animation.cpp:798 (Diff revision 1) > + // than not) the work done by AnimationTimeline/DocumentTimeline is still > + // negligible and its easier than trying to detect whenever we have a > + // transition to/from relevant. The word "transition" could be a tiny bit confusing. How about "whenever we are switching to/from being relevant"? Also, I think a lot more of the main commit message should be copied to this location. Particularly the bit about wanting to be able to garbage collect animations that aren't needed any more.
Attachment #8622169 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 39•9 years ago
|
||
Comment on attachment 8622170 [details] MozReview Request: Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations; r=jwatt https://reviewboard.mozilla.org/r/11193/#review9851 Remember you'll also need review from a DOM peer. ::: dom/animation/AnimationTimeline.cpp:23 (Diff revision 1) > +AddAnimationToSequence(nsRefPtrHashKey<dom::Animation>* aKey, void* aSequence) Maybe call this AppendAnimationToSequence to be a bit clearer?
Attachment #8622170 -
Flags: review?(jwatt) → review+
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8622162 -
Attachment description: MozReview Request: Bug 1150810 part 2 - Replace references to DocumentTimeline with AnimationTimeline → MozReview Request: Bug 1150810 part 2 - Replace references to DocumentTimeline with AnimationTimeline; r=jwatt
Attachment #8622162 -
Flags: review+ → review?(jwatt)
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8622163 -
Attachment description: MozReview Request: Bug 1150810 part 3 - Make IsPossiblyOrphanedPendingAnimation return true when there is no rendered doc → MozReview Request: Bug 1150810 part 3 - Make IsPossiblyOrphanedPendingAnimation return true when there is no rendered doc; r=jwatt
Attachment #8622163 -
Flags: review+ → review?(jwatt)
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8622164 -
Attachment description: MozReview Request: Bug 1150810 part 4 - Store global on Animation → MozReview Request: Bug 1150810 part 4 - Store global on Animation; r=smaug, jwatt
Attachment #8622164 -
Flags: review?(jwatt)
Attachment #8622164 -
Flags: review?(bugs)
Attachment #8622164 -
Flags: review+
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8622170 -
Attachment description: MozReview Request: Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations → MozReview Request: Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations; r=jwatt
Attachment #8622170 -
Flags: review+ → review?(jwatt)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 56•9 years ago
|
||
Oh, ReviewBoard, why did you do that? Why? I incorporated the review feedback, pushed and published (exactly like the docs told me to) and it spammed Bugzilla, re-requested review, and didn't resolve the issues. :/
Assignee | ||
Updated•9 years ago
|
Attachment #8622161 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622162 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622163 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622164 -
Flags: review?(jwatt)
Attachment #8622164 -
Flags: review?(bugs)
Attachment #8622164 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622165 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622166 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622168 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622167 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8622170 -
Flags: review?(jwatt) → review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8622169 -
Flags: review?(jwatt) → review+
Comment 57•9 years ago
|
||
Comment on attachment 8622170 [details] MozReview Request: Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations; r=jwatt r+ for the webidl. I don't know what kind of thing mAnimations is, but make sure the data structure doesn't have iterators - or if it does have, use such and not enumerate. Some data structures have got iterators lately.
Attachment #8622170 -
Flags: review?(bugs) → review+
![]() |
||
Comment 58•9 years ago
|
||
Comment on attachment 8622171 [details] MozReview Request: Bug 1150810 part 11 - Add some assertions to AnimationTimeline::GetAnimations https://reviewboard.mozilla.org/r/11195/#review9965 ::: dom/animation/AnimationTimeline.cpp:26 (Diff revision 2) > + AnimationTimeline* mTimeline; Maybe ifdef DEBUG it then.
Attachment #8622171 -
Flags: review?(jwatt)
![]() |
||
Updated•9 years ago
|
Attachment #8622171 -
Flags: review+
![]() |
||
Comment 59•9 years ago
|
||
Comment on attachment 8622171 [details] MozReview Request: Bug 1150810 part 11 - Add some assertions to AnimationTimeline::GetAnimations https://reviewboard.mozilla.org/r/11195/#review9967 Ship It!
![]() |
||
Comment 60•9 years ago
|
||
Comment on attachment 8622172 [details] MozReview Request: Bug 1150810 part 12 - Flush styles in AnimationTimeline::GetAnimations() https://reviewboard.mozilla.org/r/11197/#review9969 Ship It!
Attachment #8622172 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 61•9 years ago
|
||
Comment on attachment 8622173 [details] MozReview Request: Bug 1150810 part 13 - Sort the result of AnimationTimeline::GetAnimations https://reviewboard.mozilla.org/r/11199/#review9971 ::: dom/animation/AnimationTimeline.cpp:62 (Diff revision 2) > + aAnimations.Sort(AnimationPtrComparator<nsRefPtr<Animation>>()); Please add a comment to note why we sort.
Attachment #8622173 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 62•9 years ago
|
||
Comment on attachment 8622174 [details] MozReview Request: Bug 1150810 part 14 - Don't return animations targetting pseudo-elements https://reviewboard.mozilla.org/r/11201/#review9973 Ship It!
Attachment #8622174 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 63•9 years ago
|
||
Comment on attachment 8622175 [details] MozReview Request: Bug 1150810 part 15 - Add a comment about need to store more than just relevant animations https://reviewboard.mozilla.org/r/11203/#review9975 Ship It!
Attachment #8622175 -
Flags: review?(jwatt) → review+
![]() |
||
Comment 64•9 years ago
|
||
Comment on attachment 8622176 [details] MozReview Request: Bug 1150810 part 16 - Add tests for AnimationTimeline.getAnimations() https://reviewboard.mozilla.org/r/11205/#review9979 Ship It!
Attachment #8622176 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 65•9 years ago
|
||
https://reviewboard.mozilla.org/r/11199/#review10653 > Please add a comment to note why we sort. I've just added the following comment (locally, since I'm afraid of MozReview spamming the world if I update this review): // Sort by priority I could say something about them otherwise being random since we're iterating over the hashmap but the fact that the spec requires them to be sorted by priority is the real reason.
Assignee | ||
Comment 66•9 years ago
|
||
https://reviewboard.mozilla.org/r/11195/#review10649 > Maybe ifdef DEBUG it then. If we do that, we'll have to add ifdefs where we interact with that struct as well. e.g. #ifdef DEBUG AddAnimationParams params{ aAnimations, this }; #else AddAnimationParams params{ aAnimations }; #endif That seems a bit awkward to me but what do you think?
Assignee | ||
Comment 67•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #57) > Comment on attachment 8622170 [details] > MozReview Request: Bug 1150810 part 10 - Add > AnimationTimeline::GetAnimations; r=jwatt > > r+ for the webidl. > > I don't know what kind of thing mAnimations is, but make sure the data > structure > doesn't have iterators - or if it does have, use such and not enumerate. > Some data structures have got iterators lately. It's an nsTHashtable which as far as I can tell it doesn't have iterators.
Assignee | ||
Comment 68•9 years ago
|
||
https://reviewboard.mozilla.org/r/11195/#review9965 > If we do that, we'll have to add ifdefs where we interact with that struct as well. > > e.g. > > #ifdef DEBUG > AddAnimationParams params{ aAnimations, this }; > #else > AddAnimationParams params{ aAnimations }; > #endif > > That seems a bit awkward to me but what do you think? I ended up just doing this anyway.
Comment 69•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e35b93e162 https://hg.mozilla.org/integration/mozilla-inbound/rev/be9483065496 https://hg.mozilla.org/integration/mozilla-inbound/rev/efeb4afcb664 https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd85885ae3f https://hg.mozilla.org/integration/mozilla-inbound/rev/1781b7760ddb https://hg.mozilla.org/integration/mozilla-inbound/rev/f1596c2f4908 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e356e42ae0c https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa25e5dd430 https://hg.mozilla.org/integration/mozilla-inbound/rev/09689218420f https://hg.mozilla.org/integration/mozilla-inbound/rev/771b893686af https://hg.mozilla.org/integration/mozilla-inbound/rev/865b4e9c2ea3 https://hg.mozilla.org/integration/mozilla-inbound/rev/85a593b6fe42 https://hg.mozilla.org/integration/mozilla-inbound/rev/97e547e727dd https://hg.mozilla.org/integration/mozilla-inbound/rev/b70816b8015a https://hg.mozilla.org/integration/mozilla-inbound/rev/494bf6250ab2 https://hg.mozilla.org/integration/mozilla-inbound/rev/f293aaab3541
Comment 70•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/59e35b93e162 https://hg.mozilla.org/mozilla-central/rev/be9483065496 https://hg.mozilla.org/mozilla-central/rev/efeb4afcb664 https://hg.mozilla.org/mozilla-central/rev/7dd85885ae3f https://hg.mozilla.org/mozilla-central/rev/1781b7760ddb https://hg.mozilla.org/mozilla-central/rev/f1596c2f4908 https://hg.mozilla.org/mozilla-central/rev/6e356e42ae0c https://hg.mozilla.org/mozilla-central/rev/eaa25e5dd430 https://hg.mozilla.org/mozilla-central/rev/09689218420f https://hg.mozilla.org/mozilla-central/rev/771b893686af https://hg.mozilla.org/mozilla-central/rev/865b4e9c2ea3 https://hg.mozilla.org/mozilla-central/rev/85a593b6fe42 https://hg.mozilla.org/mozilla-central/rev/97e547e727dd https://hg.mozilla.org/mozilla-central/rev/b70816b8015a https://hg.mozilla.org/mozilla-central/rev/494bf6250ab2 https://hg.mozilla.org/mozilla-central/rev/f293aaab3541
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•