Move getAnimations() from AnimationTimeline to Document

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: birtles, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox46 fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

3.72 KB, patch
heycam
: review+
smaug
: review+
Details | Diff | Splinter Review
3.68 KB, patch
birtles
: review+
Details | Diff | Splinter Review
7.58 KB, patch
hiro
: review+
Details | Diff | Splinter Review
19.44 KB, patch
hiro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
As proposed here:

https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html

Doing this properly likely requires refactoring the way animations are sampled. We should probably store a map per-document(?) that maps Animations to AnimationTimelines. Then we could make that map observe the refresh driver (not the AnimationTimeline).

As a later step when we come to support script-defined timelines, we would like sample each timeline first, then pass the result to the corresponding animation. (Animations would need a way of getting the timeline time at other moments, either by storing the sample time on the animation--something that is quite complicated, see bug 1208385 comment 11--or by storing the returned timeline times on the map and having animations query the map each time.)
(Reporter)

Updated

3 years ago
Blocks: 875219
(Reporter)

Comment 1

3 years ago
Actually, I think initially this doesn't even need to be a map. It could just be a hash of the animations exactly like we currently store on AnimationTimeline. We only need the map when we do things like:

* Tick timelines as a separate step and cache their time
* Provide a means of looking up animations by their timeline
* etc.
(Assignee)

Comment 2

3 years ago
Created attachment 8677989 [details] [diff] [review]
Mochitests based on file_timeline-get-animations.html

This is mochitests what I believe to be correct behavior of Document.getAnimations().

Most of the tests are the same as tests in file_timeline-get-animations.html, some of tests are different because of lifetimes of animations in getAnimations().

The biggest difference is that 'Order of CSS Animations - free animations' test has been removed because freed animations are not appeared in getAnimations(). And the case for 'freed aniamtions are not appeared' is covered by a prior test.

I will post the diff file between file_timeline-get-animations.html and file_document-get-animations.html for reference.
Assignee: nobody → hiikezoe
(Assignee)

Comment 3

3 years ago
Created attachment 8677991 [details] [diff] [review]
The diff betwee file_timeline-get-animations.html and file_document-get-animations.html
(Assignee)

Comment 4

3 years ago
Created attachment 8678003 [details] [diff] [review]
WIP patch

This is a WIP patch which makes most of tests in attachment 8677989 [details] [diff] [review] pass except a test case which replaces style animation by new animations like this:

 div.style.animation = 'anim1 10s, anim2 10s';
 div.style.animation = 'newAnim1 10s, newAnim2 10s';

With the current WIP patch document.getAnimations() returns all of 4 animations in this case because the patch does not remove the previous animations at all in that case. I'm now thinking what is the right approach to solve this issue.
(Reporter)

Comment 5

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> The biggest difference is that 'Order of CSS Animations - free animations'
> test has been removed because freed animations are not appeared in
> getAnimations(). And the case for 'freed aniamtions are not appeared' is
> covered by a prior test.

Free animations do appear in getAnimations() if they are subsequently restarted. This test should remain since we need to test the order these restarted animations are returned in.
(Assignee)

Comment 6

