1298755 - Drop ResetWinsInCascade() in KeyframeEffect::SetTarget()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

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().

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Automation tests to check results after changing composite order caused by changing target element

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 3

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c995ff21241

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1298755 - Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect.

The purpose of ResetWinsInCascade() in SetTarget() ensures that
RequestRestyle(Layer) is invoked for the new target in the case when
composite order or CSS cascading or order is changed due to chaging target
element, but RequestRestyle(Layer) will be called right after setting
the new target in SetTarget() explicitly.

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

Comment 6

[Brian Birtles (:birtles)]

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/

Comment 7

[Brian Birtles (:birtles)]

Do we need a test for setting target to null?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 9

[Brian Birtles (:birtles)]

(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?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(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 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786122 [details]
Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75086/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786123 [details]
Bug 1298755 - Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75088/diff/1-2/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 14

[Brian Birtles (:birtles)]

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 16

[Brian Birtles (:birtles)]

(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.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(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 18

[Brian Birtles (:birtles)]

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 19

[Brian Birtles (:birtles)]

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 20

[Hiroyuki Ikezoe (:hiro)]

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!

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786122 [details]
Bug 1298755 - Part 1: Automation tests to check results after changing composite order caused by changing target element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75086/diff/2-3/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8786123 [details]
Bug 1298755 - Part 2: Drop ResetWinsInCascade in KeyframeEffect::SetTarget() since it has no effect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75088/diff/2-3/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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