From bug 1194278: If the playbackRate changes, we should notify mutation observers. We don't today because we didn't support setting the playbackRate when the mutation observers were implemented. Also, we should notify observers if the currrentTime, startTime are modified by script or the animation is paused/resumed. We intend to expose these APIs to script end of this year in release builds, so adding this to mutation observers is a good idea.

Just a WIP patch since I don't think I'll get back to this for a little while. Still to do: * Add tests and implementation for reverse() and finish() * Write test for redundant change handling in SetStartTime (see XXX in patch) * Fix up comments in nsNodeUtils * Check for test failures in browserchrome/DevTools tests * Split up

I've made a bit more progress on this but I came across one complication. For play() and pause() there are technically two moments when we should dispatch change records: when the request is initiated (since, at very least, the state will change, and in many cases startTime or currentTime will too), and when the operation completes (typically on the next frame; often the startTime or currentTime will update at this point). That's fine but it means I'm going to have to touch all the tests to make them handle the extra change event when animations start. Hopefully that's what we want. Let me know if DevTools doesn't need that, however. I've already had to touch a lot of tests since setting the currentTime now dispatches an event.

Slightly updated patch

(In reply to Brian Birtles (:birtles) from comment #2) > I've made a bit more progress on this but I came across one complication. > For play() and pause() there are technically two moments when we should > dispatch change records: when the request is initiated (since, at very > least, the state will change, and in many cases startTime or currentTime > will too), and when the operation completes (typically on the next frame; > often the startTime or currentTime will update at this point). So when you play an animation that was paused, then the startTime shoud change once, right? I'm not sure I understand why there are 2 different moments we want events for. From a devtools' perspective, it seems to me that having only one event when an animation is played or paused would be enough.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4) > (In reply to Brian Birtles (:birtles) from comment #2) > > I've made a bit more progress on this but I came across one complication. > > For play() and pause() there are technically two moments when we should > > dispatch change records: when the request is initiated (since, at very > > least, the state will change, and in many cases startTime or currentTime > > will too), and when the operation completes (typically on the next frame; > > often the startTime or currentTime will update at this point). > So when you play an animation that was paused, then the startTime shoud > change once, right? I'm not sure I understand why there are 2 different > moments we want events for. From a devtools' perspective, it seems to me > that having only one event when an animation is played or paused would be > enough. Basically, something like this will happen: Frame #1: - call play() - playState goes to 'pending' immediately Frame #2: - startTime is resolved - playState goes to 'running' - ready promise is resolved So in both frames there are observable changes to the animation's state. When play() is called in the 'idle' state there will be an additional state change in the first frame where the current time goes from null to 0 (or, depending on the playbackRate, it could jump to the end of the content). I'm guessing that in the above, however, if you go a notification on the second frame, that would be enough?

(In reply to Brian Birtles (:birtles) from comment #5) > I'm guessing that in the above, however, if you go a notification on the > second frame, that would be enough? Yes.

Getting a bit further. Still to do: * Fix intermittent failures for the currentTime redundant change test (It seems like floating-point since we set the currentTime as a double but then convert it to a TimeDuration and compare.) * Fix up comments in tests and nsNodeUtils * Check for test failures in browserchrome tests * Split up

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fbbfc4f934 Looks like there might be some failures with: devtools/client/animationinspector/test/browser_animation_timeline_rewind_button.js | All animations' currentTimes have been set to 0

(In reply to Brian Birtles (:birtles) from comment #16) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=69fbbfc4f934 > > Looks like there might be some failures with: > > devtools/client/animationinspector/test/ > browser_animation_timeline_rewind_button.js | All animations' currentTimes > have been set to 0 Patrick, I started looking into this and it seems that as soon as we start dispatching change events due to a change to currentTime I get this failure. (i.e. part 4 in this patch series causes this, but specifically the line that adds the mutation record, not any of the other logic changes) I started looking into it but you can probably take a better guess at what is happening. Could it be that the additional change records are causing us to resolve something too soon? We'll get an extra mutation record for every animation we call setCurrentTime on.

Adding some logging here I see that we basically end up doing this: 23 INFO Click on the button to rewind all timeline animations About to click rewind Called SetCurrentTime: 0.000000 x 6 (each call is from the API, and generates an mutation record) Called SetCurrentTime: 30394.262007 x 6 (each call is from the API, and generates an mutation record) Called SetCurrentTime: 36074.543749 (each call is from the API, and generates an mutation record) 24 INFO Check that the scrubber has stopped moving 25 INFO TEST-PASS | devtools/client/animationinspector/test/browser_animation_timeline_rewind_button.js | The scrubber is not moving - 26 INFO TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_timeline_rewind_button.js | All animations' currentTimes have been set to 0 - Stack trace: chrome://mochitests/content/browser/devtools/client/animationinspector/test/browser_animation_timeline_rewind_button.js:null:35 Adding further logging at this point reveals that each animation has a currentTime of 36074.54374887741 and is paused. So I guess those extra mutation records are triggering something that calls setCurrentTime.

I added JS stack tracing from Gecko, but all it told me was that in all cases we're getting called from devtools/server/actors/animation.js:829 (i.e. setCurrentTimes on AnimationsActor). I guess I need to add logging on the client side instead.

That's odd. I'll need to apply your patches locally and investigate. So far I can't find anything that would cause this. The new mutation records you're adding should be filtered out by the actor anyway, so I don't see how they could cause this. Keeping the NI? flag for now until I revisit this.

Ah, I think I know, on top of the AnimationsActor (the main server-side entry point) that listens for mutations (and filters out anything it does not care about), all AnimationPlayerActors (the thing that represents a single Animation object on the devtools server-side) also listen to mutations. And they don't filter out much, which means that, because with this patch we receive more records, these end up being received by these actors and sent to the front-end of devtools, which uses this in various ways. Bottom line is, these end up refreshing the UI somehow, and breaking things. I'll try to upload a devtools patch.

This makes the test pass (at least locally for me). The mutation observer at AnimationPlayerActor level now only sends events to the front-end when the duration, delay or iteration count change. This is what used to happen before these patches, but now, it needs to be explicit because the observer is executed more often than before. In later devtools bugs, we should make sure events are sent also when the rate changes, or startTime, etc... but for now, let's get back to the current behavior so that you can land your patches.

Comment on attachment 8674694 [details] [diff] [review] part 1 - Add AutoMutationBatchForAnimation Review of attachment 8674694 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsNodeUtils.cpp @@ +229,5 @@ > static inline Element* > GetTarget(Animation* aAnimation) > { > + // Any changes to the this method should be synchronized with > + // AutoMutationBatchForAnimation in dom/animation/Animation.cpp. s/the// What about exposing this method and calling it from AutoMutationBatchForAnimation's constructor so we don't have to keep them in sync manually?

Should we have some tests where we set the same value on a property and expect it not to generate a mutation record?

(In reply to Cameron McCormack (:heycam) from comment #24) > Should we have some tests where we set the same value on a property and > expect it not to generate a mutation record? Sorry I see you already do that in part 2.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #22) > Created attachment 8675684 [details] [diff] [review] > Bug_1194639_-_Filter_out_changed_animation_mutatio.diff > > This makes the test pass (at least locally for me). > The mutation observer at AnimationPlayerActor level now only sends events to > the front-end when the duration, delay or iteration count change. This is > what used to happen before these patches, but now, it needs to be explicit > because the observer is executed more often than before. > In later devtools bugs, we should make sure events are sent also when the > rate changes, or startTime, etc... but for now, let's get back to the > current behavior so that you can land your patches. Thanks so much Patrick! How should we proceed here? I'm not sure if I qualify as a reviewer for that patch? If you're happy to let me review it though, I'm happy to do it.

Comment on attachment 8674699 [details] [diff] [review] part 6 - Report changes from calling finish() to animation mutation observers Review of attachment 8674699 [details] [diff] [review]: ----------------------------------------------------------------- Consider also a test for an animation that was already finished not generating any mutation records when finish() is called on it. ::: dom/animation/Animation.cpp @@ +375,5 @@ > mReady->MaybeResolve(this); > } > } > UpdateTiming(SeekFlag::DidSeek, SyncNotifyFlag::Sync); > + if (didSeek && IsRelevant()) { Is it possible for us not to have seeked but to have made other changes e.g. to our play state, and that we'll need to dispatch generate a mutation record for that?

(In reply to Cameron McCormack (:heycam) from comment #27) > Consider also a test for an animation that was already finished not > generating any mutation records when finish() is called on it. Oh, again, I see you've already done that within the test.

(In reply to Cameron McCormack (:heycam) from comment #27) > Comment on attachment 8674699 [details] [diff] [review] > part 6 - Report changes from calling finish() to animation mutation observers > > Review of attachment 8674699 [details] [diff] [review]: > ----------------------------------------------------------------- > > Consider also a test for an animation that was already finished not > generating any mutation records when finish() is called on it. > > ::: dom/animation/Animation.cpp > @@ +375,5 @@ > > mReady->MaybeResolve(this); > > } > > } > > UpdateTiming(SeekFlag::DidSeek, SyncNotifyFlag::Sync); > > + if (didSeek && IsRelevant()) { > > Is it possible for us not to have seeked but to have made other changes e.g. > to our play state, and that we'll need to dispatch generate a mutation > record for that? Yes, I think it probably is. I think if we did finish() followed by pause() followed by finish(), for example, we'd hit this case. I'll write a test to prove it then fix it.

Comment on attachment 8675684 [details] [diff] [review] Bug_1194639_-_Filter_out_changed_animation_mutatio.diff Mike is a good reviewer for this, he's reviewed many of my patches on this code already. Mike: this makes sure the AnimationPlayerActor don't send events to the front every time an animation's currentTime is changed. This also fixes a latent bug where rewinding the timeline would call setCurrentTime twice on all AnimationPlayerActors.

This revises the previous patch to make sure we dispatch events when calling finish() on a paused (or pause-pending) but otherwise finished animation. Unfortunately, even without the code changes, one of added the tests still doesn't fail due to bug 1216846. Bear in mind that this patch comes before the patch to dispatch changes on calls to pause() so the added tests don't expect records from calling pause(). I'll update part 8 to tweak these tests accordingly shortly.

Rebase on top of changes to part 6

https://hg.mozilla.org/integration/mozilla-inbound/rev/0513d651719b https://hg.mozilla.org/integration/mozilla-inbound/rev/0d867dfee957 https://hg.mozilla.org/integration/mozilla-inbound/rev/90370e8e13f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/c00dac3fdf4a https://hg.mozilla.org/integration/mozilla-inbound/rev/96a15f93f1f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/af2e781dc33d https://hg.mozilla.org/integration/mozilla-inbound/rev/3b618ef232d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/ee020ced7da3 https://hg.mozilla.org/integration/mozilla-inbound/rev/3efb51b0d671

https://hg.mozilla.org/mozilla-central/rev/0513d651719b https://hg.mozilla.org/mozilla-central/rev/0d867dfee957 https://hg.mozilla.org/mozilla-central/rev/90370e8e13f2 https://hg.mozilla.org/mozilla-central/rev/c00dac3fdf4a https://hg.mozilla.org/mozilla-central/rev/96a15f93f1f4 https://hg.mozilla.org/mozilla-central/rev/af2e781dc33d https://hg.mozilla.org/mozilla-central/rev/3b618ef232d1 https://hg.mozilla.org/mozilla-central/rev/ee020ced7da3 https://hg.mozilla.org/mozilla-central/rev/3efb51b0d671