I have no idea what the test case is exactly yet. But we need it. An easy test case is that changing target which has !important style to another target which does not have !important style, and vice versa. But I guess it does not rely on ResetWinsInCascade() in KeyframeEffect::SetTarget().

OK. We need a test case like this; On Element A; a color animation On Element B; another color animation And changing the target of animation on Element B to Element A. Or On Element A; two color animations Then move an animation onto Element B.

This patch includes three test cases that I described in comment 1. We already have a test case that I described in comment 0. All of test cases in this patch pass without ResetWinsInCascade() in SetTarget(). I am inclined to remove ResetWinsInCascade().

Comment on attachment 8786122 [details] Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element. https://reviewboard.mozilla.org/r/75086/#review73068 ::: dom/animation/test/chrome/test_running_on_compositor.html:570 (Diff revision 1) > + var firstAnimation = div.animate({ opacity: [1, 0] }, 100 * MS_PER_SEC); > + var secondAnimation = div.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC); > + > + var another = addDiv(t); > + > + return secondAnimation.ready.then(function() { Nit: This might be slightly more reliable as: return Promise.all([firstAnimation.ready, secondAnimation.ready]).then(... ::: dom/animation/test/chrome/test_running_on_compositor.html:583 (Diff revision 1) > + 'running on the compositor after another animation has ' + > + 'been moved on another element'); ... after the previously overridden animation is applied to a different element. ::: dom/animation/test/chrome/test_running_on_compositor.html:586 (Diff revision 1) > + 'The first opacity animation on another element starts ' + > + 'running on the compositor'); The previously overridden opacity animation reports that it is running on the compositor after being applied to a different element. ::: dom/animation/test/chrome/test_running_on_compositor.html:589 (Diff revision 1) > + 'been moved on another element'); > + assert_equals(firstAnimation.isRunningOnCompositor, omtaEnabled, > + 'The first opacity animation on another element starts ' + > + 'running on the compositor'); > + }); > +}, 'Moving animation which was not running on the compositor due to ' + s/Moving/Active/ ? ::: dom/animation/test/chrome/test_running_on_compositor.html:612 (Diff revision 1) > + 'The second opacity animation which has been moved on ' + > + 'another element keeps running on the compositor'); The second opacity animation continue running on the compositor after being applied to a different element. ::: dom/animation/test/chrome/test_running_on_compositor.html:615 (Diff revision 1) > + 'The first opacity animation which has been staying on ' + > + 'the same element starts running on the compositor'); The previously overridden animation now reports that it is running on the compositor after the animation that was overriding it is applied to a different element. ::: dom/animation/test/chrome/test_running_on_compositor.html:618 (Diff revision 1) > +}, 'Animation which was not running on the compositor starts running on ' + > + 'the compositor after another animation which was higher composite ' + > + 'order on the element has been moved on another element'); Animation which was overridden and, as a result, not running on the compositor begins running on the compositor after the higher-priority animation is applied to a different element. ::: dom/animation/test/chrome/test_running_on_compositor.html:634 (Diff revision 1) > + 'Another opacity animation on another element reports ' + > + 'that it is running on the compositor'); Opacity animation running on a different element reports that it is running on the compositor. ::: dom/animation/test/chrome/test_running_on_compositor.html:641 (Diff revision 1) > + 'The opacity animation which has been moved on another ' + > + 'element keeps running on the compositor'); Animation continues to run on the compositor after being applied to a different element with a lower-priority animation. ::: dom/animation/test/chrome/test_running_on_compositor.html:644 (Diff revision 1) > + 'The opacity animation which has been staying the same ' + > + 'element stops running on the compositor'); Animation stops running on the compositor after a higher-priority animation originally applied to a different element is applied to the same element. ::: dom/animation/test/chrome/test_running_on_compositor.html:647 (Diff revision 1) > + 'element keeps running on the compositor'); > + assert_equals(animation.isRunningOnCompositor, false, > + 'The opacity animation which has been staying the same ' + > + 'element stops running on the compositor'); > + }); > +}, 'Moving an animation to an element which already has the same property ' + s/Moving an/Moving a higher-priority/

Do we need a test for setting target to null?