3 years ago
(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > The biggest difference is that 'Order of CSS Animations - free animations'
> > test has been removed because freed animations are not appeared in
> > getAnimations(). And the case for 'freed aniamtions are not appeared' is
> > covered by a prior test.
> 
> Free animations do appear in getAnimations() if they are subsequently
> restarted. This test should remain since we need to test the order these
> restarted animations are returned in.

I mean that 'freed animations' are animations which are cancelled by style. i.e. div.style.animation = ''.
In these cases how can we restart these animations? I think 'restart' means that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is there another way to free animations? One possible way I can think of is to remove animation element from the document. I did actually add this case as another patch.
(Reporter)

Comment 7

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > The biggest difference is that 'Order of CSS Animations - free animations'
> > > test has been removed because freed animations are not appeared in
> > > getAnimations(). And the case for 'freed aniamtions are not appeared' is
> > > covered by a prior test.
> > 
> > Free animations do appear in getAnimations() if they are subsequently
> > restarted. This test should remain since we need to test the order these
> > restarted animations are returned in.
> 
> I mean that 'freed animations' are animations which are cancelled by style.
> i.e. div.style.animation = ''.
> In these cases how can we restart these animations? I think 'restart' means
> that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is
> there another way to free animations? One possible way I can think of is to
> remove animation element from the document. I did actually add this case as
> another patch.

In this context, a free animation is an animation that is not bound to markup. You can restart it like this:

  elem.style.animation = 'anim 10s';
  var anim = elem.getAnimations()[0];
  elem.style.animation = '';
  // anim is now free (i.e. not bound to markup)
  anim.play();
  // anim is free but restarted
(Assignee)

Comment 8

3 years ago
Created attachment 8678722 [details]
An example for restarting an animation after freeing.

(In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Brian Birtles (:birtles, busy 24~29 Oct) from comment #5)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > > The biggest difference is that 'Order of CSS Animations - free animations'
> > > > test has been removed because freed animations are not appeared in
> > > > getAnimations(). And the case for 'freed aniamtions are not appeared' is
> > > > covered by a prior test.
> > > 
> > > Free animations do appear in getAnimations() if they are subsequently
> > > restarted. This test should remain since we need to test the order these
> > > restarted animations are returned in.
> > 
> > I mean that 'freed animations' are animations which are cancelled by style.
> > i.e. div.style.animation = ''.
> > In these cases how can we restart these animations? I think 'restart' means
> > that animations are newly created. i.e. div.style.animation = 'anim 10s'. Is
> > there another way to free animations? One possible way I can think of is to
> > remove animation element from the document. I did actually add this case as
> > another patch.
> 
> In this context, a free animation is an animation that is not bound to
> markup. You can restart it like this:
> 
>   elem.style.animation = 'anim 10s';
>   var anim = elem.getAnimations()[0];
>   elem.style.animation = '';
>   // anim is now free (i.e. not bound to markup)
>   anim.play();
>   // anim is free but restarted

This example is very impressive to me. The animation play state after calling anim.play() is actually 'running' but there is no visual changes on the element.
I agree it's worth testing this case but the restarting animation should be really returned in document.getAnimations() list? elem.getAnimations() returns also empty list after restarting. Is this bug?

I am attaching an example for this case.
(Reporter)

Comment 9

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> This example is very impressive to me. The animation play state after
> calling anim.play() is actually 'running' but there is no visual changes on
> the element.

That's because we don't support free animations yet. That's part of the refactoring we need to do to support script-generation animations (bug 1151731).

> I agree it's worth testing this case but the restarting animation should be
> really returned in document.getAnimations() list?

Yes.

> elem.getAnimations()
> returns also empty list after restarting. Is this bug?

Yes.
(Assignee)

Comment 10

3 years ago
Note about current hash table owned by AnimationTimeline.
The hash table contains temporary animations created in nsAnimationManager::BuildAnimations[1], even if the temporary animations are not necessary. e.g. changing animation duration property.

[1] http://hg.mozilla.org/mozilla-central/file/1e700005a0dd/layout/style/nsAnimationManager.cpp#l670

The temporary animations in the hash will be surely removed in next tick, but we should avoid it.
I am not sure we can fix this issue in case of document.getAnimations().
(Assignee)

Comment 11

3 years ago
There are three cases what I am now concerned:

a)
 div.style.animation = 'anim 1s';
 var animations = div.getAnimations();
 div.style.animation = '';
 document.getAnimations(); // Should be an array consisting of one animation because |animations| holds the reference.

b)
 div.style.animation = 'anim 1s';
 div.style.animation = '';
 document.getAnimations(); // Should be an empty array.

c)
 div.style.animation = 'anim 1s';
 var animations = div.getAnimations();
 div.style.animation = '';
 animations = null;
 document.getAnimations(); // Should be an empty array.

