As part of working of document time, I take this bug. I'm going to separate patches as follow: * Add the test of null timeline. * Modify Animation.timeline for exposing. * Modify Animation WebIDL in order to make writable. As mentioned on bug 1096776, I think we maybe should modify the relationship of Animation and Timeline when we set the new timeline. (See bug 1096776 comment #4, for more detail) WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efacc9d0385ea65be68229a44457362e78f1fb7d

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1) > As mentioned on bug 1096776, I think we maybe should modify the relationship > of Animation and Timeline when we set the new timeline. (See bug 1096776 > comment #4, for more detail) This problem is occurred when we allowed that setting the timeline except document timeline. So I'll add these change into the bug 1267510.

Hi Brian. I have two questions about this bug. 1) The source code comment[1] say that the parameter of 'update an animation's finished state' should be 'DidSeek' in current implementation. But Specification is 'NoSeek'. Is this comment correct? If so, I think we must modify the specification. 2) Same question as 1). The comment[2] say that we should call the PostUpdate except that caller is style. Now we call this function from style/nsAnimationManager and style/nsTransitionManager. Does this comment mean that we shouldn't call the PostUpdate function from these? [1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/animation/Animation.cpp#155 [1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/dom/animation/Animation.cpp#159

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3) > Hi Brian. > > I have two questions about this bug. > 1) The source code comment[1] say that the parameter of 'update an > animation's finished state' should be 'DidSeek' in current implementation. > But Specification is 'NoSeek'. > Is this comment correct? If so, I think we must modify the specification. I spent quite a while looking into this. I think the spec and code are probably right and the comment is wrong. For my own reference: the reason is that when we set a new timeline, we also preserve the current time when possible by storing it and then doing a seek after updating the timeline. The cases where this flag matters are when we are go from either (a) no active timeline -> active timeline, or (b) the reverse. For (a) I think it should probably behave as if the animation got clamped at the end of its interval -- as opposed to seeking to the equivalent current time. For (b) we need to preserve the invariant that we only set *either* the start time or the hold time when we have no timeline. Currently the call to silently set the current time does that for us by clearing the start time. However, I think we probably want to preserve the start time in this case? i.e. I think we want anim.startTime = 3000; anim.timeline = timeline; // Suppose this triggers finishing behavior // where we resolve the hold time anim.timeline = null; // At this point anim.startTime should still be 3000 as opposed to null On the other hand: anim.currentTime = 3000; anim.timeline = timeline; // Suppose this triggers finishing behavior anim.timeline = null; // At this point anim.startTime should still be null and anim.currentTime // should be 3000 I think we can probably achieve that by simply clearing the hold time if the start time is set (before we call the procedure to silently set the current time) however we might need to check for pending tasks to make sure we handle the pending state correctly. That's probably easier to verify once we have a rough implementation. > 2) Same question as 1). The comment[2] say that we should call the > PostUpdate except that caller is style. Now we call this function from > style/nsAnimationManager and style/nsTransitionManager. Does this comment > mean that we shouldn't call the PostUpdate function from these? Yes. We should probably add a separate SetTimelineNoUpdate() method and call that from SetTimeline() and call that directly from style. (Then we should go back and rename some of the *FromStyle / *FromJS / Do* methods to use the *NoUpdate naming when appropriate.)

