Closed
Bug 1194639
Opened 9 years ago
Closed 9 years ago
Notify animation mutation observers when script changes playbackrate, currenttime, starttime, etc.
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: pbro, Assigned: birtles)
References
(Blocks 1 open bug)
Details
Attachments
(9 files, 5 obsolete files)
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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.
Summary: Notify animation mutation observers when script changes rate, currenttime, starttime, etc. → Notify animation mutation observers when script changes playbackrate, currenttime, starttime, etc.
Assignee | ||
Comment 3•9 years ago
|
||
Slightly updated patch
Assignee | ||
Updated•9 years ago
|
Attachment #8662785 -
Attachment is obsolete: true
Reporter | ||
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
(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?
Reporter | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #8668846 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8674693 -
Flags: review?(cam)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8674694 -
Flags: review?(cam)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8674695 -
Flags: review?(cam)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8674696 -
Flags: review?(cam)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8674095 -
Attachment is obsolete: true
Attachment #8674697 -
Flags: review?(cam)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8674698 -
Flags: review?(cam)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8674699 -
Flags: review?(cam)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8674700 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8674095 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
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
Assignee | ||
Comment 17•9 years ago
|
||
(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.
Flags: needinfo?(pbrosset)
Assignee | ||
Comment 18•9 years ago
|
||
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.
Assignee | ||
Comment 19•9 years ago
|
||
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.
Reporter | ||
Comment 20•9 years ago
|
||
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.
Reporter | ||
Comment 21•9 years ago
|
||
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.
Flags: needinfo?(pbrosset)
Reporter | ||
Comment 22•9 years ago
|
||
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 23•9 years ago
|
||
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?
Attachment #8674694 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8674695 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8674696 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8674697 -
Flags: review?(cam) → review+
Comment 24•9 years ago
|
||
Should we have some tests where we set the same value on a property and expect it not to generate a mutation record?
Updated•9 years ago
|
Attachment #8674698 -
Flags: review?(cam) → review+
Comment 25•9 years ago
|
||
(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.
Assignee | ||
Comment 26•9 years ago
|
||
(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.
Flags: needinfo?(pbrosset)
Comment 27•9 years ago
|
||
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?
Comment 28•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8674700 -
Flags: review?(cam) → review+
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Reporter | ||
Comment 30•9 years ago
|
||
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.
Flags: needinfo?(pbrosset)
Attachment #8675684 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 31•9 years ago
|
||
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.
Attachment #8676602 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8674699 -
Attachment is obsolete: true
Attachment #8674699 -
Flags: review?(cam)
Assignee | ||
Comment 32•9 years ago
|
||
Rebase on top of changes to part 6
Attachment #8676603 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8674693 -
Attachment is obsolete: true
Attachment #8674693 -
Flags: review?(cam)
Updated•9 years ago
|
Attachment #8676602 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8676603 -
Flags: review?(cam) → review+
Attachment #8675684 -
Flags: review?(mratcliffe) → review+
Comment 33•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•