Hi David, I'm assigning most of these patches to you for now since I thought you'd like to know what's going on here. Feel free to re-assign if you're busy however. I think bz/heycam/dholbert would also be suitable reviewers.

Oops, uploaded part 2 before part 1

AnimationTimeline::GetCurrentTime returns the time value of the timeline as a double. For internal calculations however it is more useful to get this as a mozilla::TimeStamp. This patch splits the calculation of the current time into two stages: calculation as a timestamp then conversion to a double.

With both this and part 3 I'm not sure how useful the MOZ_LIKELY macros are, and how liberal we should be with these

This patch adds the WebIDL definitions and implementation of getAnimationPlayers on Element. It does not included the full definition of AnimationPlayer but only readonly versions of the currentTime and startTime attributes since these are easy to implement and enable identifying the different animations that are returned for the sake of testing. Web Animations defines getAnimationPlayers as only returning the animations that are either running or will run in the future (known as "current" animations). This will likely change since it seems desirable to be able query animations that have finished but are applying a forwards fill. For now, however, this patch makes us only return animations that have not finished. This patch also removes an assertion in ElementAnimation::GetLocalTime that would fail if called on a finished transition. This assertion is no longer necessary since an earlier patch in this series removed the overloading of the animation start time that meant calling this on a finished transition was unsafe. Furthermore, this assertion, if it were not removed, would fail if script holds onto a transition and queries its start time after it completed.

With these tests I noticed that other tests for transitions tend to use style.setProperty('left'...) rather than style.left but it wasn't clear to me what the advantage is of this so I stuck with style.left since it seems simpler to me.