Thanks Brian, (In reply to Brian Birtles (:birtles) from comment #6) > (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3) > > Hi Brian. > I think we can probably achieve that by simply clearing the hold time if the > start time is set (before we call the procedure to silently set the current > time) however we might need to check for pending tasks to make sure we > handle the pending state correctly. That's probably easier to verify once we > have a rough implementation. I implemented to SetTimeline function follow the specification. https://hg.mozilla.org/try/rev/b9b59acc1c0bf39fc7b49edc3e81aeee9cb57d15 The test was failed at following tests. > i.e. I think we want > anim.startTime = 3000; > anim.timeline = timeline; // Suppose this triggers finishing behavior > // where we resolve the hold time > anim.timeline = null; > // At this point anim.startTime should still be 3000 as opposed to null Perhaps, Current specification cleared the start time in 'silently set the current time' procedure if new timeline is null[1]. So I think that we need to set hold time to unresolved and set the 'updated hold time flag' to true in step 4 of 'Setting the timeline'[2], and we might need to add the condition 'if updated hold time flag is false' into step 6 of same procedure. [1]https://w3c.github.io/web-animations/#setting-the-current-time-of-an-animation (at step 3) [2]https://w3c.github.io/web-animations/#setting-the-timeline I tried to implement above process, the result was succeed. https://hg.mozilla.org/try/rev/bd0b4db9e5b5744cb13b7227b12c589c9042b91a Brian, I think that we need to add little change into current specification. How do you feel about this change?

Comment on attachment 8756197 [details] [diff] [review] bug1178662.investigation.patch I've gone through how this should behave for all states and updated the spec accordingly with what I think we should do: https://github.com/w3c/web-animations/commit/a4cba458190a84d267a2b905de25a58c99d5eb26

MozReview-Commit-ID: 1xdKJONyFJV

Comment on attachment 8756734 [details] [diff] [review] part 1 - Add tests for setting the timeline of an animation Review of attachment 8756734 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Brian. I'll submit a patch of implementation.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=890df66a675e

I updated local implementation. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42e62230127

Review commit: https://reviewboard.mozilla.org/r/56162/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56162/

Review commit: https://reviewboard.mozilla.org/r/56164/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56164/

This is implementation of 'Setting the timeline of an animation'.[1] [1] https://w3c.github.io/web-animations/#setting-the-timeline Review commit: https://reviewboard.mozilla.org/r/56166/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56166/

The specification of Animation interface is as follow. https://w3c.github.io/web-animations/#the-animation-interface Review commit: https://reviewboard.mozilla.org/r/56168/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56168/

Review commit: https://reviewboard.mozilla.org/r/56170/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56170/

If we run following code, content process will crash. ------ anim1.timeline = timeline; anim2.timeline = document.timeline; anim2.timeline = timeline; ------ The AnimationTimeline has LinkedList variant. (called mAnimationOrder) And Animation is sub-class of LinkedListElement. We will create the relationship of AnimationTimeline and Animation when calling the AnimationTimeline::NotifyAnimationUpdated. However we didn't remove these relation ship when setting new timeline. So we should remove these relationship when setting new timeline object. Review commit: https://reviewboard.mozilla.org/r/56172/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56172/

Comment on attachment 8757746 [details] MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles https://reviewboard.mozilla.org/r/56166/#review52802

Comment on attachment 8757745 [details] MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles https://reviewboard.mozilla.org/r/56164/#review52804 ::: dom/animation/Animation.h:104 (Diff revision 1) > void SetId(const nsAString& aId); > KeyframeEffectReadOnly* GetEffect() const { return mEffect; } > void SetEffect(KeyframeEffectReadOnly* aEffect); > AnimationTimeline* GetTimeline() const { return mTimeline; } > void SetTimeline(AnimationTimeline* aTimeline); > + void SetTimelineNoUpdate(AnimationTimeline* aTimeline); This declaration should probably be moved alongside CancelFromStyle.

Comment on attachment 8757747 [details] MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug https://reviewboard.mozilla.org/r/56168/#review52806 This needs review from a DOM peer, not me.

Comment on attachment 8757748 [details] MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles https://reviewboard.mozilla.org/r/56170/#review52808

Comment on attachment 8757749 [details] MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles https://reviewboard.mozilla.org/r/56172/#review52810 I'd like to know the answer to bug 1096776 comment 11 before reviewing this, i.e. why is AnimationTimeline::NotifyAnimationUpdated not doing this for us? Is it not being called? Or does it not called in time?

Comment on attachment 8757744 [details] MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in Animation. r?birtles https://reviewboard.mozilla.org/r/56162/#review52816 This isn't really what I had in mind, but I realised that what I had in mind might not work and this is still an improvement.

https://reviewboard.mozilla.org/r/56172/#review52810 Thanks Brian, We called to AnimationTimeline::NotifyAnimationUpdated in Animation. But I think that the way of calling this function is wrong. We used following code when calling this function in Animation.[1] mTimeline->NotifyAnimationUpdated(*this) If we called AnimationTimeline::NotifyAnimationUpdated function as above code, 'this instance' will be Animation's timeline object and aAnimation of parameter will be Animation in this function. Then following condition code will ignored.[2] (aAnimtion.GetTimeline() = aAnimation.mTimeline = this) if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) { aAnimation.GetTimeline()->RemoveAnimation(&aAnimation); } So we didn't remove the relationship of Animation and Timeline, even if we called AnimationTimeline::NotifyAnimationUpdated function. [1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/animation/Animation.cpp#1024 [2] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/animation/AnimationTimeline.cpp#44

Comment on attachment 8757745 [details] MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56164/diff/1-2/

Comment on attachment 8757746 [details] MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56166/diff/1-2/

Comment on attachment 8757747 [details] MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56168/diff/1-2/

Comment on attachment 8757748 [details] MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56170/diff/1-2/

Comment on attachment 8757749 [details] MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56172/diff/1-2/

(In reply to Brian Birtles (:birtles) from comment #38) > Comment on attachment 8757744 [details] > MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in > Animation. r?birtles > > https://reviewboard.mozilla.org/r/56162/#review52816 > > This isn't really what I had in mind, but I realised that what I had in mind > might not work and this is still an improvement. I filed the bug.(bug 1276581)

Comment on attachment 8757747 [details] MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug https://reviewboard.mozilla.org/r/56168/#review52954 It is somewhat unclear to me why this is a good thing to do in the API, but I guess API designers had good use cases and all.

(In reply to Olli Pettay [:smaug] from comment #46) > It is somewhat unclear to me why this is a good thing to do in the API, but > I guess API designers had good use cases and all. Any specific concern here? This is part of the groundwork for being able to plug in different types of timelines: https://wiki.mozilla.org/Platform/Layout/Extended_Timelines

Is there a use case for moving an animation from one timeline to another (that is what the API change hints about). Why not just create a new Animation in the other timeline? I definitely admit that I'm not familiar with all the use cases for which Animation API is designed for.

(In reply to Olli Pettay [:smaug] from comment #48) > Is there a use case for moving an animation from one timeline to another > (that is what the API change hints about). Why not just create a new > Animation in the other timeline? We haven't really worked out how the interaction will work, but the kind of use case we had in mind was a pull-to-update where the animation initially tracks the scroll/touch position, but after crossing some threshold we substitute in a clock-based timeline. Or at least that's the approach we have in mind to try initially (based on discussion with gfx team last December).

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #39) > https://reviewboard.mozilla.org/r/56172/#review52810 > We called to AnimationTimeline::NotifyAnimationUpdated in Animation. But I > think that the way of calling this function is wrong. > We used following code when calling this function in Animation.[1] > > mTimeline->NotifyAnimationUpdated(*this) > > If we called AnimationTimeline::NotifyAnimationUpdated function as above > code, 'this instance' will be Animation's timeline object and aAnimation of > parameter will be Animation in this function. > Then following condition code will ignored.[2] (aAnimtion.GetTimeline() = > aAnimation.mTimeline = this) > > if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) { > aAnimation.GetTimeline()->RemoveAnimation(&aAnimation); > } > > So we didn't remove the relationship of Animation and Timeline, even if we > called AnimationTimeline::NotifyAnimationUpdated function. Thanks, I totally forgot what NotifyAnimationUpdated was trying to do but I went through the related changeset[1] and I see that it is just ensuring that when we do add animations to a timeline's linked list, we make sure they're not already in another linked list. However, that assumes that the animation's timeline pointer has not yet been updated. I suspect that code never actually gets called given the way we're updating timelines and it could probably be replaced with an assertion. For now, however, this patch seems fine. [1] https://hg.mozilla.org/mozilla-central/rev/61455317cc85

Comment on attachment 8757749 [details] MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles https://reviewboard.mozilla.org/r/56172/#review53002

Comment on attachment 8757744 [details] MozReview Request: Bug 1178662 part 2 - Rename *NoUpdate function in Animation. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56162/diff/1-2/

Comment on attachment 8757745 [details] MozReview Request: Bug 1178662 part 3 - Separate SetTimeline function in order to call from style. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56164/diff/2-3/

Comment on attachment 8757746 [details] MozReview Request: Bug 1178662 part 4 - Implement the setting timeline procedure. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56166/diff/2-3/

Comment on attachment 8757747 [details] MozReview Request: Bug 1178662 part 5 - Modify the attribute of Animation's WebIDL in order to conform web animation spec. r?smaug Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56168/diff/2-3/

Comment on attachment 8757748 [details] MozReview Request: Bug 1178662 part 6 - Remove the Animation's w-p-f meta file associated setting the timeline. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56170/diff/2-3/

Comment on attachment 8757749 [details] MozReview Request: Bug 1178662 part 7 - Remove relationship of timeline and animation when setting new timeline. r?birtles Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56172/diff/2-3/

Thanks Brian, I rebased patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/00287f808a35 https://hg.mozilla.org/integration/mozilla-inbound/rev/2f29a879a99b https://hg.mozilla.org/integration/mozilla-inbound/rev/ca028a4f21e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/127e959fd862 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ba1a910376f https://hg.mozilla.org/integration/mozilla-inbound/rev/f2abcc29e65a

https://hg.mozilla.org/mozilla-central/rev/00287f808a35 https://hg.mozilla.org/mozilla-central/rev/2f29a879a99b https://hg.mozilla.org/mozilla-central/rev/ca028a4f21e9 https://hg.mozilla.org/mozilla-central/rev/127e959fd862 https://hg.mozilla.org/mozilla-central/rev/0ba1a910376f https://hg.mozilla.org/mozilla-central/rev/f2abcc29e65a