To distinguish there cases I am now checking reference count of dom::Animation instance in Document::GetAnimations(). I think if the reference count equals to 1 we can ignore the animation. (KeyframeEffectReadOnly holds a reference.). But in case of b) and c), the reference count is 2. Even if the reference count is 2, the animation instance is disposed after a while. I guess it's done by cycle collector.

I believe KeyframeEffectReadOnly holds one reference but I have yet no idea who the other one is held by.
(Reporter)

Comment 12

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> There are three cases what I am now concerned:
> 
> a)
>  div.style.animation = 'anim 1s';
>  var animations = div.getAnimations();
>  div.style.animation = '';
>  document.getAnimations(); // Should be an array consisting of one animation
> because |animations| holds the reference.

In this case document.getAnimations() should *not* return any animations since they have all been cancelled.

i.e.
div.style.animation = '' will call cancel() on all CSS animations associated with |div| which puts them in the idle state. Since they are idle, they are not current or in-effect so they won't be returned from getAnimations.

> To distinguish there cases I am now checking reference count of
> dom::Animation instance in Document::GetAnimations(). I think if the
> reference count equals to 1 we can ignore the animation.

We shouldn't need to do that. In all cases, we should not return the cancelled animations.
(Assignee)

Updated

3 years ago
Blocks: 1234403
(Assignee)

Comment 13

3 years ago
Created attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

Now we have Element::GetAnimations using EffectSet, so we can use it from document.

This patch calls Element::GetAnimations which involves sorting each time when it's called. I wondered it's slower than calling EffectSet::GetEffectSet directly to avoid the sorting.  So I tried to measure average time of calling Document.getAnimations() 100 times.

Measured on http://codepen.io/juliangarnier/pen/idhuG
There are 26 animations in the document.

average time (ms)
Use Element::GetAnimations (this patch)
average = 0.11280000000006112
average = 0.11474999999980355
average = 0.14084999999933642
average = 0.13170000000114668
average = 0.13854999999959547

Use EffectSet::GetEffectSet directly
average = 0.14510000000023865
average = 0.1582500000006985
average = 0.12480000000083237
average = 0.12075000000011642
average = 0.15894999999960419

There is no signifficant differences. That's because, I guess, all of animating elements have only one animation.  I think it's a common use case in the real world.  Even if some elements have a couple of animations, I think the result will not be quite different.
Attachment #8677989 - Attachment is obsolete: true
Attachment #8677991 - Attachment is obsolete: true
Attachment #8678003 - Attachment is obsolete: true
Attachment #8678722 - Attachment is obsolete: true
Attachment #8700901 - Flags: review?(cam)
(Assignee)

Comment 14

3 years ago
Created attachment 8700903 [details] [diff] [review]
Part 2: Tests for document.getAnimations.

Just replaced timeline.getAnimations.
Attachment #8700903 - Flags: review?(cam)
(Assignee)

Comment 15

3 years ago
Created attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.
Attachment #8700904 - Flags: review?(cam)
(Assignee)

Comment 16

3 years ago
I forgot to mention about animations on pseudo elements. Currently Document.getAnimations() does not return any animations on pseudo elements. Once we fix bug 1174575, we should return animations on pseudo elements (bug 1234403).
(Reporter)

Comment 17

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Created attachment 8700901 [details] [diff] [review]
> Part 1: Implement Document.getAnimations()
> 
> Now we have Element::GetAnimations using EffectSet, so we can use it from
> document.
> 
> This patch calls Element::GetAnimations which involves sorting each time
> when it's called. I wondered it's slower than calling
> EffectSet::GetEffectSet directly to avoid the sorting.

I think we should only sort once at the end rather than re-sorting.

If you want to re-use code from Element::GetAnimations you could factor out Element::GetAnimationsUnsorted and call that from both Element::GetAnimations and Document::GetAnimations.

I think we should also avoid calling FlushPendingNotifications on every element.
(Assignee)

Comment 18

3 years ago
Created attachment 8700922 [details] [diff] [review]
Part 1.5: Add Element::GetAnimationsUnsorted and use it.

