This patch fails the test - for some reason, after the call to atan2, we no longer do any restyle for the canvas element, so although the animation code computes the correct time values, we don't do any sampling. Brian, do you have any ideas for why this would happen? Perhaps there is some flag that we set when updating animation play state that we should also be setting inside AnimationPlayer::Pause/Play?

Nevermind, just needed to post a restyle event. There's still something wrong with pausing though.

Comment on attachment 8494009 [details] [diff] [review] Patch Review of attachment 8494009 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/AnimationPlayer.cpp @@ +75,5 @@ > + mHoldTime.SetNull(); > + nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(), > + eRestyle_Self, > + nsChangeHint_AllReflowHints); > +} I'm going to check if I can use less powerful hints here.

Comment on attachment 8494009 [details] [diff] [review] Patch Review of attachment 8494009 [details] [diff] [review]: ----------------------------------------------------------------- It looks mostly good but I want to check: a) what your thoughts are about throwing exceptions when the timeline is null / has a null current time b) what you think about splitting off a CSS subclass c) more thorough test coverage ::: dom/animation/AnimationPlayer.cpp @@ +60,5 @@ > + if (!mTimeline || mTimeline->GetCurrentTimeDuration().IsNull()) { > + mStartTime.SetNull(); > + rv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > + return; > + } I think the spec needs to be fixed here. Currently it's possible to set the current time of an animation without a timeline (or with an inactive timeline). The use case is a purely script-driven animation. However, if you pause an animation, then clear its timeline, I think it should still be possible to unpause it so that when you associate it with a new timeline it starts playing immediately. So I think play() and pause() shouldn't throw when there's no timeline or the timeline has a null time. Instead, I think they should just update the pause state. What do you think? It would probably simplify some of the other parts of this patch if we didn't have to check for possible failure. I might go ahead and try to update the spec later today and see if this approach works. @@ +83,5 @@ > +{ > + if (!mTimeline || mTimeline->GetCurrentTimeDuration().IsNull()) { > + rv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > + return; > + } Here too, I think the spec may be wrong and this should not throw. ::: dom/animation/AnimationPlayer.h @@ +77,5 @@ > bool mIsRunningOnCompositor; > > nsRefPtr<AnimationTimeline> mTimeline; > nsRefPtr<Animation> mSource; > + bool mPausedByCSS; Can we make this protected and add some wrapper methods? void PauseFromCSS() <-- sets the flag and calls Pause() void PlayFromCSS() <-- clears the flag and calls Play() bool PausedByCSS() const It would be good to group these together with a comment explaining that they are for CSS-animations only (and may be split off into a separate subclass later) and the relationship between mPaused and mPausedByCSS. (Also, if possible I guess the bools should go together.) Actually, what do you think about splitting off the CSS Animations subclass now? ::: dom/webidl/AnimationPlayer.webidl @@ +27,5 @@ > readonly attribute Promise ready; > readonly attribute Promise finished; > void cancel (); > + void finish (); */ > + [Throws] These [Throws] annotations may not necessary depending on what you think of my comments earlier. ::: layout/style/nsAnimationManager.cpp @@ +313,5 @@ > + oldPlayer->mPausedByCSS = true; > + oldPlayer->Pause(rv); > + } else if (oldPlayer->mPausedByCSS && !newPlayer->IsPaused()) { > + oldPlayer->mPausedByCSS = false; > + oldPlayer->Play(rv); This seems good except two issues: 1. If an animation is paused by calling pause(), then we change animation-play-state to 'pause' and then back to 'running', do you expect the original call to pause() to "stick"? It's more specific (since animation-play-state might affect *many* animation players but pause() only affects one) and generally more specific actions override more general ones. But maybe in this case it's reasonable that animation-play-state wins? The author's intention is obvious and if they want a specific override they can use local style? What do you think? (A similar situation occurs when we call play() and then from the API set animation-play-state to running then back to paused) 2. When we introduce finishing behavior, play() can have the side-effect of rewinding the animation to the start. Suppose we call play() on an animation player that is paused using animation-play-state:paused, then *also* set animation-play-state: running some time later. I don't think you'd expect the animation to rewind in that case. That suggests that we should only call oldPlayer->Play() if *both* oldPlayer->mPausedByCSS is true *and* oldPlayer->mPaused is true. ::: layout/style/test/test_current-time.html @@ +1,1 @@ > +<!DOCTYPE HTML> Why is this called current-time? It seems like it is testing play state? @@ +4,5 @@ > +https://bugzilla.mozilla.org/show_bug.cgi?id=435442 > +--> > +<!-- > + > + ====== PLEASE KEEP THIS IN SYNC WITH test_animations_omta.html ======= Does this comment actually apply here? @@ +23,5 @@ > + @keyframes anim1 { > + 0% { margin-left: 0px } > + 50% { margin-left: 80px } > + 100% { margin-left: 100px } > + } It might be easier to just have an animation go from 0px to 100px (i.e. just two keyframes). That would make the logic further down easier to follow. @@ +54,5 @@ > + if (typeof(style) != "string") { > + ok(false, "test author forgot to pass argument"); > + } > + div = document.createElement(tagname); > + console.log(div); Not needed @@ +102,5 @@ > + cs = null; > + if (events_received.length) { > + ok(false, "caller should have called check_events"); > + } > +} Please move listen, check_events, new_div, done_div etc. into animation_utils.js rather than duplicating them here. We should probably adjust them at the same time to make them more reusable by, e.g. putting them in a closure and exporting only the methods so that local variables 'div' etc. don't get exported as globals. We could make new_div return 'div' so call sites could continue using it. (I'm pretty sure we support destructuring assignment so you could return 'div' and 'cs' as an array for call sites that need both.) This can happen as follow-up but I'd really prefer it happen first so that I know it does actually happen. @@ +110,5 @@ > + > +/* > + * css3-animations: 3.7. The 'animation-play-state' Property > + * http://dev.w3.org/csswg/css3-animations/#the-animation-play-state-property- > + */ Is this comment correct? It seems like these tests focus on the combination of animation-play-state and the Web Animations API. @@ +118,5 @@ > +div.style.animationTimingFunction = "linear"; > +div.style.animationName = "anim1"; > +div.style.animationDuration = "1s"; > +div.style.animationDirection = "alternate"; > +div.style.animationIterationCount = "2"; This can all be set as div.style.animation = "anim1 1s 2 linear alternate" or is there a reason to split it up? @@ +120,5 @@ > +div.style.animationDuration = "1s"; > +div.style.animationDirection = "alternate"; > +div.style.animationIterationCount = "2"; > +cs.marginLeft; > +var player = div.getAnimationPlayers()[0]; I think I'd like to make getAnimationPlayers() flush style so that the cs.marginLeft line is not needed. What do you think? @@ +121,5 @@ > +div.style.animationDirection = "alternate"; > +div.style.animationIterationCount = "2"; > +cs.marginLeft; > +var player = div.getAnimationPlayers()[0]; > +console.log(div.getAnimationPlayers()); Not needed. @@ +132,5 @@ > +is_approx(px_to_num(cs.marginLeft), 80 * 0.5, 0.01, > + "animation-play-state test 1 at 250ms"); > +advance_clock(250); > +is_approx(px_to_num(cs.marginLeft), 80 * 0.5, 0.01, > + "animation-play-state test 1 still at 500ms"); This comment doesn't make much sense? If we're reporting the animation time then it should be "still at 250ms" but if we're reporting the clock time then it should be just "at 500ms"? @@ +144,5 @@ > +is(cs.marginLeft, "100px", "animation-play-state test 1 at 1250ms"); > +advance_clock(250); > +is_approx(px_to_num(cs.marginLeft), 80 + 20 * 0.5, 0.01, > + "animation-play-state test 1 at 1500ms"); > +console.log('Pausing'); Remove @@ +145,5 @@ > +advance_clock(250); > +is_approx(px_to_num(cs.marginLeft), 80 + 20 * 0.5, 0.01, > + "animation-play-state test 1 at 1500ms"); > +console.log('Pausing'); > +player.pause(); What is this second call to pause() testing that we didn't already test? @@ +154,5 @@ > + "animation-play-state test 1 at 3500ms"); > +advance_clock(500); > +is_approx(px_to_num(cs.marginLeft), 80 + 20 * 0.5, 0.01, > + "animation-play-state test 1 at 4000ms"); > +console.log('Plauing'); Remove @@ +165,5 @@ > +advance_clock(250); > +is(cs.marginLeft, "0px", "animation-play-state test 1, at 4750ms"); > +advance_clock(250); > +is(cs.marginLeft, "0px", "animation-play-state test 1, at 5000ms"); > +done_div(); I think these tests aren't very comprehensive. I think we should be testing the various permutations of calling play()/pause() and setting div.animationPlayState but from what I can tell we only test play()/pause(). Also, we need to test the OMTA case. Particularly, we need to test that after setting animation-play-state: paused, calling play() triggers a layer transaction that puts the animation back on the compositor.

(In reply to Brian Birtles (:birtles) from comment #5) > Comment on attachment 8494009 [details] [diff] [review] > Patch > > > So I think play() and pause() shouldn't throw when there's no timeline or > the timeline has a null time. Instead, I think they should just update the > pause state. > > What do you think? It would probably simplify some of the other parts of > this patch if we didn't have to check for possible failure. > > I might go ahead and try to update the spec later today and see if this > approach works. > We decided throwing is OK for now so I've left this as-is. > @@ +83,5 @@ > > +{ > > + if (!mTimeline || mTimeline->GetCurrentTimeDuration().IsNull()) { > > + rv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); > > + return; > > + } > > Here too, I think the spec may be wrong and this should not throw. > Ditto. > Actually, what do you think about splitting off the CSS Animations subclass > now? > Yep, that was my original idea :) > > ::: layout/style/nsAnimationManager.cpp > @@ +313,5 @@ > > + oldPlayer->mPausedByCSS = true; > > + oldPlayer->Pause(rv); > > + } else if (oldPlayer->mPausedByCSS && !newPlayer->IsPaused()) { > > + oldPlayer->mPausedByCSS = false; > > + oldPlayer->Play(rv); > > This seems good except two issues: > > 1. If an animation is paused by calling pause(), then we change > animation-play-state to 'pause' and then back to 'running', do you expect > the original call to pause() to "stick"? > I think it makes sense for the call to pause() to stick because it's more specific. Also, CSS is declarative, so at least in my mind, it comes ahead of time, and then script modifies the state through the API (I know this isn't really accurate, but it sort of makes sense). I think that no matter what, mixing CSS and the web animations API will be a mess. > > 2. When we introduce finishing behavior, play() can have the side-effect of > rewinding the animation to the start. > > Suppose we call play() on an animation player that is paused using > animation-play-state:paused, then *also* set animation-play-state: running > some time later. I don't think you'd expect the animation to rewind in that > case. > > That suggests that we should only call oldPlayer->Play() if *both* > oldPlayer->mPausedByCSS is true *and* oldPlayer->mPaused is true. > True, although we don't have finishing behavior yet. I've changed it. > Please move listen, check_events, new_div, done_div etc. into > animation_utils.js rather than duplicating them here. > I've moved them, but I haven't put them in a closure yet...if I don't get to that before you review the updated patch, I'll do it as a follow-up. > > I think I'd like to make getAnimationPlayers() flush style so that the > cs.marginLeft line is not needed. What do you think? > As discussed on IRC, I agree, but I've left the flush for now. > > I think these tests aren't very comprehensive. I think we should be testing > the various permutations of calling play()/pause() and setting > div.animationPlayState but from what I can tell we only test play()/pause(). > Yeah, I rewrote the test to test more combinations. > Also, we need to test the OMTA case. Particularly, we need to test that > after setting animation-play-state: paused, calling play() triggers a layer > transaction that puts the animation back on the compositor. How do we test OMTA these days? getOMTAStyle?

(In reply to David Zbarsky (:dzbarsky) from comment #6) > (In reply to Brian Birtles (:birtles) from comment #5) > > ::: layout/style/nsAnimationManager.cpp > > @@ +313,5 @@ > > > + oldPlayer->mPausedByCSS = true; > > > + oldPlayer->Pause(rv); > > > + } else if (oldPlayer->mPausedByCSS && !newPlayer->IsPaused()) { > > > + oldPlayer->mPausedByCSS = false; > > > + oldPlayer->Play(rv); > > > > This seems good except two issues: > > > > 1. If an animation is paused by calling pause(), then we change > > animation-play-state to 'pause' and then back to 'running', do you expect > > the original call to pause() to "stick"? > > > > I think it makes sense for the call to pause() to stick because it's more > specific. Also, CSS is declarative, so at least in my mind, it comes ahead > of time, and then script modifies the state through the API (I know this > isn't really accurate, but it sort of makes sense). I think that no matter > what, mixing CSS and the web animations API will be a mess. Yeah, toggling play state using both style and the API is probably going to produce some surprising results sometimes. Still, if you can get the "sticky" behavior to work in a more-or-less consistent and logical manner, that'd be great. > > Also, we need to test the OMTA case. Particularly, we need to test that > > after setting animation-play-state: paused, calling play() triggers a layer > > transaction that puts the animation back on the compositor. > > How do we test OMTA these days? getOMTAStyle? Yep. See test_animations_omta.html basically. (It uses some utility functions to abstract away the asynchronicity but is basically doing getOMTAStyle under the hood.) Ideally, I think I'd like to make even Gecko-specific tests use the testharness format but stick them in a separate folder. James Graham had some ideas about how to set this up at one point but I don't think it's been implemented yet. Basically though, it would be nice if all the tests for Web Animations followed a similar format.

Removed some stray debug lines.

Comment on attachment 8495671 [details] [diff] [review] Patch Review of attachment 8495671 [details] [diff] [review]: ----------------------------------------------------------------- Looks good although I think I want to take one last look to see what you find out about PostRestyleEvent. ::: dom/animation/AnimationPlayer.cpp @@ +77,5 @@ > + // GetTarget returns the element if this animation targets the pseudo, > + // but restyle the element itself is fine here. > + nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(), > + eRestyle_Self, > + nsChangeHint_AllReflowHints); Add a check for !mSource || !mSource->GetTarget() Perhaps reference bug 1073336 where we hope to find a better approach to this (more lightweight and works when mSource or mSource->GetTarget() are null). Also, I think we shouldn't do this when we're called due to a change to animation-play-state. Once we have bug 1073336 we'll probably be able to batch changes better, but for now could we split off a protected PlayInternal / PlaySilent / or something similar (and something similar for Pause) and call it from Play and PlayFromCSS? Alternatively, if you can verify that the extra work this is causing is trivial then I'm ok with it as-is provided there's a suitable comment about it. ::: dom/animation/AnimationPlayer.h @@ +82,5 @@ > + Nullable<TimeDuration> mHoldTime; > + bool mPaused; > +}; > + > +class CSSAnimationPlayer MOZ_FINAL : public AnimationPlayer I think we should just put this in the mozilla namespace since we don't expose it via the DOM? Also, should it go in layout/style instead? @@ +96,5 @@ > + } > + > + void PauseFromCSS(); > + void PlayFromCSS(); > + bool PausedByCSS() const { This should be IsPausedByCSS() to match IsPause() on the parent. Also, perhaps we should replace By with From here and in the member variable, i.e. IsPausedFromCSS and mPausedFromCSS. ::: layout/style/nsAnimationManager.cpp @@ +287,5 @@ > size_t oldIdx = collection->mPlayers.Length(); > while (oldIdx-- != 0) { > AnimationPlayer* a = collection->mPlayers[oldIdx]; > if (a->Name() == newPlayer->Name()) { > + oldPlayer = static_cast<CSSAnimationPlayer*>(a); I'd rather we added a virtual AsCSSAnimationPlayer() to AnimationPlayer. It's safer and if we ultimately succeed in storing CSS Animations and script-generated animations in the safe array, we'll need it then anyway. It adds a vtable to AnimationPlayer but I think that's going to happen anyway when we start pushing event dispatch etc. into CSSAnimationPlayer. Perhaps we could even change the first line to: AnimationPlayer* a = collection->mPlayers[oldIdx]->AsCSSAnimationPlayer(); then test for a && a->Name() == ... ? ::: layout/style/test/animation_utils.js @@ +3,5 @@ > +} > + > +var div = null; > +var cs = null; > +var events_received = []; Can you wrap all this stuff up in a closure and perhaps return [div, cs] from new_div. (We can wrap these methods inside test_animations.html to call the version in animation_utils.js and copy the return values to the global 'div', 'cs' variables so that we don't need to touch the rest of the file.) Also, we should make test_animations_omta.html use this common definition of the functions. ::: layout/style/test/test_play-state.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=1070745 > +--> This should be put in the public domain: Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/ https://www.mozilla.org/MPL/license-policy.html @@ +4,5 @@ > +https://bugzilla.mozilla.org/show_bug.cgi?id=1070745 > +--> > + > +<head> > + <title>Test for css3-animations (Bug 1070745)</title> This should say something about play-state @@ +15,5 @@ > + @keyframes anim1 { > + 0% { margin-left: 0px } > + 100% { margin-left: 100px } > + } > + Extra line not needed @@ +62,5 @@ > +player.pause(); > +is(cs.marginLeft, "100px", "animation-play-state paused at 1500ms"); > +advance_clock(100); > +div.style.animationPlayState = "paused"; > +is(cs.marginLeft, "100px", "animation-play-state paused at 1600ms"); This is much better but I think I'd be more confident if we broke it down a bit based on what we're testing. e.g. pause() a running animation (e.g. just calling pause() on an animation-play-state: running animation) pause() overrides animation-play-state (e.g. call pause() on an animation-play-state: paused animation, then change it to animation-play-state: running and check it's still paused) play() a paused animation (e.g. just calling play() on an animation-play-state: running animation) play() does not stick (e.g. call play() on an animation-play-state: paused animation, then change it to animation-play-state: running and back and check it pauses) ... or whatever permutations there may be When we convert this to something suitable for web-platform-tests we'll probably make it into a number of short methods (that's generally the style there) so splitting it up now will help. @@ +82,5 @@ > +addAsyncAnimTest(function *() { > + > + new_div(""); > + console.log(div); > + console.log(display); Remove these @@ +86,5 @@ > + console.log(display); > + div.style.animation = "anim2 1s 2 linear alternate"; > + div.style.width = "200px"; > + div.style.height="200px"; > + div.style.backgroundColor = "green"; Just define this in the stylesheet at the top (If you refactor new_div appropriately using the version in test_animations_omta.html, it adds the "target" class which you can use to define these.) @@ +93,5 @@ > + > + div.style.animationPlayState = "paused"; > + is(cs.opacity, "0", "initial opacity"); > + advance_clock(250); > + is(cs.opacity, "0", "initial opacity when paused"); Can you verify that the compositor is also not playing, e.g. omta_is(div, "opacity", 0, RunningOn.Either, "Not running on compositor either"); @@ +96,5 @@ > + advance_clock(250); > + is(cs.opacity, "0", "initial opacity when paused"); > + player.play(); > + advance_clock(500); > + yield waitForPaintsFlushed(); Do we need the flushed here? Just yield waitForPaints() should be enough? @@ +99,5 @@ > + advance_clock(500); > + yield waitForPaintsFlushed(); > + > + omta_is(div, "opacity", 0.5, RunningOn.Compositor, > + "OMTA animation is animating as expected"); This comment needs to say something more descriptive. @@ +115,5 @@ > +player.play(); > +advance_clock(500); > +is(player.isRunningOnCompositor, true, 'Animation playing on compositor'); > +SpecialPowers.DOMWindowUtils.restoreNormalRefresh(); > +*/ Remove this part.

(In reply to Brian Birtles (:birtles) from comment #10) > Comment on attachment 8495671 [details] [diff] [review] > Patch > > Review of attachment 8495671 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good although I think I want to take one last look to see what you > find out about PostRestyleEvent. > > ::: dom/animation/AnimationPlayer.cpp > @@ +77,5 @@ > > + // GetTarget returns the element if this animation targets the pseudo, > > + // but restyle the element itself is fine here. > > + nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(), > > + eRestyle_Self, > > + nsChangeHint_AllReflowHints); > > Add a check for !mSource || !mSource->GetTarget() > > Perhaps reference bug 1073336 where we hope to find a better approach to > this (more lightweight and works when mSource or mSource->GetTarget() are > null). > Done. > Also, I think we shouldn't do this when we're called due to a change to > animation-play-state. Once we have bug 1073336 we'll probably be able to > batch changes better, but for now could we split off a protected > PlayInternal / PlaySilent / or something similar (and something similar for > Pause) and call it from Play and PlayFromCSS? > I added PlayInternal. We don't need PauseInternal because pausing doesn't cause a restyle. > > + > > +class CSSAnimationPlayer MOZ_FINAL : public AnimationPlayer > > I think we should just put this in the mozilla namespace since we don't > expose it via the DOM? > Sure. > Also, should it go in layout/style instead? > I think it's fine where it is for now, because it's nice to be able to see the full definitions of Play/PauseFromCSS in one file. We could split it out later if AnimationPlayer grows too big. > > This should be IsPausedByCSS() to match IsPause() on the parent. > > Also, perhaps we should replace By with From here and in the member > variable, i.e. IsPausedFromCSS and mPausedFromCSS. > Yeah, that was weirdly inconsistent. > ::: layout/style/nsAnimationManager.cpp > @@ +287,5 @@ > > size_t oldIdx = collection->mPlayers.Length(); > > while (oldIdx-- != 0) { > > AnimationPlayer* a = collection->mPlayers[oldIdx]; > > if (a->Name() == newPlayer->Name()) { > > + oldPlayer = static_cast<CSSAnimationPlayer*>(a); > > I'd rather we added a virtual AsCSSAnimationPlayer() to AnimationPlayer. > It's safer and if we ultimately succeed in storing CSS Animations and > script-generated animations in the safe array, we'll need it then anyway. > Good idea. > It adds a vtable to AnimationPlayer but I think that's going to happen > anyway when we start pushing event dispatch etc. into CSSAnimationPlayer. > It already had a vtable from WrapObject. > > ::: layout/style/test/animation_utils.js > @@ +3,5 @@ > > +} > > + > > +var div = null; > > +var cs = null; > > +var events_received = []; > > Can you wrap all this stuff up in a closure and perhaps return [div, cs] > from new_div. > I tried doing that, but I couldn't get it working. I'll attach a patch in case you want to take a look. > > @@ +62,5 @@ > > +player.pause(); > > +is(cs.marginLeft, "100px", "animation-play-state paused at 1500ms"); > > +advance_clock(100); > > +div.style.animationPlayState = "paused"; > > +is(cs.marginLeft, "100px", "animation-play-state paused at 1600ms"); > > This is much better but I think I'd be more confident if we broke it down a > bit based on what we're testing. > > e.g. > pause() a running animation (e.g. just calling pause() on an > animation-play-state: running animation) > pause() overrides animation-play-state (e.g. call pause() on an > animation-play-state: paused animation, then change it to > animation-play-state: running and check it's still paused) > play() a paused animation (e.g. just calling play() on an > animation-play-state: running animation) > play() does not stick (e.g. call play() on an animation-play-state: paused > animation, then change it to animation-play-state: running and back and > check it pauses) > ... or whatever permutations there may be > > When we convert this to something suitable for web-platform-tests we'll > probably make it into a number of short methods (that's generally the style > there) so splitting it up now will help. > Sure, I'll do that. > > @@ +93,5 @@ > > + > > + div.style.animationPlayState = "paused"; > > + is(cs.opacity, "0", "initial opacity"); > > + advance_clock(250); > > + is(cs.opacity, "0", "initial opacity when paused"); > > Can you verify that the compositor is also not playing, e.g. > > omta_is(div, "opacity", 0, RunningOn.Either, > "Not running on compositor either"); > Shouldn't this be RunningOn.MainThread?

(In reply to David Zbarsky (:dzbarsky) from comment #11) > > ::: layout/style/test/animation_utils.js > > @@ +3,5 @@ > > > +} > > > + > > > +var div = null; > > > +var cs = null; > > > +var events_received = []; > > > > Can you wrap all this stuff up in a closure and perhaps return [div, cs] > > from new_div. > > > > I tried doing that, but I couldn't get it working. I'll attach a patch in > case you want to take a look. Yes, please give it a go. If the destructuring assignment isn't working, just return an object { div: div, cs: cs }. But if the scoping is giving you trouble, attach something here or send me a pastebin on IRC. > > @@ +93,5 @@ > > > + > > > + div.style.animationPlayState = "paused"; > > > + is(cs.opacity, "0", "initial opacity"); > > > + advance_clock(250); > > > + is(cs.opacity, "0", "initial opacity when paused"); > > > > Can you verify that the compositor is also not playing, e.g. > > > > omta_is(div, "opacity", 0, RunningOn.Either, > > "Not running on compositor either"); > > > > Shouldn't this be RunningOn.MainThread? Possibly. Opacity is a bit weird. For transforms we can differentiate between transforms set on the compositor due to animation or not. For opacity we don't track that information. So if the element hasn't be de-layerized yet we'll still see opacity set on the compositor. If this test passes with RunningOn.MainThread, that's ideal. The main thing we want to test for, though, is that the compositor isn't playing the animation. If we have a layer on the compositor but it has the correct opacity, then that's still acceptable.

(In reply to David Zbarsky (:dzbarsky) from comment #13) > Created attachment 8496648 [details] [diff] [review] > Closure test patch What's the error you're seeing? Why are you reassigning window.new_element etc. in your script? You don't need to go ahead and wrap up everything else, just the new stuff you're adding so we don't end up exposing 'div' and 'cs' everywhere. (In particular waitForPaints, loadPaintListener etc. don't need to be added to the window object. They're deliberately wrapped up inside runOMTATest.)

I started incorporating my review feedback and rewriting these tests as testharness.js tests. I haven't yet go to convert the OMTA tests over but I can have a look at that tomorrow and see if we still need to move code to animation_utils.js. This work in progress stuff is running through try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f9fb59908912

Oh, I forgot to mention, with that updated patch, two of the testharness.js tests fail. David, can you tell what's going on? Is my understanding on how these properties override wrong?

Sorry Brian, I probably won't have time to look at this for a few days. I've attached a copy of my patch that has all the changes we discussed applied (except for the closure stuff and splitting the test into smaller tests). Hopefully that will help.

I've fixed the failing tests, added some more, fixed them, and done a bunch of other clean up. Still to do: * Write / fix up OMTA tests * Add tests for transitions * Merge changes from attachment 8498206 [details] [diff] [review]

(In reply to Brian Birtles (:birtles) from comment #16) > Oh, I forgot to mention, with that updated patch, two of the testharness.js > tests fail. David, can you tell what's going on? Is my understanding on how > these properties override wrong? I fixed this in attachment 8498670 [details] [diff] [review].

We use new_div, check_events etc. in a number of animation-related mochitests and with this bug we'll want to use them in a few more. This patch factors them out into animation_utils.js and tidies them up a little.

We map play() to PlayFromJS internally so that internal users don't accidentally call Play() which flushes style.

We only need to store if an animation is paused or not, hence a bool is sufficient. Furthermore, the convenience of using the same type as the specified style of animation-play-state will disappear once pausing behavior is wrapped up behind Play() and Pause() methods.

This patch introduces the basic implementation of play() and pause(). There are a lot of gaps still because we don't yet: * Support the pending state (to be covered in bug 927349) * Support finishing behavior (to be covered in bug 1074630) * Have a good way of updating animation state outside of style resolution (bug 1073336) Also, we don't call these methods from CSS yet because the interaction between play()/pause() and animation-play-state requires storing some extra state which we introduce in subsequent patches in this series.

This patch introduces a CSSAnimationPlayer subclass of AnimationPlayer. This is because when we have an animation player running an animation it has some special behavior not needed in the general case such as: * Flushing style before performing certain operations to ensure we are using the latest information. * Dispatching CSS Animation events when we start/end. * Storing extra state with regards to "how" the animation was paused. This is because, for example, if an animation is paused by script this overrides specified style.

When an animation player is paused by script this overrides whatever animation-play-state is set by style. The logic for producing the correct override behavior will be added to nsAnimationManager but first we need to record in the player where it was paused.

For players running CSS animations and CSS transitions we should perform a style flush before running play() so that we are operating on the most up-to-date state of animation-play-state. For transitions, which don't have a play-state property, we will still need to run this style flush before running play() to get the right finishing behavior if transition-duration has changed. We haven't implemented finishing yet (that's bug 1074630) but I've kept the flush for both cases now since I'm afraid we'll forget it later. Also, since we don't have a subclass of AnimationPlayer for transitions yet I've put the style flush in the base class. In future, when we add CSSTransitionPlayer we can move the flush to only those players that need up to date style information.

This patch uses the PlayFromStyle/PauseFromStyle methods on CSSAnimationPlayer to perform play/pause control. (This allows us to encapsulate mHoldTime and mPaused. We will encapsulate mStartTime etc. in subsequent bugs. The override behavior of play()/pause() with regard to animation-play-state is: * pause()/play() override the current animation-play-state * pause() causes the player to remain paused until play() is called regardless of changes to animation-play-state (* Calling play() will override the animation-play-state but won't "stick". i.e. subsequently setting animation-play-state: paused will pause the animation.) These different permutations are tested in the next patch in this series. This interaction will probably become more complicated once we introduce finishing behavior (since we might not want animations to restart when setting animation-play-state: running).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=98b5ba6ebe8d I've split the patch up into pieces but I've left the attribution as David except where I've written most of it and any blame should be directed my way.

Comment on attachment 8499423 [details] [diff] [review] part 2 - Update AnimationPlayer IDL to enable play() and pause() >+++ b/dom/bindings/Bindings.conf I'd prefer you put this annotation directly in the IDL now that we can do that. See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#BinaryName (which I just added, so no fault of yours that you hadn't seen it before!).

Comment on attachment 8499420 [details] [diff] [review] part 1 - Factor out new_div etc. to animation_utils.js Review of attachment 8499420 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following: ::: layout/style/test/animation_utils.js @@ +27,5 @@ > + } > + gElem = document.createElement(tagname); > + gElem.setAttribute("style", style); > + gElem.classList.add("target"); > + document.getElementById("display").appendChild(gElem); Now that this lives in a .js file, isolated from actual HTML, it seems a little magical/fragile for it to assume that the document must an element with ID "display"... If someone changes the ID of that element or removes it, this will start failing in a weird way. I was *going* to suggest adding the parent-node as an arg, but after looking at the callers, it is nice to let them not worry about that. So, I think we should just make this assumption into a more explicit requirement. In particular, could you: (1) Add an explicit ok()-failure if document.getElementById("display") is null here? e.g.: const parentId = "display"; var parent = document.getElementById(parentId); if (!parent) { ok(false, "test file needs to have an element with id='" + parentId + "'"); } parent.appendChild(gElem); (2) Add some documentation above new_element, mentioning that: - the new element will be attached as a child of the node with ID "display" - the new element will be given class "target", which can be used (if desired) for additional styling. @@ +29,5 @@ > + gElem.setAttribute("style", style); > + gElem.classList.add("target"); > + document.getElementById("display").appendChild(gElem); > + gCS = getComputedStyle(gElem, ""); > + return [ gElem, gCS ]; I don't think we need to hold onto gCS -- its only usages are in the caller, via this return value. So, let's just call it "cs" here, and drop all other references to gCS. @@ +59,5 @@ > + var rec = gEventsReceived[i]; > + for (var prop in exp) { > + if (prop == "elapsedTime") { > + // Allow floating point error. > + ok(Math.abs(rec.elapsedTime - exp.elapsedTime) < 0.000002, This should probably use "isfuzzy" -- maybe file a followup on that? could be a good-first-bug, even. isfuzzy is documented here: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#262 @@ +77,5 @@ > + > + function done_element() { > + if (!gElem) { > + ok(false, "test author called done_elem/done_div without matching" > + + " call to new_elem/new_div"); Use the full name of "new_element" in both lines of this ok() message, instead of abbreviating it to new_elem. (You're not close enough to the 80-character-limit for those extra 3 characters to make a difference :) And if & when someone actually makes this check fail, it'll be better if we give them the correct function-name.) @@ +79,5 @@ > + if (!gElem) { > + ok(false, "test author called done_elem/done_div without matching" > + + " call to new_elem/new_div"); > + } > + document.getElementById("display").removeChild(gElem); Replace with: gElem.parentElement.removeChild(gElem); (A little bit simpler, and removes an assumption about exact parentage.)

Comment on attachment 8499425 [details] [diff] [review] part 4 - Add AnimationPlayer::Play and Animation::Pause Review of attachment 8499425 [details] [diff] [review]: ----------------------------------------------------------------- I think I need to look through the spec a bit before reviewing the rest of this. I should be able to get to that & review the rest of this bug on Monday, I think. One (optional/followup) nit included below, though: ::: dom/animation/AnimationPlayer.h @@ +82,5 @@ > // The beginning of the delay period. > Nullable<TimeDuration> mStartTime; > Nullable<TimeDuration> mHoldTime; > + bool mIsRunningOnCompositor : 1; > + bool mPaused : 1; Optional nit (maybe for a followup): this should probably be named "mIsPaused", for consistency in naming with "mIsRunningOnCompositor". (Also, I'm not convinced it's worth having ": 1" here, since there are only two bools, which should pack nicely already. However, it looks like the ": 1" goes away in part 8 anyway, so it doesn't really matter I guess.)

(In reply to Daniel Holbert [:dholbert] from comment #32) > Comment on attachment 8499425 [details] [diff] [review] > part 4 - Add AnimationPlayer::Play and Animation::Pause > > Review of attachment 8499425 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think I need to look through the spec a bit before reviewing the rest of > this. I should be able to get to that & review the rest of this bug on > Monday, I think. That's no problem at all. I couldn't see you on IRC so I hope you won't mind if I update these patches to incorporate your initial feedback, split parts 3 and 4 up properly, and fix some failures that try turned up.

Incorporate review feedback

Incorporate review feedback. Also, try server showed some test failures on B2G emulator where we didn't seem to be removing the animation from the compositor immediately for B2G. Flushing style at the end of Pause() seems to fix this so I've added PauseFromJS as well to do this. We should be able to remove this later when we have the ability to wait for a pause to take effect (a follow up to bug 927349).

Update patch with contents that accidentally ended up in patch 4 (whilst simultaneously addressing feedback from comment 33).

Updated the patch based on underlying changes and to fix bitrot now that bug 1075137 has landed which necessitates quite a different approach to detecting if we a processing restyles or not.

Fix bitrot

Fix order of Play/Pause

Fix bitrot

Fix bitrot

Fix bitrot

I think AnimationPlayer::Pause should probably set mIsRunningOnCompositor to false. This is mostly going to be rewritten in bug 1073336 anyway (which I'm currently working on) and only affects animations paused from the API at the moment but is still probably work fixing.

Comment on attachment 8500243 [details] [diff] [review] part 4 - Add AnimationPlayer::Play and Animation::Pause (Sorry for the lag time here.) >+++ b/dom/animation/AnimationPlayer.cpp >+AnimationPlayer::Play(UpdateFlags aFlags) > { [...] >+ if (aFlags == UpdateStyle && mSource && mSource->GetTarget()) { >+ // GetTarget returns the element if this animation targets a pseudo element, >+ // but restyling the element itself is fine here. >+ // >+ // FIXME: This is a bit heavy-handed but in bug 1073336 we hope to >+ // introduce a better means for players to update style. >+ nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(), >+ eRestyle_Self, >+ nsChangeHint_AllReflowHints); Fix indentation on eRestyle_Self there. Also: is nsChangeHint_AllReflowHints really needed here? It seems like it might be overkill... For all we know, our animation might not be targeting any properties that impact layout. Also: this block of code (if-condition & function-call) is copypasted in ::Pause -- maybe refactor it into a private helper-method? e.g. "MaybePostRestyleEvent(UpdateFlags aFlags)" (Also: it looks like Animation::GetTarget() is currently guaranteed to return non-null, per MOZ_ASSERT in the Animation constructor[1]. But the language of that MOZ_ASSERT seems to imply that's changing, so probably good that we're null-checking.) [1] http://mxr.mozilla.org/mozilla-central/source/dom/animation/Animation.h?rev=2ae2d0f3a546&force=1#145 > void > AnimationPlayer::PlayFromJS() > { > // TODO (flush styles etc.) > > Play(); >+ >+ // If this animation can be run on the compositor we need to flush styles >+ // to trigger the layer transaction. >+ // Hopefully, in bug 1073336 we'll find a way of avoiding this. >+ FlushStyle(); > } Drop the pre-existing TODO comment, I think? (looks like the added lines address it, and this patch removes the corresponding comment from PauseFromJs().) >+++ b/dom/animation/AnimationPlayer.h >+ enum UpdateFlags { >+ NoUpdate, >+ UpdateStyle >+ }; Use "e" prefix -- eNoUpdate, eUpdateStyle. >- void Play(); >- void Pause(); >+ void Play(UpdateFlags aUpdateFlags = UpdateStyle); >+ void Pause(UpdateFlags aUpdateFlags = UpdateStyle); I'd rather not use a default value for the arg here, unless it's e.g. necessary to cleanly keep a bunch of external callers working after this patch. (But I don't think there are external callers; none that I could find from a hacky MXR search, at least.) So -- if the only calls are in PlayFromJS() / PauseFromJS(), I'd rather that we just make those callers explicitly pass "eUpdateStyle" instead of using a default arg. This ensures that callers are cognizant of the behavior that they're going to get.

Comment on attachment 8500242 [details] [diff] [review] part 5 - Add CSSAnimationPlayer subclass >Bug 1070745 part 5 - Add CSSAnimationPlayer subclass > >This patch introduces a CSSAnimationPlayer subclass of AnimationPlayer. This is >because when we have an animation player running an animation it has some >special behavior not needed in the general case such as: Maybe "s/running an animation/running a CSS animation/"? (if that's what the comment here means to say) (Right now, "animation player running an animation" sounds like what I would consider "general" [i.e. not-CSS-specific], so it's confusing when it's juxtaposed against "the general case".) > } // namespace dom >+ >+class CSSAnimationPlayer MOZ_FINAL : public dom::AnimationPlayer >+{ >+public: >+ explicit CSSAnimationPlayer(dom::AnimationTimeline* aTimeline) >+ : dom::AnimationPlayer(aTimeline) Is there a reason CSSAnimationPlayer is being specifically excluded from the dom namespace, when its superclass is inside the namespace? (Is it just because it's CSS-flavored? I don't think that justifies this distinction -- it might if this class lived in a layout/style header perhaps, but that's not where it lives. It lives in /dom/animation, so a "dom" namespace makes sense. It's _driven_ by stuff in /layout/style, but it lives in /dom and is associated with stuff there.) Anyway -- if this namespace exclusion is actually important, it probably merits a brief explanatory code-comment here. If it's not important, then let's just put this in the dom namespace with its parent. (and then we can do away with a bunch of unnecssary dom:: prefixing) r=me with that

Comment on attachment 8500244 [details] [diff] [review] part 6 - Add means of marking a CSSAnimationPlayer as being paused by style I'm unclear on the management of mIsPausedFromStyle in "part 6". In particular: (a) Why does PauseFromJS() need to forcibly clear mIsPausedFromStyle? (b) We seem to be inconsistent about what value mIsPausedFromStyle should end up having, when we're paused from both style & JS. If we get "PauseFromStyle(); PauseFromJS()", we'll end up with the flag being false, but if we get the calls in the other order, we'll end up with the flag being true. Is that bad? (Maybe only one call-ordering is possible? If so, can we add assertions to enforce that?) (c) If we get calls to "PauseFromStyle(); PlayFromJS()", it looks like mIsPausedFromStyle will remain set to *true*, even though we're not paused. (since we don't have a PlayFromJS override, so PlayFromJS doesn't know about this flag at all.) That seems odd. Is that supposed to happen? (Maybe this call-ordering isn't supposed to be possible?) (d) Is there anything useful we can assert about mIsPausedFromStyle and/or mIsPaused in these functions? That would help make it clearer what function-call interleavings we actually need to consider here. Perhaps a documentation comment for IsPausedFromStyle() (in the .h file) would help here, too. (particularly to explain what its return value means in scenarios where we've been paused from style *and* JS, since I'm unclear on how that's supposed to work, per (b) above.)

Comment on attachment 8500246 [details] [diff] [review] part 7 - Add style flush at the beginning of play() > AnimationPlayer::PlayFromJS() > { >- // TODO (flush styles etc.) >+ // FIXME: Once we introduce CSSTransitionPlayer, this should move to an >+ // override of PlayFromJS in CSSAnimationPlayer and CSSTransitionPlayer and >+ // we should skip it in the general case. >+ FlushStyle(); > > Play(); > > // If this animation can be run on the compositor we need to flush styles > // to trigger the layer transaction. > // Hopefully, in bug 1073336 we'll find a way of avoiding this. > FlushStyle(); (Ah, this is that TODO from comment 44. I guess we did still need that comment until this patch, so disregard that chunk of comment 44 -- sorry!) Why do we actually need to flush & have up-to-date style information before *and after* we call Play()? It looks like Play() sets the time & calls PostRestyleEvent(); why can't we rely on the (existing) FlushStyle() call after that to get things up-to-date?

Comment on attachment 8500247 [details] [diff] [review] part 8 - Use play/pause from nsAnimationManager >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp >--- a/layout/style/nsAnimationManager.cpp >+++ b/layout/style/nsAnimationManager.cpp >- AnimationPlayer* a = collection->mPlayers[oldIdx]; >+ CSSAnimationPlayer* a = >+ collection->mPlayers[oldIdx]->AsCSSAnimationPlayer(); > if (a->Name() == newPlayer->Name()) { AsCSSAnimationPlayer (added in part 5) is allowed to return null, and does so, on the base class. So, seems like we should be null-checking it here, or at least asserting that it's non-null with an explanation before we use it it (if we're really sure that this particular array will only contain CSSAnimationPlayer instances). (I need to brush up on nsAnimationManager inner-workings before reviewing the rest of this patch... the oldPlayer/newPlayer logic is a bit confusing, and I don't immediately grok what's going on there. More soon.)

(In reply to Daniel Holbert [:dholbert] from comment #44) > Comment on attachment 8500243 [details] [diff] [review] > part 4 - Add AnimationPlayer::Play and Animation::Pause ... > >+ if (aFlags == UpdateStyle && mSource && mSource->GetTarget()) { > >+ // GetTarget returns the element if this animation targets a pseudo element, > >+ // but restyling the element itself is fine here. > >+ // > >+ // FIXME: This is a bit heavy-handed but in bug 1073336 we hope to > >+ // introduce a better means for players to update style. > >+ nsLayoutUtils::PostRestyleEvent(mSource->GetTarget(), > >+ eRestyle_Self, > >+ nsChangeHint_AllReflowHints); > > Fix indentation on eRestyle_Self there. > > Also: is nsChangeHint_AllReflowHints really needed here? It seems like it > might be overkill... For all we know, our animation might not be targeting > any properties that impact layout. No, it's not needed. That's what bug 1073336 addresses. This is a stop-gap measure until that bug lands (patches up for review in a few hours) and it is only ever performed when calling the method from the JS API (which we never do anywhere). > Also: this block of code (if-condition & function-call) is copypasted in > ::Pause -- maybe refactor it into a private helper-method? e.g. > "MaybePostRestyleEvent(UpdateFlags aFlags)" I'd rather just fix it in bug 1073336 if that's ok? > (Also: it looks like Animation::GetTarget() is currently guaranteed to > return non-null, per MOZ_ASSERT in the Animation constructor[1]. But the > language of that MOZ_ASSERT seems to imply that's changing, so probably good > that we're null-checking.) Yes, the handling of that is pretty inconsistent. Eventually we want to allow mTarget to be null but I now realise that's going to be a fair bit of work so it's a way off. (In reply to Daniel Holbert [:dholbert] from comment #45) > Comment on attachment 8500242 [details] [diff] [review] > part 5 - Add CSSAnimationPlayer subclass ... > > } // namespace dom > >+ > >+class CSSAnimationPlayer MOZ_FINAL : public dom::AnimationPlayer > >+{ > >+public: > >+ explicit CSSAnimationPlayer(dom::AnimationTimeline* aTimeline) > >+ : dom::AnimationPlayer(aTimeline) > > Is there a reason CSSAnimationPlayer is being specifically excluded from the > dom namespace, when its superclass is inside the namespace? (Is it just > because it's CSS-flavored? I don't think that justifies this distinction -- > it might if this class lived in a layout/style header perhaps, but that's > not where it lives. It lives in /dom/animation, so a "dom" namespace makes > sense. It's _driven_ by stuff in /layout/style, but it lives in /dom and is > associated with stuff there.) I'm not really sure. I asked David to move this to layout/style but he seemed to think it lived here. In bug 1078122 I moved it to layout/style anyway. In light of that, is it ok to leave it in the dom namespace? (In reply to Daniel Holbert [:dholbert] from comment #46) > Comment on attachment 8500244 [details] [diff] [review] > part 6 - Add means of marking a CSSAnimationPlayer as being paused by style > > I'm unclear on the management of mIsPausedFromStyle in "part 6". > > In particular: > (a) Why does PauseFromJS() need to forcibly clear mIsPausedFromStyle? Yeah, the way the pause-override logic is split between this class and nsAnimationManager is not great. This is needed because pausing from JS overrides pausing from script. We don't have a member, mIsPausedFromScript, rather that's equivalent to mPaused && !mIsPausedFromStyle. I think there's probably a way to move some of the logic out of nsAnimationManager into CSSAnimationPlayer and hopefully make this a little more clear. > > (b) We seem to be inconsistent about what value mIsPausedFromStyle should > end up having, when we're paused from both style & JS. If we get > "PauseFromStyle(); PauseFromJS()", we'll end up with the flag being false, > but if we get the calls in the other order, we'll end up with the flag being > true. Is that bad? (Maybe only one call-ordering is possible? If so, can we > add assertions to enforce that?) Yes, this relates to the above. Basically, we'll never get the sequence of: PauseFromJS(); PauseFromStyle(); because nsAnimationManager will check than the animation is not already paused before calling PauseFromStyle. > (c) If we get calls to "PauseFromStyle(); PlayFromJS()", it looks like > mIsPausedFromStyle will remain set to *true*, even though we're not paused. > (since we don't have a PlayFromJS override, so PlayFromJS doesn't know about > this flag at all.) That seems odd. Is that supposed to happen? (Maybe this > call-ordering isn't supposed to be possible?) From a style point of view the animation is still paused so that's the thinking behind it. I think it probably won't be correct once we come to implement finishing behavior however so I'll try to rework this when I try to incorporate the logic from nsAnimationManager. > (d) Is there anything useful we can assert about mIsPausedFromStyle and/or > mIsPaused in these functions? That would help make it clearer what > function-call interleavings we actually need to consider here. Probably, I'll look into it. (In reply to Daniel Holbert [:dholbert] from comment #48) > Comment on attachment 8500247 [details] [diff] [review] > part 8 - Use play/pause from nsAnimationManager > > >diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp > >--- a/layout/style/nsAnimationManager.cpp > >+++ b/layout/style/nsAnimationManager.cpp > >- AnimationPlayer* a = collection->mPlayers[oldIdx]; > >+ CSSAnimationPlayer* a = > >+ collection->mPlayers[oldIdx]->AsCSSAnimationPlayer(); > > if (a->Name() == newPlayer->Name()) { > > AsCSSAnimationPlayer (added in part 5) is allowed to return null, and does > so, on the base class. So, seems like we should be null-checking it here, > or at least asserting that it's non-null with an explanation before we use > it it (if we're really sure that this particular array will only contain > CSSAnimationPlayer instances). Yes, the array should only ever contain CSSAnimationPlayer instances. Originally I had an assertion for that but I thought it just made the code harder to read. I'll put it back.

Address review feedback

Address review feedback

I completely rewrote this based on the initial review feedback

Reworked based on changes to part 6

Reworked now that we no longer flush after play()

(In reply to Daniel Holbert [:dholbert] from comment #47) > Comment on attachment 8500246 [details] [diff] [review] > part 7 - Add style flush at the beginning of play() [...] > Why do we actually need to flush & have up-to-date style information before > *and after* we call Play()? It looks like Play() sets the time & calls > PostRestyleEvent(); why can't we rely on the (existing) FlushStyle() call > after that to get things up-to-date? We need the FlushStyle before hand in the case of players that correspond to CSS animations/transitions. This is because the author may have specified various property like animation-play-state, animation-duration etc. and play() should operate on the up-to-date state. If we don't incorporate these changes to style you'll get surprising results. There are some tests in part 8 that cover this. We don't really need the flush *after* play() though. I added that to make testing OMTA easier but I've adjusted the test now so it's not necessary and removed this step from the underlying patch.

(In reply to Brian Birtles (:birtles) from comment #49) > > Also: this block of code (if-condition & function-call) is copypasted in > > ::Pause -- maybe refactor it into a private helper-method? e.g. > > "MaybePostRestyleEvent(UpdateFlags aFlags)" > > I'd rather just fix it in bug 1073336 if that's ok? Sure, that sounds good. (I hadn't realized this chunk of code was changing imminently. :)) > > Is there a reason CSSAnimationPlayer is being specifically excluded from the > > dom namespace > > In bug 1078122 I moved it to layout/style [...] is it ok to leave it in the dom namespace? Ah - in that case, excluding it from the dom namespace (as it is in the patch) seems fine then. > (In reply to Daniel Holbert [:dholbert] from comment #48) > Yes, the array should only ever contain CSSAnimationPlayer instances. > Originally I had an assertion for that but I thought it just made the code > harder to read. I'll put it back. Thanks! (& thanks for the other clarifications / updates here) (In reply to Brian Birtles (:birtles) from comment #55) > We need the FlushStyle before hand in the case of players that correspond to > CSS animations/transitions. This is because the author may have specified > various property like animation-play-state, animation-duration etc. and > play() should operate on the up-to-date state Ah, makes sense. So we need up-to-date style info *for CSS properties that control animations*. A comment to that effect would be helpful here; I'd initially assumed the goal here was to get up-to-date styles for potentially-animated properties, and that seemed unnecessary at this point in the code.

Comment on attachment 8500234 [details] [diff] [review] part 2 - Update AnimationPlayer IDL to enable play() and pause() Drive-by nit on Part 2: >diff --git a/dom/animation/AnimationPlayer.cpp b/dom/animation/AnimationPlayer.cpp >+void >+AnimationPlayer::PauseFromJS() >+{ >+ Play(); ^^^^ This ^ should be Pause() (not Play()), since we're in *Pause*FromJS. (I noticed this when looking at the updated version of part 4, which replaces this code.) It ultimately ends up correct, since Part 4 replaces this code, but it'd be nice to not have this Play/Pause mismatch in the revision history, now that we've noticed it. (You should be able to fix this without causing bitrot by hand-editing your Part 2 & Part 4 patch files, and just adjusting this line directly.)

Comment on attachment 8502308 [details] [diff] [review] part 4 - Add AnimationPlayer::Play and Animation::Pause >Bug 1070745 part 4 - Add AnimationPlayer::Play and Animation::Pause; r=dholbert ^^^^^^^^^ Nit: This means to say AnimationPlayer -----------------^ (not "Animation") >diff --git a/dom/animation/AnimationPlayer.cpp b/dom/animation/AnimationPlayer.cpp >+AnimationPlayer::Play(UpdateFlags aFlags) > { [...] >+ Nullable<TimeDuration> now = mTimeline->GetCurrentTimeDuration(); Per IRC, could you add a code-comment here & in the Pause(), to mention the different timescales for the different GetCurrentTimeDuration() calls? >+ mStartTime.SetValue(now.Value() - mHoldTime.Value()); Two things: (1) Per IRC, could you add a comment here explaining that these values are in different timescales, and we're subtracting to compute the difference? (and perhaps also that we'll have to update this when we introduce playback rate) (2) This line implicitly assumes that mHoldTime is non-Null. (If it were Null, the Value() invocation would be invalid and would fatally assert, in an underlying method in the Maybe<> that backs the Nullable.) Could you add an assertion to make that assumption explicit? (I'm guessing this assumption depends on the fact that this function (Play) only does anything if we're paused, so Pause() must have been called; and that Pause() call will have hopefully set mHoldTime to something non-null.) >+AnimationPlayer::Pause(UpdateFlags aFlags) > { >- // TODO >+ if (mIsPaused) { >+ return; >+ } >+ >+ // Bug 927349 - check for null result here and go to pending state >+ mHoldTime = GetCurrentTimeDuration(); >+ mStartTime.SetNull(); >+ mIsPaused = true; Nit: Here or in a followup, let's bump the "mIsPaused = true;" line up a bit, to be right after the check-and-early-return -- both for consistency with ::Play() (which does it this way), and for more-atomic management of this bool. (The logic flow is saner if we check & set it all in one spot)

Comment on attachment 8502310 [details] [diff] [review] part 6 - Add means of marking a CSSAnimationPlayer as being paused by style >+++ b/dom/animation/AnimationPlayer.cpp >+void >+CSSAnimationPlayer::Play(UpdateFlags aUpdateFlags) >+{ >+ mPauseShouldStick = false; >+ AnimationPlayer::Play(aUpdateFlags); >+} It looks to me like we really want to be overriding PlayFromScript() & PauseFromScript() here, instead of overriding Play()/Pause(). I say that because we're setting "mPauseShouldStick", which is specifically supposed to be controlled by scripted play()/pause() calls. So, it seems like this really wants to be an implementation of the scripted method. (and it should call the superclass's FromScript()-flavored method after setting mPauseShouldStick) >+void >+CSSAnimationPlayer::PlayFromStyle() >+{ >+ mIsStylePaused = false; >+ if (!mPauseShouldStick) { >+ AnimationPlayer::Play(eNoUpdate); If I'm right about the Play() vs. PlayFromScript() overriding above, then we can drop the "AnimationPlayer::" prefix here, since we won't have our own ::Play() override anymore. (The same goes for PauseFromStyle's call to AnimationPlayer::Pause.) >+CSSAnimationPlayer::PauseFromStyle() >+{ >+ // Check if we are overriding the pause state >+ if (!mIsPaused && mIsStylePaused) { >+ return; >+ } >+ >+ mIsStylePaused = true; >+ AnimationPlayer::Pause(eNoUpdate); >+} s/we are/script is/, to be clearer about what can make the pause state be overridden. (Or some similar language tweak.) HOWEVER: it looks to me like we shouldn't bother checking mIsPaused here -- i.e. I think we can just have "if (mIsStylePaused)" to guard your early-return. In particular -- let's say mIsStylePaused is true *and* mIsPaused is true. Right now, your code will skip the early return and call AnimationPlayer::Pause(), but I'd expect we could return early, because we're already paused, and we don't stand to gain anything from invoking AnimationPlayer::Pause() again. >+++ b/dom/animation/AnimationPlayer.h >+ > } // namespace mozilla > class CSSAnimationPlayer MOZ_FINAL : public dom::AnimationPlayer > { [...] >+ >+ // When combining animation-play-state with play() / pause() the following >+ // behavior applies: Maybe add "...from script" after "with play() / pause()" here? (or, say "...with calls to the JS play() / pause() APIs", or something like that) It's worth clarifying that you're talking about a JS API here, since this class and its superclass do have methods named "Play()" & "Pause()", and this comment could be mistaken as referring directly to *those* methods. >+ // The base class, AnimationPlayer already provides a boolean value, >+ // mIsPaused which gives us two states. To this we add a further two booleans >+ // to represent the states as follows. >+ // >+ // A. Running >+ // (!mIsPaused; !mIsStylePaused; !mPauseShouldStick) [...] >+ // E. Paused by animation-play-state >+ // (mIsPaused; mIsStylePaused; !mPauseShouldStick) Perhaps worth noting in a parenthetical at the bottom of this list that there are three remaining possible combinations of these booleans (to make a total of 8 = 2^3), and these last 3 don't represent any valid state. (In particular: it looks like the remaining combinations are "!mIsPaused; [anything]; mPauseShouldStick;" (which implies we've been paused by script and it was supposed to stick, but somehow we didn't end up actually-paused), and "mIsPaused; !mIsStylePaused; !mPauseShouldStick" (which implies we're paused, but nobody's claiming credit for it). r=me with the above addressed.

(Thank you for the state diagram & excellent explanatory header-comment in part 6, BTW!! That made this much easier to understand and reason about.)

(In reply to Daniel Holbert [:dholbert] from comment #59) > Comment on attachment 8502310 [details] [diff] [review] > part 6 - Add means of marking a CSSAnimationPlayer as being paused by style > > >+++ b/dom/animation/AnimationPlayer.cpp > >+void > >+CSSAnimationPlayer::Play(UpdateFlags aUpdateFlags) > >+{ > >+ mPauseShouldStick = false; > >+ AnimationPlayer::Play(aUpdateFlags); > >+} > > It looks to me like we really want to be overriding PlayFromScript() & > PauseFromScript() here, instead of overriding Play()/Pause(). > > I say that because we're setting "mPauseShouldStick", which is specifically > supposed to be controlled by scripted play()/pause() calls. So, it seems > like this really wants to be an implementation of the scripted method. (and > it should call the superclass's FromScript()-flavored method after setting > mPauseShouldStick) I actually think this same behavior should apply if a C++ caller were to invoke AnimationPlayer::Play/Pause. The behavior of this methods is specified in the Web Animations spec so I've tried to make them behave in the same way whether they are called from script or C++ with only a few exceptions such as converting times to doubles (rather than the TimeDurations we use internally, see bug 1078119) and flushing style (something I expect a C++ caller to take care of and likely not want). > >+CSSAnimationPlayer::PauseFromStyle() > >+{ > >+ // Check if we are overriding the pause state > >+ if (!mIsPaused && mIsStylePaused) { > >+ return; > >+ } > >+ > >+ mIsStylePaused = true; > >+ AnimationPlayer::Pause(eNoUpdate); > >+} > > s/we are/script is/, to be clearer about what can make the pause state be > overridden. (Or some similar language tweak.) > > HOWEVER: it looks to me like we shouldn't bother checking mIsPaused here -- > i.e. I think we can just have "if (mIsStylePaused)" to guard your > early-return. > > In particular -- let's say mIsStylePaused is true *and* mIsPaused is true. > Right now, your code will skip the early return and call > AnimationPlayer::Pause(), but I'd expect we could return early, because > we're already paused, and we don't stand to gain anything from invoking > AnimationPlayer::Pause() again. Good point. I'll make that change. > >+++ b/dom/animation/AnimationPlayer.h > >+ > > } // namespace mozilla > > class CSSAnimationPlayer MOZ_FINAL : public dom::AnimationPlayer > > { > [...] > >+ > >+ // When combining animation-play-state with play() / pause() the following > >+ // behavior applies: > > Maybe add "...from script" after "with play() / pause()" here? (or, say > "...with calls to the JS play() / pause() APIs", or something like that) > > It's worth clarifying that you're talking about a JS API here, since this > class and its superclass do have methods named "Play()" & "Pause()", and > this comment could be mistaken as referring directly to *those* methods. As before I'm referring to the API whether called by JS or C++. I'm not sure if there's a better term than that? > >+ // The base class, AnimationPlayer already provides a boolean value, > >+ // mIsPaused which gives us two states. To this we add a further two booleans > >+ // to represent the states as follows. > >+ // > >+ // A. Running > >+ // (!mIsPaused; !mIsStylePaused; !mPauseShouldStick) > [...] > >+ // E. Paused by animation-play-state > >+ // (mIsPaused; mIsStylePaused; !mPauseShouldStick) > > Perhaps worth noting in a parenthetical at the bottom of this list that > there are three remaining possible combinations of these booleans (to make a > total of 8 = 2^3), and these last 3 don't represent any valid state. Will do. > (In particular: it looks like the remaining combinations are "!mIsPaused; > [anything]; mPauseShouldStick;" (which implies we've been paused by script > and it was supposed to stick, but somehow we didn't end up actually-paused), > and "mIsPaused; !mIsStylePaused; !mPauseShouldStick" (which implies we're > paused, but nobody's claiming credit for it). :)

Comment on attachment 8500246 [details] [diff] [review] part 7 - Add style flush at the beginning of play() >Bug 1070745 part 7 - Add style flush at the beginning of play() > >For players running CSS animations and CSS transitions we should perform a style >flush before running play() so that we are operating on the most up-to-date >state of animation-play-state. Nit: s/play()/Play()/, in "before running play()" in the extended commit-message here. The flush actually happens **while we're running play()**, but before we call Play(). (I'm interpreting "play()" as referring to the JS-exposed function, and "Play()" as the C++ function that we're actually sticking this style-flush right before.) >+++ b/dom/animation/AnimationPlayer.cpp > void > AnimationPlayer::PlayFromJS() > { >- // TODO (flush styles etc.) >+ // FIXME: Once we introduce CSSTransitionPlayer, this should move to an >+ // override of PlayFromJS in CSSAnimationPlayer and CSSTransitionPlayer and >+ // we should skip it in the general case. >+ FlushStyle(); > > Play(); Please add a comment here explaining why we need this style flush. (to be sure we've got up-to-date information from the properties that control animations) (Might also be worth explicitly noting that the Flush() call here might end up invoking PauseFromStyle() or PlayFromStyle() on us before it returns, if 'this' happens to be a CSSAnimationPlayer instance. That's what we want, of course, to make sure that these play/pause calls interleave in the correct order.) > // If this animation can be run on the compositor we need to flush styles > // to trigger the layer transaction. > // Hopefully, in bug 1073336 we'll find a way of avoiding this. > FlushStyle(); > } (I'm assuming this contextual FlushStyle() (at the end of the function) no longer exists in your local version of this patch, since it went away in the updated part 4 & as noted at end of comment 55.) r=me with the above addressed.

(In reply to Brian Birtles (:birtles) from comment #61) > > It looks to me like we really want to be overriding PlayFromScript() & > > PauseFromScript() here, instead of overriding Play()/Pause(). [...] > I actually think this same behavior should apply if a C++ caller were to > invoke AnimationPlayer::Play/Pause. > The behavior of this methods is specified in the Web Animations spec Ah, I was thinking of Play() as being just the "internal" method that backs PlayFromJS / PlayFromStyle. (It looks like that's the role it serves right now, at least) It still feels broken to have CSSAnimationPlayer add a custom Play()/Pause() overrides, and then its other methods having to specifically ask *not* to call those overrides when invoking Play/Pause... That feels like it could be structured. Per IRC discussion, I think this would probably be cleaner if we shifted the current contents of Play() to a truly internal, protected, "DoPlay()" method, and let Play() just be a wrapper around that which gets called by PlayFromJS() & any future C++ callers. And then CSSAnimationPlayer would override Play() to set the sticky flag. (and PlayFromStyle() would invoke DoPlay() instead of Play(), to avoid setting the sticky flag.) This can happen in a followup, to minimize bitrot pain. > > >+++ b/dom/animation/AnimationPlayer.h > > >+ // When combining animation-play-state with play() / pause() the following > > >+ // behavior applies: > > > > Maybe add "...from script" after "with play() / pause()" here? (or, say > > "...with calls to the JS play() / pause() APIs", or something like that) > > [...] > As before I'm referring to the API whether called by JS or C++. I'm not sure > if there's a better term than that? (This will become less confusing after the refactoring discussed above, since style changes would no longer go through Play() at all -- they'd go through DoPlay() or whatever we call it.)

(In reply to Daniel Holbert [:dholbert] from comment #62) > (I'm assuming this contextual FlushStyle() (at the end of the function) no > longer exists in your local version of this patch, since it went away in the > updated part 4 & as noted at end of comment 55.) Right, I removed that but didn't update all the patches in order to reduce bug spam a little. :)

(In reply to Daniel Holbert [:dholbert] from comment #63) > Per IRC discussion, I think this would probably be cleaner if we shifted the > current contents of Play() to a truly internal, protected, "DoPlay()" > method, and let Play() just be a wrapper around that which gets called by > PlayFromJS() & any future C++ callers. And then CSSAnimationPlayer would > override Play() to set the sticky flag. (and PlayFromStyle() would invoke > DoPlay() instead of Play(), to avoid setting the sticky flag.) This can > happen in a followup, to minimize bitrot pain. Filed bug 1081007 for this.

Comment on attachment 8502311 [details] [diff] [review] part 8 - Use play/pause from nsAnimationManager >+++ b/layout/style/nsAnimationManager.cpp >- AnimationPlayer* a = collection->mPlayers[oldIdx]; >+ CSSAnimationPlayer* a = >+ collection->mPlayers[oldIdx]->AsCSSAnimationPlayer(); > if (a->Name() == newPlayer->Name()) { This still needs an assertion about "a" being non-null [i.e. the array should only containing CSSAnimationPlayer instances], per end of comment 49. r=me with that.

Comment on attachment 8502312 [details] [diff] [review] part 9 - Tests for play/pause behavior >+++ b/dom/animation/test/css-integration/test_animation-pausing.html >+<style> >+@keyframes anim { >+ 0% { margin-left: 0px } >+ 100% { margin-left: 10000px } >+} [...] >+async_test(function(t) { >+ var div = addDiv(); >+ var cs = window.getComputedStyle(div); >+ div.style.animation = 'anim 1000s'; >+ >+ var player = div.getAnimationPlayers()[0]; >+ >+ assert_true(getMarginLeft(cs) >= 0, >+ 'Initial value of margin-left is positive'); Two things: (1) The test message doesn't correspond to what's being tested. ("positive" would be > 0, but we're checking for >= 0). (2) Really, shouldn't the margin value be *exactly* 0 at this point? When this test runs, we've *just* added the animation; shouldn't we be at its "0%" value, which is 0px? The rest of the css-animation testcases in this file all seem assert that the 'Initial value of margin-left is zero', so I think we just want to use that same assertion here as well, unless I'm missing something. >+ player.pause(); >+ div.style.animationPlayState = 'running'; >+ cs.animationPlayState; // Trigger style resolution >+ >+ waitForFrame().then(function() { >+ t.step(function() { >+ assert_equals(getMarginLeft(cs), 0, >+ 'Paused value of margin-left is zero'); >+ }); >+ div.remove(); >+ t.done(); >+ }); >+}, 'pause() overrides animation-play-state'); Optional: I'd think it'd be worth asserting that your animationPlayState tweak was actually effective here, by inspecting cs.animationPlayState in an assert_equals call. (Because, for example, if you made a typo in the "div.style.animationPlayState = 'running'" line, or if we later rename this property or the 'running' keyword for some reason, this test would *spuriously pass*, without any indication that we're not actually testing what we intend to test.) (Same goes for the other tests here that tweak div.style.animationPlayState) >+ waitForFrame().then(function() { >+ t.step(function() { >+ previousAnimVal = getMarginLeft(cs); >+ div.style.animationPlayState = 'paused'; >+ cs.animationPlayState; // Trigger style resolution >+ }); >+ return waitForFrame(); >+ }).then(function() { >+ t.step(function() { >+ assert_equals(getMarginLeft(cs), previousAnimVal, >+ 'Animated value of margin-left does not change when' >+ + ' paused by script'); s/paused by script/paused by style/ in the message here, I think? (We're paused due to the div.style.animationPlayState = 'paused' line, not via a call to pause().) >+ }); >+ div.remove(); >+ t.done(); >+ }); >+}, 'play() does not force play state to remain running'); I think this would be clearer/more understandable as: 'play() is overridden by a later "animation-play-state: paused" tweak' (feel free to pick better language or stick with what you've got if you disagree) >+async_test(function(t) { >+ var previousAnimVal = getMarginLeft(cs); >+ >+ waitForFrame().then(function() { >+ t.step(function() { >+ assert_equals(getMarginLeft(cs), 0, >+ 'Paused value of margin-left does not change'); previousAnimVal is currently unused here -- I think you want to use it in the assert_equals test, instead of 0? (previousAnimVal does happen to be 0, since we're paused from the start here) Alternately, if you want to keep 0 in the assertion, then just drop previousAnimVal, since it's unused. >+ }); >+ div.remove(); >+ t.done(); >+ }); >+}, 'pause() applies pending changes to animation-play-state first'); This description seems like it's claiming more than it really can justify -- this test doesn't *actually* prove that pause() applies pending animation-play-state changes first. (As you explain in the comment inside this test, pause() is sticky, so we'd end up at the right answer here, even if we incorrectly serviced things out of order.) Could you do one of the following: (1) Add a comment adjacent to that message (maybe right after it), saying something like: // (Note that we can't actually test for this; see comment above, in test-body.) ...or: (2) Fix the message to be more accurate about what's really being tested, e.g.: 'pause() overrides an immediately-prior tweak to animation-play-state' >+async_test(function(t) { >+ var div = addDiv(); >+ var cs = window.getComputedStyle(div); >+ >+ div.style.marginLeft = '0px'; >+ cs.transitionProperty; >+ div.style.transition = 'margin-left 100s'; >+ div.style.marginLeft = '10000px'; >+ cs.marginLeft; Do we really need the "cs.transitionProperty;" line there, before we've even touched the 'transition' property? I suspect that might really want to be removed, or replaced with "cs.marginLeft" to ensure that the initial marginLeft="0px" setting gets flushed...? (so that we treat the change to 10000px as being a change & start a transition) >+ var player = div.getAnimationPlayers()[0]; >+ assert_true(getMarginLeft(cs) >= 0, >+ 'Initial value of margin-left is positive'); Similar to the first chunk at the top of this bug-comment: - "is positive" doesn't match the actual ">= 0" test here. - Should we really be checking that it's == 0? (Is it allowable for the transition to have started changing the value yet here? I don't think it is, but I'm not sure) >+ }).then(function() { >+ t.step(function() { >+ assert_true(getMarginLeft(cs) > previousAnimVal, >+ 'margin-left increases after calling play()'); Fix indentation: ^^^ >+++ b/layout/style/test/test_animations_pausing.html [...] >+SimpleTest.waitForExplicitFinish(); >+ >+function finish() { >+ SpecialPowers.DOMWindowUtils.restoreNormalRefresh(); >+ SimpleTest.finish(); >+} >+ >+runOMTATest(function() { >+ runAllAsyncAnimTests().then(function() { >+ SimpleTest.finish(); >+ }); >+}, SimpleTest.finish); It looks to me like this "function finish()" is unused -- runOMTATest just directly calls SimpleTest.finish, rather than the local "finish" function. I think we can drop the local finish() function, since we don't use it, right? (Looks like animation_utils.js takes care of invoking restoreNormalRefresh for us, so I don't think we need it.) r=me with the above addressed.

Intermittent test failures on B2G emulator and Mulet builds in test_animation-pausing.html: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c1669d49dfc

(In reply to Brian Birtles (:birtles) from comment #68) > Intermittent test failures on B2G emulator and Mulet builds in > test_animation-pausing.html: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c1669d49dfc The bug is basically that for transitions that are paused and resumed, we appear to resume the player but not update style. The good news is this is fixed by subsequent bugs (either bug 1073336 or bug 1078122): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f6f7c1779a0f The bad news is I can't work out why because I can't reproduce it locally. I've sent a bug of patches to try with debugging statements and worked out it's not a problem with refresh times. My subsequent try patches overran the output buffer before they produced any useful information but it appears to be that we're not flushing transitions properly. There's some sort of race going on because the test fails roughly 50% of the time. One guess is that simply posting restyles in the manner we do in these patches does not guarantee that transitions get revisited in this particular case (although on other platforms we have other pending restyles that do cover this?). However, the approach introduced in bug 1073336 of posting animation restyles does reliably cause us to revisit transitions. An alternative theory is that we're not resetting some state in the transition case (e.g. mNeedsRefreshes) but since bug 1078122 makes us share more code between animations and transitions it makes us update the appropriate state. I'm not sure why it works on other platforms though. Given the limits on buffer size and the 4hr turnaround time on each test I'm just going to wait and land this bug together with bug 1073336 and bug 1078122. If we end up wanting to do a partial backout of either of those bugs, we can just disable this test on Mulet/B2G emulator desktop temporarily.

(In reply to Brian Birtles (:birtles) from comment #69) > One guess is that simply posting restyles in the manner we do in these > patches does not guarantee that transitions get revisited in this particular > case (although on other platforms we have other pending restyles that do > cover this?). However, the approach introduced in bug 1073336 of posting > animation restyles does reliably cause us to revisit transitions. This appears to be the case since these patches still fail with bug 1078122 applied. Ultimately, this bug should probably be made dependent on bug 1073336, not the other way around. Reworking all the patches from both bugs, though, seems like a massive effort. It's probably better to either: * Disable the transitions test in this bug and re-enable when landing bug 1073336, or * Just land them together and if we need to do a partial backout, disable the transitions test then.

https://hg.mozilla.org/integration/mozilla-inbound/rev/496e867cd2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/00ec97d72ebf https://hg.mozilla.org/integration/mozilla-inbound/rev/5a78f9e22770 https://hg.mozilla.org/integration/mozilla-inbound/rev/e279fb6525b8 https://hg.mozilla.org/integration/mozilla-inbound/rev/8a082e5bb4c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/445a34c37d95 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9236ef0f6b https://hg.mozilla.org/integration/mozilla-inbound/rev/33635f1b4005 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e104207af9d

https://hg.mozilla.org/mozilla-central/rev/496e867cd2cd https://hg.mozilla.org/mozilla-central/rev/00ec97d72ebf https://hg.mozilla.org/mozilla-central/rev/5a78f9e22770 https://hg.mozilla.org/mozilla-central/rev/e279fb6525b8 https://hg.mozilla.org/mozilla-central/rev/8a082e5bb4c3 https://hg.mozilla.org/mozilla-central/rev/445a34c37d95 https://hg.mozilla.org/mozilla-central/rev/8d9236ef0f6b https://hg.mozilla.org/mozilla-central/rev/33635f1b4005 https://hg.mozilla.org/mozilla-central/rev/8e104207af9d