Comment on attachment 8448513 [details] [diff] [review] part 5 - Add GetAnimationPlayers to Element > It does not included the full definition of AnimationPlayer but only readonly s/included/include/ >+nsINode::GetAnimationPlayers(nsTArray<nsRefPtr<ElementAnimation> >& aPlayers) Why put this on nsINode and not on Element? >+ if (!document) { OwnerDoc() is never null. What is this check actually trying to test for? >+++ b/dom/bindings/Bindings.conf Might be nice to name this class mozilla::dom::AnimationPlayer and put it in the appropriate header... Ah, well. >+ // |now| should only be null in very unusual circumstances such as when >+ // we can't get a prescontext. Note that on the web this is not very unusual... I'd like to understand what the OwnerDoc() story is.

Comment on attachment 8448513 [details] [diff] [review] part 5 - Add GetAnimationPlayers to Element Also, this is missing AnimationPlayer.webidl, right?

Address review feedback

(In reply to Boris Zbarsky [:bz] from comment #7) > Comment on attachment 8448513 [details] [diff] [review] > part 5 - Add GetAnimationPlayers to Element > > > It does not included the full definition of AnimationPlayer but only readonly > > s/included/include/ Fixed. > >+nsINode::GetAnimationPlayers(nsTArray<nsRefPtr<ElementAnimation> >& aPlayers) > > Why put this on nsINode and not on Element? Fixed. > >+ if (!document) { > > OwnerDoc() is never null. What is this check actually trying to test for? I was just blindly copying code from nsINode::DispatchEvent. Fixed. > >+++ b/dom/bindings/Bindings.conf > > Might be nice to name this class mozilla::dom::AnimationPlayer and put it in > the appropriate header... Ah, well. I definitely want to do that eventually. I just wasn't quite sure how best to approach this since currently ElementAnimation fulfills the role of AnimationPlayer, Animation, and AnimationTiming hence I filed: https://ask.mozilla.org/question/856/what-is-the-state-of-the-art-when-implementing-dom-interfaces-with-moving-parts/ I'm more than happy to move ElementAnimation to AnimationPlayer now though. > >+ // |now| should only be null in very unusual circumstances such as when > >+ // we can't get a prescontext. > > Note that on the web this is not very unusual... This just shows my ignorance. Fixed. > I'd like to understand what the OwnerDoc() story is. Again, just ignorance. (In reply to Boris Zbarsky [:bz] from comment #8) > Comment on attachment 8448513 [details] [diff] [review] > part 5 - Add GetAnimationPlayers to Element > > Also, this is missing AnimationPlayer.webidl, right? Aah, yes. Oops. Fixed. I've also added Animatable.webidl. I wanted to mark the following declaration so that it only applies when dom.animations-api.core.enabled is set: Element implements Animatable; However simply sticking '[Pref="dom.animations-api.core.enabled"]' before it didn't work so I simply put it before the only member in that interface (which is [NoInterfaceObject] so I presume the empty interface won't show up in the prototype chain).

Comment on attachment 8449157 [details] [diff] [review] part 5 - Add GetAnimationPlayers to Element > I was just blindly copying code from nsINode::DispatchEvent. Yeah, I dunno what that code is doing and why. It might predate OwnerDoc never returning null. > hence I filed: Right. I'm hoping to answer that one tomorrow, fwiw; I just didn't get to it today. > so I simply put it before the only member in that interface That's the right thing to do, yes. "implements" statements do not affect the prototype chain; they basically just pull in all the members of the right-hand side onto the prototype object of the left-hand side. >+ [Pure] readonly attribute double currentTime; I'm not entirely sure it should be [Pure]. This only updates on refresh ticks, right? Maybe that's ok. r=me

(In reply to Boris Zbarsky [:bz] from comment #11) > >+ [Pure] readonly attribute double currentTime; > > I'm not entirely sure it should be [Pure]. This only updates on refresh > ticks, right? Maybe that's ok. It fulfills the first condition, "Used to flag attributes whose getter has no side-effects", from [1] but I probably misunderstood the second one, "Attributes/methods flagged in this way promise that they will keep returning the same value as long as no DOM setters or non-[Pure] DOM methods executed", as being restricted to a single execution context. I suppose once we start associating players with other types of animation timelines this value might change even within a single execution context so it's probably better just to drop this to begin with. [1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

Comment on attachment 8449876 [details] [diff] [review] part 7 - Add AnimationPlayer to test_interfaces.html r=me

Comment on attachment 8448510 [details] [diff] [review] part 1 - Don't overload start time for marking finished transitions r=dbaron

Comment on attachment 8448509 [details] [diff] [review] part 2 - Add a timeline member to ElementAnimations I'm a little bit surprised that they're *not* going to need a pointer to the element, if they're going to be exposed to script and might be held alive by that script. Or are they? And if they are, would it make more sense to hold the element than the timeline? (Will the element keep the timeline alive by keeping the document alive?)

Comment on attachment 8448511 [details] [diff] [review] part 3 - Add AnimationTimeline::GetCurrentTimeStamp >+ if (!MOZ_UNLIKELY(presShell)) { > return result; >+ } > > nsPresContext* presContext = presShell->GetPresContext(); >- if (!presContext) >+ if (!MOZ_UNLIKELY(presContext)) { > return result; >+ } You want the ! inside the MOZ_UNLIKELY in both cases. r=dbaron with that

Comment on attachment 8448512 [details] [diff] [review] part 4 - Add AnimationTimeline::ToTimelineTime helper method r=dbaron

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #16) > Comment on attachment 8448509 [details] [diff] [review] > part 2 - Add a timeline member to ElementAnimations > > I'm a little bit surprised that they're *not* going to need a pointer to the > element, if they're going to be exposed to script and might be held alive by > that script. > > Or are they? And if they are, would it make more sense to hold the element > than the timeline? (Will the element keep the timeline alive by keeping the > document alive?) Yes, you're right. They will eventually need a pointer to the element (since there is a "target" member on Animation) so we just go ahead and add that now and navigate to the timeline from the element. I'll post an updated patch later today.

Comment on attachment 8448514 [details] [diff] [review] part 6 - Add tests for Element.getAnimationPlayers r=dbaron (I didn't look all that closely; let me know if there's something in particular you want me to look at.) (style.setProperty('left',...) vs. style.left setter doesn't make much difference, although the former should give a CSS warning if there's a typo in the property name)

(In reply to Brian Birtles (:birtles) from comment #19) > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from > comment #16) > > Comment on attachment 8448509 [details] [diff] [review] > > part 2 - Add a timeline member to ElementAnimations > > > > I'm a little bit surprised that they're *not* going to need a pointer to the > > element, if they're going to be exposed to script and might be held alive by > > that script. > > > > Or are they? And if they are, would it make more sense to hold the element > > than the timeline? (Will the element keep the timeline alive by keeping the > > document alive?) > > Yes, you're right. They will eventually need a pointer to the element (since > there is a "target" member on Animation) so we just go ahead and add that > now and navigate to the timeline from the element. Actually, on further thought, we will eventually need both since we plan to support timelines other than the document timeline (such as scroll-based timelines). Also, technically an element can be animated by a document timeline other than its own. So I think I'd rather leave the reference to the timeline for now. (Technically in Web Animations terms, AnimationPlayers have a reference to a timeline and Animations have a reference to a player and a target element. It's just that currently we only have players, hence the slightly odd arrangement.) Thanks for all the reviews!

(In reply to Brian Birtles (:birtles) from comment #21) > Actually, on further thought, we will eventually need both [Element and Timeline] since we plan to > support timelines other than the document timeline (such as scroll-based > timelines). Also, technically an element can be animated by a document > timeline other than its own. So I think I'd rather leave the reference to > the timeline for now. > > (Technically in Web Animations terms, AnimationPlayers have a reference to a > timeline and Animations have a reference to a player and a target element. > It's just that currently we only have players, hence the slightly odd > arrangement.) David, what do you think?

Comment on attachment 8448509 [details] [diff] [review] part 2 - Add a timeline member to ElementAnimations >+ ElementAnimation(dom::AnimationTimeline* aTimeline) add |explicit| keyword >+ ElementPropertyTransition(mozilla::dom::AnimationTimeline* aTimeline) add |explicit| keyword r=dbaron with that; sorry for the delay

https://hg.mozilla.org/integration/mozilla-inbound/rev/389f489afdb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6065670318 https://hg.mozilla.org/integration/mozilla-inbound/rev/35f359aca0c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/08f2190d7e40 https://hg.mozilla.org/integration/mozilla-inbound/rev/e672d5bffa8f https://hg.mozilla.org/integration/mozilla-inbound/rev/f681694c9df1 https://hg.mozilla.org/integration/mozilla-inbound/rev/42e8d9fd94e0

https://hg.mozilla.org/mozilla-central/rev/389f489afdb6 https://hg.mozilla.org/mozilla-central/rev/5a6065670318 https://hg.mozilla.org/mozilla-central/rev/35f359aca0c3 https://hg.mozilla.org/mozilla-central/rev/08f2190d7e40 https://hg.mozilla.org/mozilla-central/rev/e672d5bffa8f https://hg.mozilla.org/mozilla-central/rev/f681694c9df1 https://hg.mozilla.org/mozilla-central/rev/42e8d9fd94e0