Thanks Brian!

average = 0.11205000000005384
average = 0.11970000000004802
average = 0.11644999999996798
average = 0.11399999999975989
average = 0.11569999999996071

Little bit faster, and stable?
Attachment #8700922 - Flags: review?(cam)
(Reporter)

Comment 19

3 years ago
Comment on attachment 8700922 [details] [diff] [review]
Part 1.5: Add Element::GetAnimationsUnsorted and use it.

>Bug 1212720 - Part 1.5: Add Element::GetAnimationsUnsorted and use it. r?heycam
>@@ -3138,17 +3138,17 @@ nsDocument::GetAnimations(nsTArray<RefPt
>   for (nsIContent* node = nsINode::GetFirstChild();
>        node;
>        node = node->GetNextNode(this)) {
>     if (!node->IsElement()) {
>       continue;
>     }
> 
>     nsTArray<RefPtr<Animation>> array;
>-    node->AsElement()->GetAnimations(array);
>+    node->AsElement()->GetAnimationsUnsorted(array);
>     if (array.IsEmpty()) {
>       continue;
>     }
> 
>     aAnimations.AppendElements(array);

Why don't you just pass aArray into GetAnimationsUnsorted? (Rather than allocating a new array, only to copy and discard it.)
(Assignee)

Comment 20

3 years ago
Created attachment 8700924 [details] [diff] [review]
Part 1.5: Add Element::GetAnimationsUnsorted and use it. v2

(In reply to Brian Birtles (:birtles) from comment #19)
> >     nsTArray<RefPtr<Animation>> array;
> >-    node->AsElement()->GetAnimations(array);
> >+    node->AsElement()->GetAnimationsUnsorted(array);
> >     if (array.IsEmpty()) {
> >       continue;
> >     }
> > 
> >     aAnimations.AppendElements(array);
> 
> Why don't you just pass aArray into GetAnimationsUnsorted? (Rather than
> allocating a new array, only to copy and discard it.)

Thanks again!
Attachment #8700922 - Attachment is obsolete: true
Attachment #8700922 - Flags: review?(cam)
Attachment #8700924 - Flags: review?(cam)
Hiro, did you upload the right patch?  Part 1.5 looks identical to Part 1.
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 22

3 years ago
Created attachment 8700933 [details] [diff] [review]
Part 1.5: Add Element::GetAnimationsUnsorted and use it. v2

I am sorry for the wrong patch.
Attachment #8700924 - Attachment is obsolete: true
Attachment #8700924 - Flags: review?(cam)
Flags: needinfo?(hiikezoe)
Attachment #8700933 - Flags: review?(cam)
Comment on attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

Review of attachment 8700901 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but I would fold in Part 1.5 into this patch when landing.
Attachment #8700901 - Flags: review?(cam) → review+
Comment on attachment 8700933 [details] [diff] [review]
Part 1.5: Add Element::GetAnimationsUnsorted and use it. v2

Review of attachment 8700933 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.h
@@ +821,5 @@
>    {
>    }
>  
>    void GetAnimations(nsTArray<RefPtr<Animation>>& aAnimations);
> +  void GetAnimationsUnsorted(nsTArray<RefPtr<Animation>>& aAnimations);

Please add a comment in here pointing out that GetAnimations will flush style while GetAnimationsUnsorted won't.
Attachment #8700933 - Flags: review?(cam) → review+
Comment on attachment 8700903 [details] [diff] [review]
Part 2: Tests for document.getAnimations.

Review of attachment 8700903 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/mochitest.ini
@@ +66,5 @@
>  [css-transitions/test_element-get-animations.html]
>  skip-if = buildapp == 'mulet'
>  support-files = css-transitions/file_element-get-animations.html
> +[css-transitions/test_document-get-animations.html]
> +support-files = css-transitions/file_document-get-animations.html

It looks like the tests in this file are listed in sorted order, so please move this renamed test file into its correct sorted position.
Attachment #8700903 - Flags: review?(cam) → review+
Comment on attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.

Review of attachment 8700904 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if it's worth preserving the comment about not supporting pseudo-elements, somewhere in nsDocument::GetAnimations?
Attachment #8700904 - Flags: review?(cam) → review+
BTW you'll need to get review from a DOM peer on the patches that touch .webidl files.
(Reporter)

Comment 28

3 years ago
Also, I wonder what we should do about this:

  https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32

This function specifically handles animations that run on elements that aren't part of the document tree.
(Assignee)

Comment 29

3 years ago
Comment on attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

Olli, could you please look the part of webidl change?
Attachment #8700901 - Flags: review?(bugs)
(Assignee)

Comment 30

3 years ago
Comment on attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.

And this?
Attachment #8700904 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Blocks: 1234481
(Assignee)

Comment 31

3 years ago
(In reply to Brian Birtles (:birtles) from comment #28)
> Also, I wonder what we should do about this:
> 
>  
> https://dxr.mozilla.org/mozilla-central/rev/
> 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32
> 
> This function specifically handles animations that run on elements that
> aren't part of the document tree.

Yeah, I left this part as is because I am not confident that the case (animations still exist in the tree) is still valid or not when we use EffectSet, and I have no idea when the case happens/happened. test_timeline_get_animations covers the case?, or bug 1232829 was related to?

I just filed bug 1234481 for that.
No longer blocks: 1234481
(Assignee)

Updated

3 years ago
Blocks: 1234481
(Reporter)

Comment 32

3 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> (In reply to Brian Birtles (:birtles) from comment #28)
> > Also, I wonder what we should do about this:
> > 
> >  
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32
> > 
> > This function specifically handles animations that run on elements that
> > aren't part of the document tree.
> 
> Yeah, I left this part as is because I am not confident that the case
> (animations still exist in the tree) is still valid or not when we use
> EffectSet, and I have no idea when the case happens/happened.
> test_timeline_get_animations covers the case?, or bug 1232829 was related to?

I don't think we need that any more. It was most likely needed because the number of animations returned by document.timeline.getAnimations() was sometimes wrong due to animations that were running on elements no longer bound to the tree. That's no longer going to happen with this new implementation of getAnimations().

> I just filed bug 1234481 for that.

I think you should fix it here rather than check in code that calls functions that no longer exist.
(Assignee)

Comment 33

3 years ago
Created attachment 8700959 [details] [diff] [review]
Part 4: Remove all cancelAllAnimationsOnEnd. It is not needed any more.

(In reply to Brian Birtles (:birtles) from comment #32)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> > (In reply to Brian Birtles (:birtles) from comment #28)
> > > Also, I wonder what we should do about this:
> > > 
> > >  
> > > https://dxr.mozilla.org/mozilla-central/rev/
> > > 388bdc46ba51ee31da8b8abe977e0ca38d117434/dom/animation/test/testcommon.js#32
> > > 
> > > This function specifically handles animations that run on elements that
> > > aren't part of the document tree.
> > 
> > Yeah, I left this part as is because I am not confident that the case
> > (animations still exist in the tree) is still valid or not when we use
> > EffectSet, and I have no idea when the case happens/happened.
> > test_timeline_get_animations covers the case?, or bug 1232829 was related to?
> 
> I don't think we need that any more. It was most likely needed because the
> number of animations returned by document.timeline.getAnimations() was
> sometimes wrong due to animations that were running on elements no longer
> bound to the tree. That's no longer going to happen with this new
> implementation of getAnimations().
> 
> > I just filed bug 1234481 for that.
> 
> I think you should fix it here rather than check in code that calls
> functions that no longer exist.

OK. This patch removes it.
Attachment #8700959 - Flags: review?(bbirtles)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1234481
(Reporter)

Updated

3 years ago
Attachment #8700959 - Flags: review?(bbirtles) → review+
Comment on attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

So the spec still doesn't have getAnimations() in Document interface, even though https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html was sent months ago. I assume it is still the plan to change the spec, right?

What kinds of interfaces does blink use here? At least their dev builds have getAnimations() in Element. What about release versions?

It is always harder or less-comfortable to r+ webidl changes for something not defined in any spec.

hiikezoe, could you please check what blink ships and where, and whether
Element.getAnimations will be in release builds soon. Because if so, we may not be able to do the change.
And then re-ask review if they aren't going to ship what is in the spec, but what was proposed in the email.
Attachment #8700901 - Flags: review?(bugs) → feedback?(hiikezoe)
Comment on attachment 8700904 [details] [diff] [review]
Part 3:  Remove AnimationTimeline.getAnimations.

Same applies to this patch.
Attachment #8700904 - Flags: review?(bugs)
(Reporter)

Comment 37

3 years ago
(In reply to Olli Pettay [:smaug] from comment #35)
> Comment on attachment 8700901 [details] [diff] [review]
> Part 1: Implement Document.getAnimations()
> 
> So the spec still doesn't have getAnimations() in Document interface, even
> though https://lists.w3.org/Archives/Public/public-fx/2015JulSep/0073.html
> was sent months ago. I assume it is still the plan to change the spec, right?

I've updated the spec now:

  http://w3c.github.io/web-animations/#extensions-to-the-document-interface
  (Changeset: https://github.com/w3c/web-animations/commit/2e7e1485b414255fece69840621314fe1e293c96)

> What kinds of interfaces does blink use here? At least their dev builds have
> getAnimations() in Element. What about release versions?

Right, but they don't expose that on release versions and there is no intent to ship that to release yet (same with us). I'll be meeting with Google early next month to discuss plans here.
(Assignee)

Comment 38

3 years ago
Thank you, Brian, for updating the spec.

(In reply to Olli Pettay [:smaug] from comment #35)
> hiikezoe, could you please check what blink ships and where, and whether
> Element.getAnimations will be in release builds soon. Because if so, we may
> not be able to do the change.
> And then re-ask review if they aren't going to ship what is in the spec, but
> what was proposed in the email.

In their document for their intents [1], there is no entry for Element.getAnimations(). (Actually it has been already implemented as Brian noted.)

[1] https://docs.google.com/spreadsheets/d/1pvXEMD5pRioognaqEzglS-4ZBSQ_YmzL8Fiz7yt4Bb4/edit#gid=0
(Assignee)

Comment 39

3 years ago
Comment on attachment 8700901 [details] [diff] [review]
Part 1: Implement Document.getAnimations()

I noticed that getAnimations() in webidl should be moved right below 'DocumentTimeline timeline' entry.
I will fix it once this patch gets review+.

Thank you,
Attachment #8700901 - Flags: feedback?(hiikezoe) → review?(bugs)
(Assignee)

Updated

3 years ago
Attachment #8700904 - Flags: review?(bugs)

Updated

3 years ago
Attachment #8700901 - Flags: review?(bugs) → review+

Updated

3 years ago
Attachment #8700904 - Flags: review?(bugs) → review+
(Assignee)

Comment 40

3 years ago
Created attachment 8704813 [details] [diff] [review]
Part 1: Implement Document.getAnimations()
Attachment #8700901 - Attachment is obsolete: true
Attachment #8700933 - Attachment is obsolete: true
Attachment #8704813 - Flags: review+
(Assignee)

Comment 41

3 years ago
Created attachment 8704814 [details] [diff] [review]
Part 2: Tests for document.getAnimations.
Attachment #8700903 - Attachment is obsolete: true
Attachment #8704814 - Flags: review+
(Assignee)

Comment 42

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af1ff2d054ff

Thank you, all!
Keywords: checkin-needed

Comment 44

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/832653b10478
https://hg.mozilla.org/mozilla-central/rev/26974dc2a13d
https://hg.mozilla.org/mozilla-central/rev/bf320c73ab1a
https://hg.mozilla.org/mozilla-central/rev/f2fefee9f84e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1309198
You need to log in before you can comment on or make changes to this bug.