Closed Bug 1150810 Opened 9 years ago Closed 9 years ago

Implement AnimationTimeline.getAnimations()

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(18 files)

WIP patch v1
39.27 KB, patch
Details | Diff | Splinter Review
Call cancel from property destructor
3.87 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1150810 part 1 - Move DocumentTimeline methods up to AnimationTimeline; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 2 - Replace references to DocumentTimeline with AnimationTimeline; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 3 - Make IsPossiblyOrphanedPendingAnimation return true when there is no rendered doc; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 4 - Store global on Animation; r=smaug, jwatt
40 bytes, text/x-review-board-request
birtles
: review+
birtles
: review+
Details
MozReview Request: Bug 1150810 part 5 - Handle Timeline() returning null; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 6 - Rename Timeline() to GetTimeline(); r=smaug
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 7 - Add Animation::SetTimeline; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 8 - Add AnimationTimeline::AddAnimations/RemoveAnimations; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 9 - Add relevant animations to timeline; r=jwatt
40 bytes, text/x-review-board-request
birtles
: review+
Details
MozReview Request: Bug 1150810 part 10 - Add AnimationTimeline::GetAnimations; r=jwatt
40 bytes, text/x-review-board-request
smaug
: review+
Details
MozReview Request: Bug 1150810 part 11 - Add some assertions to AnimationTimeline::GetAnimations
40 bytes, text/x-review-board-request
jwatt
: review+
Details
MozReview Request: Bug 1150810 part 12 - Flush styles in AnimationTimeline::GetAnimations()
40 bytes, text/x-review-board-request
jwatt
: review+
Details
MozReview Request: Bug 1150810 part 13 - Sort the result of AnimationTimeline::GetAnimations
40 bytes, text/x-review-board-request
jwatt
: review+
Details
MozReview Request: Bug 1150810 part 14 - Don't return animations targetting pseudo-elements
40 bytes, text/x-review-board-request
jwatt
: review+
Details
MozReview Request: Bug 1150810 part 15 - Add a comment about need to store more than just relevant animations
40 bytes, text/x-review-board-request
jwatt
: review+
Details
MozReview Request: Bug 1150810 part 16 - Add tests for AnimationTimeline.getAnimations()
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.
Depends on: 1151731
Depends on: 1152171
Attached patch WIP patch v1 — — Splinter Review 
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
Note that most of the refactoring outlined in comment 1 is happening under bug 1151731 although the renaming already happened in bug 1152171.
Summary: Implement DocumentTimeline.getAnimations() → Implement AnimationTimeline.getAnimations()
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)
Patch to reproduce the issue
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)
(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!
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).
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.
(In reply to Brian Birtles (:birtles) from comment #8)
> I'll file a separate bug for it.

That would be bug 1168688.
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)
Component: DOM → DOM: Animation
Depends on: 1171817
Moved sorting behavior to bug 1171817 so it can land separately (and because it turned into a lot of work).
Flags: needinfo?(dbaron)
Depends on: 1174575
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)
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)
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)
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)
Bug 1150810 part 5 - Handle Timeline() returning null
Attachment #8622165 - Flags: review?(jwatt)
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)
Bug 1150810 part 7 - Add Animation::SetTimeline
Attachment #8622167 - Flags: review?(jwatt)
Bug 1150810 part 8 - Add AnimationTimeline::AddAnimations/RemoveAnimations
Attachment #8622168 - Flags: review?(jwatt)
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)
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)
Bug 1150810 part 11 - Add some assertions to AnimationTimeline::GetAnimations
Attachment #8622171 - Flags: review?(jwatt)
Bug 1150810 part 12 - Flush styles in AnimationTimeline::GetAnimations()
Attachment #8622172 - Flags: review?(jwatt)
Bug 1150810 part 13 - Sort the result of AnimationTimeline::GetAnimations
Attachment #8622173 - Flags: review?(jwatt)
Bug 1150810 part 14 - Don't return animations targetting pseudo-elements
Attachment #8622174 - Flags: review?(jwatt)
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)
Bug 1150810 part 16 - Add tests for AnimationTimeline.getAnimations()
Attachment #8622176 - Flags: review?(jwatt)
These patches need the changes from bug 1171817.
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+
Attachment #8622166 - Flags: review?(bugs) → review+
Attachment #8622161 - Flags: review?(jwatt) → review+
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 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 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+
Attachment #8622164 - Flags: review+
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 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 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 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 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 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+
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)
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)
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+
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)
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. :/
Attachment #8622161 - Flags: review?(jwatt) → review+
Attachment #8622162 - Flags: review?(jwatt) → review+
Attachment #8622163 - Flags: review?(jwatt) → review+
Attachment #8622164 - Flags: review?(jwatt)
Attachment #8622164 - Flags: review?(bugs)
Attachment #8622164 - Flags: review+
Attachment #8622165 - Flags: review?(jwatt) → review+
Attachment #8622166 - Flags: review?(bugs) → review+
Attachment #8622168 - Flags: review?(jwatt) → review+
Attachment #8622167 - Flags: review?(jwatt) → review+
Attachment #8622170 - Flags: review?(jwatt) → review?(bugs)
Attachment #8622169 - Flags: review?(jwatt) → review+
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 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)
Attachment #8622171 - Flags: review+
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 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 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 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 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 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+
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.
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?
(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.
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.
You need to log in before you can comment on or make changes to this bug.