(In reply to Brian Birtles (:birtles) from comment #7) > Do we need a test for setting target to null? Though I didn't know, we already have it. http://hg.mozilla.org/mozilla-central/file/26e22af660e5/dom/animation/test/chrome/test_running_on_compositor.html#l443

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > (In reply to Brian Birtles (:birtles) from comment #7) > > Do we need a test for setting target to null? > > Though I didn't know, we already have it. > http://hg.mozilla.org/mozilla-central/file/26e22af660e5/dom/animation/test/ > chrome/test_running_on_compositor.html#l443 Why does this test pass without the call to ResetWinsInCascade? As far as I can see we only reset mWinsInCascade in: * KeyframeEffectReadOnly::UpdateProperties * EffectCompositor::UpdateCascadeResults But I would have expected that we don't call UpdateProperties from SetTarget when the new target is nullptr? And we would be removed from the EffectSet and so not included in other calls to UpdateProperties / UpdateCascadeResults?

(In reply to Brian Birtles (:birtles) from comment #9) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > (In reply to Brian Birtles (:birtles) from comment #7) > > > Do we need a test for setting target to null? > > > > Though I didn't know, we already have it. > > http://hg.mozilla.org/mozilla-central/file/26e22af660e5/dom/animation/test/ > > chrome/test_running_on_compositor.html#l443 > > Why does this test pass without the call to ResetWinsInCascade? As far as I > can see we only reset mWinsInCascade in: > > * KeyframeEffectReadOnly::UpdateProperties > * EffectCompositor::UpdateCascadeResults > > But I would have expected that we don't call UpdateProperties from SetTarget > when the new target is nullptr? > And we would be removed from the EffectSet and so not included in other > calls to UpdateProperties / UpdateCascadeResults? OK. As far as I can tell, the mWinsInCascade flag of the effect which are removed from an element will not be changed until the effect is associated with a new target. So a problem will happen when associating the new target. I will add the test case and see the result.

Comment on attachment 8786122 [details] Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element. The patch includes following three new test cases, it should be reviewed again. 1. apply to an element which has an important style once after setting null for its target 2. apply to an element which has a lower-priority animation once after setting null for its target 3. apply to an element which has a higher-priority animation once after setting null for its target All tests passed with and without ResetWinsInCascade() in SetTarget() locally. I think mWinsInCascade flag updates when associating with a new target.

I still don't understand, what clears mWinsInCascade when we set the target to null?

(In reply to Brian Birtles (:birtles) from comment #14) > I still don't understand, what clears mWinsInCascade when we set the target > to null? I don't mean the flag is cleared when setting the target to null. The flag will be updated when setting the target to an element again. Holding the previous flag value while the animation is isolated from any element will be a matter? If it's a matter, I will keep the ResetWinsIncascade() with test cases.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > (In reply to Brian Birtles (:birtles) from comment #14) > > I still don't understand, what clears mWinsInCascade when we set the target > > to null? > > I don't mean the flag is cleared when setting the target to null. The flag > will be updated when setting the target to an element again. Holding the > previous flag value while the animation is isolated from any element will be > a matter? If it's a matter, I will keep the ResetWinsIncascade() with test > cases. Oh, never mind, since it's not observable it's fine. I didn't realize we were using isRunningOnCompositor as a proxy for testing mWinsInCascade.

(In reply to Brian Birtles (:birtles) from comment #16) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > > (In reply to Brian Birtles (:birtles) from comment #14) > > > I still don't understand, what clears mWinsInCascade when we set the target > > > to null? > > > > I don't mean the flag is cleared when setting the target to null. The flag > > will be updated when setting the target to an element again. Holding the > > previous flag value while the animation is isolated from any element will be > > a matter? If it's a matter, I will keep the ResetWinsIncascade() with test > > cases. > > Oh, never mind, since it's not observable it's fine. I didn't realize we > were using isRunningOnCompositor as a proxy for testing mWinsInCascade. Yeah, there is no way to check mWinsInCascade directly. I was going to expose the flag once (bug 1252730), but the bug will be closed once we drop mWinsInCascade flag.

Comment on attachment 8786122 [details] Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element. https://reviewboard.mozilla.org/r/75086/#review73474 ::: dom/animation/test/chrome/test_running_on_compositor.html:678 (Diff revisions 1 - 2) > + return waitForFrame(); > + }).then(function() { > assert_equals(animation.isRunningOnCompositor, false, > - 'The opacity animation which has been staying the same ' + > - 'element stops running on the compositor'); > + 'Animation is NOT running on the compositor even after ' + > + 'being applied to a different element which has an ' + > + 'important style of opacity'); ...has an !important opacity declaration. ::: dom/animation/test/chrome/test_running_on_compositor.html:680 (Diff revisions 1 - 2) > -}, 'Moving an animation to an element which already has the same property ' + > - 'animation running on the compositor makes the initial animation stop ' + > - 'running on the compositor'); > +}, 'Animation continues not to running on the compositor after being ' + > + 'applied to an element which has an important style once after ' + > + 'disassociating with an element'); Animation continues not running on the compositor after being applied to an element which has an important declaration and having previously been temporarily associated with no target element. ::: dom/animation/test/chrome/test_running_on_compositor.html:691 (Diff revisions 1 - 2) > + var another = addDiv(t); > + > + var lowerAnimation = div.animate({ opacity: [1, 0] }, 100 * MS_PER_SEC); > + var higherAnimation = another.animate({ opacity: [0, 1] }, 100 * MS_PER_SEC); > + > + return Promise.all([lowerAnimation.ready, higherAnimation.ready]).then(function() { Nit: This looks longer than 80 chars ::: dom/animation/test/chrome/test_running_on_compositor.html:704 (Diff revisions 1 - 2) > + lowerAnimation.effect.target = null; > + return waitForFrame(); > + }).then(function() { > + assert_equals(lowerAnimation.isRunningOnCompositor, false, > + 'Animation is no longer running on the compositor after ' + > + 'removing from the element'); s/removing from/being removed from/ ::: dom/animation/test/chrome/test_running_on_compositor.html:709 (Diff revisions 1 - 2) > + 'removing from the element'); > + lowerAnimation.effect.target = another; > + return waitForFrame(); > + }).then(function() { > + assert_equals(lowerAnimation.isRunningOnCompositor, false, > + 'A lower-priority animation does NOT begin to running ' + s/begin to running/begin running/ ::: dom/animation/test/chrome/test_running_on_compositor.html:717 (Diff revisions 1 - 2) > +}, 'Animation continues not to running on the compositor after being ' + > + 'applied to an element which has a higher-priority animation once after ' + > + 'disassociating with an element'); Animation continues not running on the compositor after being applied to an element which has a higher-priority animation and after being temporarily associated with no target element. ::: dom/animation/test/chrome/test_running_on_compositor.html:741 (Diff revisions 1 - 2) > + higherAnimation.effect.target = null; > + return waitForFrame(); > + }).then(function() { > + assert_equals(higherAnimation.isRunningOnCompositor, false, > + 'Animation is no longer running on the compositor after ' + > + 'removing from the element'); s/removing from/being removed from/ ::: dom/animation/test/chrome/test_running_on_compositor.html:753 (Diff revisions 1 - 2) > + assert_equals(higherAnimation.isRunningOnCompositor, omtaEnabled, > + 'A higher-priority animation begins to running on the ' + > + 'compositor after being applied to an element which has ' + > + 'a lower-priority-animation'); > + }); > +}, 'Animation begins to running on the compositor after being applied ' + s/to running/running/ (In general it's either "to run" or "running", not "to running")

Comment on attachment 8786123 [details] Bug 1298755 - Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect. https://reviewboard.mozilla.org/r/75088/#review73478 ::: dom/animation/KeyframeEffect.cpp (Diff revision 2) > } > > if (mTarget) { > UnregisterTarget(); > ResetIsRunningOnCompositor(); > - ResetWinsInCascade(); Can we add a comment here along the lines of: We don't need to reset the mWinsInCascade member since it will be updated when we later associate with a different target (and until that time this flag is not used). ?

Comment on attachment 8786123 [details] Bug 1298755 - Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect. https://reviewboard.mozilla.org/r/75088/#review73478 > Can we add a comment here along the lines of: > > We don't need to reset the mWinsInCascade member since it will be updated when we later associate with a different target (and until that time this flag is not used). > > ? Added. Thanks for the great idea!

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/1949ccaea239 Part 1: Automation tests to check results after changing composite order caused by changing target element. r=birtles https://hg.mozilla.org/integration/autoland/rev/f4b63a81c8d5 Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect. r=birtles

https://hg.mozilla.org/mozilla-central/rev/1949ccaea239 https://hg.mozilla.org/mozilla-central/rev/f4b63a81c8d5