1067769 - Allow setting KeyframeEffect.target

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Boris Chiou [:boris]]

I'd like to split this bug into three parts:

1. Support nullable target
  * Part of our code handles null target properly, so I have to make sure the other part does as well.

2. Allow setting KeyframeEffect.target
  * Support this API, as the bug title says.

3. Test cases.

Comment 2

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

We set a null target only from Web Animations API, so make sure
KeyframeEffectReadOnly::ConstructKeyframeEffect() can handle null target
properly.

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

Comment 3

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 1: Avoid doing RequestRestyle and mutation batch for null target. r=birtles

We don't need to restyle if mTarget is nullptr, so skip it. For
AnimationMutationBatch, we also skip it if mTarget is nullptr.

      CSS Animations and CSS Transtions, so the effect should have an effective
      target. Therefore, I think we don't have to revise them.

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

Comment 4

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

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

Comment 5

[Boris Chiou [:boris]]

Hi, Brian,

Part 1 ~ Part 3 are only for supporting null target. I'm still working on setting target, but I think it's better to review them first. Thanks.

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

https://reviewboard.mozilla.org/r/46387/#review43253

r=me with comments addressed.

::: dom/animation/KeyframeEffect.cpp:767
(Diff revision 1)
> +  // Use doc as the parent if targetElement is null.
>    RefPtr<KeyframeEffectType> effect =
> -    new KeyframeEffectType(targetElement->OwnerDoc(), targetElement,
> -                           pseudoType, timingParams);
> +    new KeyframeEffectType(targetElement ? targetElement->OwnerDoc() : doc,
> +                           targetElement, pseudoType, timingParams);

This obviously needs to be specced [1] but I think it's probably best to just use |doc| here. At least that makes sense to me that the KeyframeEffect has a consistent global regardless of the target element used (or lack of target element) when it is initialized.

[1] https://github.com/w3c/web-animations/issues/145

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8741609 [details]
MozReview Request: Bug 1067769 - Part 1: Avoid doing RequestRestyle and mutation batch for null target. r=birtles

https://reviewboard.mozilla.org/r/46389/#review43257

r=me but I really want to know why that assertion is valid (and be sure we can never be in a situation where that might not hold)

::: dom/animation/KeyframeEffect.cpp:577
(Diff revision 1)
>      }
>    }
>  
>    if (mAnimation) {
>      nsPresContext* presContext = GetPresContext();
> -    if (presContext) {
> +    if (presContext && mTarget) {

Why not move this test up to where we check mAnimation?

Or, perhaps better still, move this whole block beginning with "if (mAnimation) {" inside the "if (mTarget) {" block above?

::: dom/animation/KeyframeEffect.cpp:1125
(Diff revision 1)
>  }
>  
>  bool
>  KeyframeEffectReadOnly::CanThrottleTransformChanges(nsIFrame& aFrame) const
>  {
> +  MOZ_ASSERT(mTarget);

We should have a comment or assertion description saying why we think this invariant holds.

::: dom/animation/KeyframeEffect.cpp:1382
(Diff revision 1)
>  
>  void KeyframeEffect::NotifySpecifiedTimingUpdated()
>  {
>    // Use the same document for a pseudo element and its parent element.
> -  nsAutoAnimationMutationBatch mb(mTarget->OwnerDoc());
> +  // Use nullptr if we don't have mTarget, so disable the mutation batch.
> +  nsAutoAnimationMutationBatch mb(mTarget ? mTarget->OwnerDoc() : nullptr);

(This is a somewhat minor point, but the order of these patches is not quite right. In part 1 we stop throwing when null is passed into the constructor--that means mTarget could be nullptr. If we just apply part 1 and this method gets called, we will end up crashing.

Sometimes it can be quite difficult, but as far as possible we should try to make each patch able to be applied in turn and compile, pass all tests, and not crash.

So in this bug it would be better if we made this change first, and only after it is safe for mTarget to be nullptr, update the constructor to allow it.

It's a minor point but it is a good goal in future. It makes bisecting easier, for example.)

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

https://reviewboard.mozilla.org/r/46435/#review43265

This is good but I want to just check the test for the finished promise and the test for the finish event.

We should probably also add a test that an animation observer record is *not* created for a new animation with a null target.

::: testing/web-platform/tests/web-animations/animation/constructor.html:67
(Diff revision 1)
> +  anim.currentTime += 2500;
> +  assert_equals(effect.getComputedTiming().progress, 0.25);
> +  anim.currentTime += 2500;
> +  assert_equals(effect.getComputedTiming().progress, 0.5);

I don't think that checking at both 0.25 and 0.5 adds any value to this test. Just checking at 0.5 would be fine.

We should also test that the finished promise is resolved and finish events are fired since that will be the primary way in which effects with a null target are used: as a means of synchronization.

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:712
(Diff revision 1)
> +                                          { left: ["10px", "20px"] },
> +                                          { duration: 10000,
> +                                            fill: "forwards" });
> +  assert_true(!!effect, "Create effect with null target successfully");
> +
> +  var anim = new Animation(effect, document.timeline);

We need to actually play() the animation before it would be even considered by document.getAnimations().

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

https://reviewboard.mozilla.org/r/46387/#review43269

::: dom/animation/KeyframeEffect.cpp:749
(Diff revision 1)
>      return nullptr;
>    }
>  
> -  if (aTarget.IsNull()) {
> -    // We don't support null targets yet.
> -    aRv.Throw(NS_ERROR_DOM_ANIM_NO_TARGET_ERR);
> +  RefPtr<Element> targetElement;
> +  CSSPseudoElementType pseudoType = CSSPseudoElementType::NotPseudo;
> +  if (!aTarget.IsNull()) {

We also need to update dom/base/domerr.msg and xpcom/base/ErrorList.h to drop NS_ERROR_DOM_ANIM_NO_TARGET_ERR.

Comment 10

[Brian Birtles (:birtles)]

Oh I don't understand why MozReview is so hard to use. I just wanted to add a comment and it cleared the review flag... Anyway part 1 is still r+

Comment 11

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46387/#review43253

> This obviously needs to be specced [1] but I think it's probably best to just use |doc| here. At least that makes sense to me that the KeyframeEffect has a consistent global regardless of the target element used (or lack of target element) when it is initialized.
> 
> [1] https://github.com/w3c/web-animations/issues/145

Got it. I will |doc| directly.

Comment 12

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46387/#review43269

> We also need to update dom/base/domerr.msg and xpcom/base/ErrorList.h to drop NS_ERROR_DOM_ANIM_NO_TARGET_ERR.

Got it. I will update them soon.

Comment 13

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46389/#review43257

> Why not move this test up to where we check mAnimation?
> 
> Or, perhaps better still, move this whole block beginning with "if (mAnimation) {" inside the "if (mTarget) {" block above?

Yes, it's much better.

> We should have a comment or assertion description saying why we think this invariant holds.

Indeed, if mTarget is nullptr, we will not call CanThrottleTransformChanges() from CanThrottle(). However, I think it is a fuction block and we should let people know not to call this if your mTarget is nullptr in the future. In the current implementation, we can drop this assertion, so I will remove it.

> (This is a somewhat minor point, but the order of these patches is not quite right. In part 1 we stop throwing when null is passed into the constructor--that means mTarget could be nullptr. If we just apply part 1 and this method gets called, we will end up crashing.
> 
> Sometimes it can be quite difficult, but as far as possible we should try to make each patch able to be applied in turn and compile, pass all tests, and not crash.
> 
> So in this bug it would be better if we made this change first, and only after it is safe for mTarget to be nullptr, update the constructor to allow it.
> 
> It's a minor point but it is a good goal in future. It makes bisecting easier, for example.)

Yes, you are right. I'd change the order of part 1 and part 2, so they will be better.

Comment 14

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review43265

> I don't think that checking at both 0.25 and 0.5 adds any value to this test. Just checking at 0.5 would be fine.
> 
> We should also test that the finished promise is resolved and finish events are fired since that will be the primary way in which effects with a null target are used: as a means of synchronization.

Got it. I will add one more test for it.

> We need to actually play() the animation before it would be even considered by document.getAnimations().

OK. I forgot to play it.

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8741609 [details]
MozReview Request: Bug 1067769 - Part 1: Avoid doing RequestRestyle and mutation batch for null target. r=birtles

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

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

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

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

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

Comment 18

[Brian Birtles (:birtles)]

Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

https://reviewboard.mozilla.org/r/46387/#review43529

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

https://reviewboard.mozilla.org/r/46435/#review43531

Looking good but I'd like to have a quick look at web-platform/tests/web-animations/document/getAnimations.html before we land this.

::: dom/animation/test/chrome/test_animation_observers.html:1703
(Diff revision 2)
> +addAsyncAnimTest("create_animation_without_target",
> +                 { observe: document, subtree: true }, function*() {
> +  var effect = new KeyframeEffectReadOnly(null,
> +                                          { opacity: [ 0, 1 ] },
> +                                          { duration: 10000 });
> +  var anim = new Animation(effect, document.timeline);

You need to play the animation before we'd consider dispatching an "added" record for it.

::: dom/animation/test/chrome/test_animation_observers.html:1706
(Diff revision 2)
> +                                          { opacity: [ 0, 1 ] },
> +                                          { duration: 10000 });
> +  var anim = new Animation(effect, document.timeline);
> +
> +  yield await_frame();
> +  assert_records([], "no records after animation is added");

After adding the call to play(), we should probably cancel the animation here and add a check that we don't get a removed record.

::: testing/web-platform/tests/web-animations/animation/finish.html:222
(Diff revision 2)
> +  return animation.ready.then(function() {
> +    animation.finish();
> +  }).then(function() {

This is probably fine, but maybe we should have a separate test for when the animation finishes normally?

The reason is that it's quite possible that an implementation would successfully resolve the finished promise when finish() is called but not during a normal tick.

So perhaps we could add a test that:

* sets the currentTime to animation.effect.getComputedTiming().endTime - 1
* waits 1 frame (or maybe 2 frames? I can't remember the exact sequencing of when the finished state is evaluated, promise callbacks are run, and rAF callbacks are run---there's probably a similar test elsewhere in this file we can follow, though)
* checks that the animation has finished

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:710
(Diff revision 2)
>  test(function(t) {
> +  var effect = new KeyframeEffectReadOnly(null,
> +                                          { left: ["10px", "20px"] },
> +                                          { duration: 10000,
> +                                            fill: "forwards" });
> +  assert_true(!!effect, "Create effect with null target successfully");

I don't think the constructor will ever return null without throwing so I don't know that this test makes sense.

How about we just add a test like the following:

  assert_equals(effect.target, null, "Effect created with null target has correct target");

::: testing/web-platform/tests/web-animations/keyframe-effect/constructor.html:712
(Diff revision 2)
> +  var anim = new Animation(effect, document.timeline);
> +  anim.play();
> +  assert_equals(document.getAnimations().length, 0,
> +                "document.getAnimations() doesn't return animations with " +
> +                "null target");

We should make this a separate test in a new file:

  web-platform/tests/web-animations/document/getAnimations.html

and put this test there (plus probably adding a few simple tests similar to those in dom/animation/test/css-animations/file_document-get-animations.html).

Comment 20

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review43531

> This is probably fine, but maybe we should have a separate test for when the animation finishes normally?
> 
> The reason is that it's quite possible that an implementation would successfully resolve the finished promise when finish() is called but not during a normal tick.
> 
> So perhaps we could add a test that:
> 
> * sets the currentTime to animation.effect.getComputedTiming().endTime - 1
> * waits 1 frame (or maybe 2 frames? I can't remember the exact sequencing of when the finished state is evaluated, promise callbacks are run, and rAF callbacks are run---there's probably a similar test elsewhere in this file we can follow, though)
> * checks that the animation has finished

OK. I will also add this separate test for null target.

> We should make this a separate test in a new file:
> 
>   web-platform/tests/web-animations/document/getAnimations.html
> 
> and put this test there (plus probably adding a few simple tests similar to those in dom/animation/test/css-animations/file_document-get-animations.html).

OK. I agree. Let me add web-platform/tests/web-animations/document/getAnimations.html.

Comment 21

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review43531

> OK. I will also add this separate test for null target.

BTW, it should wait 2 frames in accordance with other similar and my tests. 1 frame is not enough. Thanks.

Comment 22

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review43531

> OK. I agree. Let me add web-platform/tests/web-animations/document/getAnimations.html.

I will add the test for null target in document/getAniamtions.html in this patch. For other similar tests, from dom/animations/test/css-animations, I prefer to use another patch (i.e. part 4) to make my purpose clearer. Thanks.

Comment 23

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 4: Add some simple tests for document.getAnimation() in wpt. r=birtles

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

Comment 24

[Boris Chiou [:boris]]

Comment on attachment 8741609 [details]
MozReview Request: Bug 1067769 - Part 1: Avoid doing RequestRestyle and mutation batch for null target. r=birtles

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

Comment 25

[Boris Chiou [:boris]]

Comment on attachment 8741608 [details]
MozReview Request: Bug 1067769 - Part 2: Support nullable target in KeyframeEffect(ReadOnly) constructor. r=birtles

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

Comment 26

[Boris Chiou [:boris]]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

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

Comment 27

[Brian Birtles (:birtles)]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

https://reviewboard.mozilla.org/r/46435/#review44011

r=birtles with comments addressed

::: testing/web-platform/tests/web-animations/animation/finish.html:238
(Diff revisions 2 - 3)
> +    resolvedFinished = true;
> +  });
> +
> +  return animation.ready.then(function() {
> +    animation.currentTime = animation.effect.getComputedTiming().endTime - 1;
> +    animation.play();

We don't need this line, right?

::: testing/web-platform/tests/web-animations/document/getAnimations.html:5
(Diff revision 3)
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>document.getAnimations tests</title>
> +<link rel="help" href="https://w3c.github.io/web-animations/#dom-document-getanimations">
> +<link rel="author" title="Boris Chiou" href="[e-mail]">

We're not adding author lines anymore.

Comment 28

[Brian Birtles (:birtles)]

Comment on attachment 8742299 [details]
MozReview Request: Bug 1067769 - Part 4: Add some simple tests for document.getAnimation() in wpt. r=birtles

https://reviewboard.mozilla.org/r/47113/#review44023

r=birtles with comments addressed

::: testing/web-platform/tests/web-animations/document/getAnimations.html:31
(Diff revision 1)
> +  assert_equals(document.getAnimations().length, 1,
> +                'getAnimation returns a running animation');
> +
> +  var anim2 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  assert_equals(document.getAnimations().length, 2,
> +                'getAnimation returns running animations');

We should add a test here for the order. (In fact, that probably more important than the test on line 26-27 for document.getAnimations().length == 1. I think we could probably drop that.)

e.g.

  var anim1 = div.animate(...);
  var anim2 = div.animate(...);

  assert_array_equals(document.getAnimations(),
                      [ anim1, anim2 ],
                      'getAnimations() returns running animations');

::: testing/web-platform/tests/web-animations/document/getAnimations.html:37
(Diff revision 1)
> +
> +  anim1.finish();
> +  anim2.finish();
> +  assert_equals(document.getAnimations().length, 0,
> +                'getAnimation only returns running animations');
> +}, 'Test document.getAnimations for script generated animations')

Nit: script-generated

::: testing/web-platform/tests/web-animations/document/getAnimations.html:39
(Diff revision 1)
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim1 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  var anim2 = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +  var anims = document.getAnimations();
> +  assert_equals(anims[0], anim1, 'first generated animation');
> +  assert_equals(anims[1], anim2, 'second generated animation');
> +}, 'Test the order of document.getAnimations with script generated animations')

Oh, I see you are testing the order here! I think we could probably simplify this as suggested above and make it the one test.

Comment 29

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review44011

> We don't need this line, right?

After setting currentTime, the playState is still "paused", so I found I have to play it explicity, or we will not receive finished event.

> We're not adding author lines anymore.

Sure. I will remove it.

Comment 30

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/47113/#review44023

> We should add a test here for the order. (In fact, that probably more important than the test on line 26-27 for document.getAnimations().length == 1. I think we could probably drop that.)
> 
> e.g.
> 
>   var anim1 = div.animate(...);
>   var anim2 = div.animate(...);
> 
>   assert_array_equals(document.getAnimations(),
>                       [ anim1, anim2 ],
>                       'getAnimations() returns running animations');

OK. I will drop line 26-27, and simplify the test of order according to your suggestion. Thanks.

Comment 31

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/46435/#review44011

> After setting currentTime, the playState is still "paused", so I found I have to play it explicity, or we will not receive finished event.

Oh right, we never played it to begin with. We should probably just play it when we create the animation.

Comment 32

[Boris Chiou [:boris]]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

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

Comment 33

[Boris Chiou [:boris]]

Comment on attachment 8742299 [details]
MozReview Request: Bug 1067769 - Part 4: Add some simple tests for document.getAnimation() in wpt. r=birtles

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

Comment 34

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/46435/#review44011

> Oh right, we never played it to begin with. We should probably just play it when we create the animation.

OK. I will move the play() after creation.

Comment 35

[Boris Chiou [:boris]]

Comment on attachment 8741610 [details]
MozReview Request: Bug 1067769 - Part 3: Test for KeyframeEffectReadOnly with null target. r=birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46435/diff/4-5/

Comment 36

[Boris Chiou [:boris]]

Comment on attachment 8742299 [details]
MozReview Request: Bug 1067769 - Part 4: Add some simple tests for document.getAnimation() in wpt. r=birtles

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

Comment 37

[Boris Chiou [:boris]]

Hi Brian,

I have a problem about the state of the old target after setting a new target for a specific effect.

e.g.

var anim = div.animate({ 'marginLeft': ['0px', '100px'] }, 4000);
anim.play();

anim.ready.then(function() {
  anim.currentTime = 3000;
  console.log(getComputedStyle(div).marginLeft);  // restyle, and the output is '75px'
  anim.effect.target = null;  // set target to null

  // what is the expected value or marginLeft now?
});

I checked the spec and I didn't see the expected behavior of setting an effect target for this case. (Do I miss anything?)

In most cases, after finishing or canceling an animation, the computed value (e.g. marginLeft) will become the initial value. But after we set the target as null, I think this animation/effect shouldn't do anything to the original target, and we have to keep this animation going, so should we set the marginLeft of the original target back to the initial value, (just as cancel())? Or keep the marginLeft being its last value after restyling, so its value in the above example is still '75px', not initial value, '0px'.

Thanks.

Comment 38

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #37)
> Hi Brian,
> 
> I have a problem about the state of the old target after setting a new
> target for a specific effect.
> 
> e.g.
> 
> var anim = div.animate({ 'marginLeft': ['0px', '100px'] }, 4000);
> anim.play();
> 
> anim.ready.then(function() {
>   anim.currentTime = 3000;
>   console.log(getComputedStyle(div).marginLeft);  // restyle, and the output
> is '75px'
>   anim.effect.target = null;  // set target to null
> 
>   // what is the expected value or marginLeft now?

It should be its initial value.

> I checked the spec and I didn't see the expected behavior of setting an
> effect target for this case. (Do I miss anything?)

I think the relevant section is: https://w3c.github.io/web-animations/#model-liveness

> In most cases, after finishing or canceling an animation, the computed value
> (e.g. marginLeft) will become the initial value. But after we set the target
> as null, I think this animation/effect shouldn't do anything to the original
> target,

Yes, when we compose the animation value for marginLeft we will find no animation effects targeting that property and will simply set it to its base value (initial value)

> and we have to keep this animation going, so should we set the
> marginLeft of the original target back to the initial value, (just as
> cancel())? Or keep the marginLeft being its last value after restyling, so
> its value in the above example is still '75px', not initial value, '0px'.

It should be '0px'.

Comment 39

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #38)
> Yes, when we compose the animation value for marginLeft we will find no
> animation effects targeting that property and will simply set it to its base
> value (initial value)

Thanks, Brian. Looks like I have to make sure we set the property of the old element to its base value if no animation targeting it.

Comment 40

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #39)
> (In reply to Brian Birtles (:birtles) from comment #38)
> > Yes, when we compose the animation value for marginLeft we will find no
> > animation effects targeting that property and will simply set it to its base
> > value (initial value)
> 
> Thanks, Brian. Looks like I have to make sure we set the property of the old
> element to its base value if no animation targeting it.

Sorry. Do you know how do I get the base value (the original value from CSS rule, if we set it as 10px, not 0px) of a specific animation property in KeyframeEffect? Or where is the normal flow for resetting a finished/canceled animation property? Thanks.

Comment 41

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #38)
> Yes, when we compose the animation value for marginLeft we will find no
> animation effects targeting that property and will simply set it to its base
> value (initial value)

Hi Brian,

Sorry, I still have some questions about this. I'm thinking what is the most suitable way to set it to its base value. (Apparently, according to the current implementation, setting mTarget to nullptr directly cannot let it back to its base value.)

Firstly, I'm not familiar with that how to cascade the value from animation interpolation and the value from original css rule, so could you please give me some hints about this part? How do we override the interpolation value on the animation property. We compute the interpolation value in KeyframeEffectReadOnly::ComposeStyle(), and add the new value to the AnimValuesStyleRule which will map them into rule data. After finishing/canceling the animation, how do we remove the computed value from this property? So the property value becomes the original css value.

Thanks.

Comment 42

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #39)
> (In reply to Brian Birtles (:birtles) from comment #38)
> > Yes, when we compose the animation value for marginLeft we will find no
> > animation effects targeting that property and will simply set it to its base
> > value (initial value)
> 
> Thanks, Brian. Looks like I have to make sure we set the property of the old
> element to its base value if no animation targeting it.

I think you should only need to call prescontext->EffectCompositor()->RequestRestyle(elem, pseudo, EffectCompositor::RestyleType::Layer, mAnimation->CascadeLevel()). Basically the same thing we do at the end of KeyframeEffect::NotifySpecifiedTimingUpdated. Of course you'll need to keep a reference to elem / pseudo if you've already cleared mTarget / mPseudoType.

You certainly shouldn't need to set the base value directly.

Comment 43

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #42)
> I think you should only need to call
> prescontext->EffectCompositor()->RequestRestyle(elem, pseudo,
> EffectCompositor::RestyleType::Layer, mAnimation->CascadeLevel()). Basically
> the same thing we do at the end of
> KeyframeEffect::NotifySpecifiedTimingUpdated. Of course you'll need to keep
> a reference to elem / pseudo if you've already cleared mTarget / mPseudoType.
> 
> You certainly shouldn't need to set the base value directly.

Actually, I tried to call RequestRestyle(..), but it may be not enough.

e.g.
I declare two new members
nsCOMPtr<Element> mRemovedTarget;
CSSPseudoElementType mRemovedPseudoType;

void SetTarget()
{
  // if set to null
  // backup first
  mRemovedTarget = mTarget;
  mRemovedPseudoType = mPseudoType;
  mTarget = nullptr;
  mPseudoType = CSSPseudoElementType::NotPseudo;

  // Request restyle
  presContext->EffectCompositor()->
    RequestRestyle(mRemovedTarget, mRemovedPseudoType,
                   EffectCompositor::RestyleType::Layer,
                   mAnimation->CascadeLevel());
}

The marginLeft is still '75px' after calling SetTarget(null) in the example (comment 37) and it keeps the value (75px) forever. Therefore, I think there are sill some bugs for these case. Maybe I have to check why we don't apply the base value back if mTarget becomes null, so I may need some hints for this part. Thanks.

Comment 44

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #43)
> e.g.
> I declare two new members
> nsCOMPtr<Element> mRemovedTarget;
> CSSPseudoElementType mRemovedPseudoType;

Why aren't local variables enough?

> void SetTarget()
> {
>   // if set to null
>   // backup first
>   mRemovedTarget = mTarget;
>   mRemovedPseudoType = mPseudoType;
>   mTarget = nullptr;
>   mPseudoType = CSSPseudoElementType::NotPseudo;
> 
>   // Request restyle
>   presContext->EffectCompositor()->
>     RequestRestyle(mRemovedTarget, mRemovedPseudoType,
>                    EffectCompositor::RestyleType::Layer,
>                    mAnimation->CascadeLevel());

I think we could call RequestRestyle before update mTarget / mPseudoType?

> The marginLeft is still '75px' after calling SetTarget(null) in the example
> (comment 37) and it keeps the value (75px) forever. Therefore, I think there
> are sill some bugs for these case. Maybe I have to check why we don't apply
> the base value back if mTarget becomes null, so I may need some hints for
> this part. Thanks.

Yes, you'll need to debug that one. I suspect we're doing an early return somewhere when we see mTarget is nullptr.

A few things we probably ought to be doing when we clear the target are:

* Some subset of KeyframeEffectReadOnly::UpdateTargetRegistration where we remove ourselves from the EffectSet.
* (Possibly calling effectSet->MarkCascadeNeedsUpdate() although hopefully calling RemoveEffect does that.)
* Reset the mIsRunningOnCompositor / mWinsInCascade member on all the animation properties.
  * You can call ResetIsRunningOnCompositor for the former.
  * For the latter, normally EffectCompositor::UpdateCascadeResults would do it, but I think we'll need to do it ourselves here.
* You *may* need to trigger a call to mAnimation->mTimeline->NotifyAnimationUpdated(mAnimation) but I'm not sure. Hopefully not.

Comment 45

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #44)
> > nsCOMPtr<Element> mRemovedTarget;
> > CSSPseudoElementType mRemovedPseudoType;
> 
> Why aren't local variables enough?

Oh yes, I will use local.

> A few things we probably ought to be doing when we clear the target are:
> 
> * Some subset of KeyframeEffectReadOnly::UpdateTargetRegistration where we
> remove ourselves from the EffectSet.
> * (Possibly calling effectSet->MarkCascadeNeedsUpdate() although hopefully
> calling RemoveEffect does that.)
> * Reset the mIsRunningOnCompositor / mWinsInCascade member on all the
> animation properties.
>   * You can call ResetIsRunningOnCompositor for the former.
>   * For the latter, normally EffectCompositor::UpdateCascadeResults would do
> it, but I think we'll need to do it ourselves here.
> * You *may* need to trigger a call to
> mAnimation->mTimeline->NotifyAnimationUpdated(mAnimation) but I'm not sure.
> Hopefully not.

OK. I think I only need to do these:
1. RemoveEffect, if it is empty, also call DestroyEffectSet().
2. RequestRestyle for the original element
3. Set mTarget = nullptr.

By these three steps, I can reset the animation property of the original element to its base value, and looks like I don't have to reset mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)

Thanks, these hints are really helpfully.

Comment 46

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #44)
> * Some subset of KeyframeEffectReadOnly::UpdateTargetRegistration where we
> remove ourselves from the EffectSet.
> * (Possibly calling effectSet->MarkCascadeNeedsUpdate() although hopefully
> calling RemoveEffect does that.)
> * Reset the mIsRunningOnCompositor / mWinsInCascade member on all the
> animation properties.
>   * You can call ResetIsRunningOnCompositor for the former.
>   * For the latter, normally EffectCompositor::UpdateCascadeResults would do
> it, but I think we'll need to do it ourselves here.

OK. I think I only need to do these:
1. RemoveEffect, if it is empty, also call DestroyEffectSet().
2. RequestRestyle for the original element
3. Set mTarget = nullptr.

By these three steps, I can reset the animation property of the original element to its base value, and looks like I don't have to reset mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)

Thanks, these hints are really helpful.

Comment 47

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #46)
> By these three steps, I can reset the animation property of the original
> element to its base value, and looks like I don't have to reset
> mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)

Yes, you need to reset those. We should probably add a test that calling the chrome-only getProperties() method returns false for 'runningOnCompositor' immediately after setting target = null.

Comment 48

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #47)
> > Looks like I don't have to reset
> > mIsRunningOnCompositor / mWindsInCascade. (Or resetting would be better?)
> 
> Yes, you need to reset those. We should probably add a test that calling the
> chrome-only getProperties() method returns false for 'runningOnCompositor'
> immediately after setting target = null.

I see. I should add a test for it.

Comment 49

[Boris Chiou [:boris]]

I'm writing the mutation observer for setting a new target, and I think there are 4 cases:
1. null -> null      : do nothing
2. null -> non-null  : add this animation to the new target  
3. non-null->null    : remove this animation from the old target
4. non-null->non-null: remove this animation from the old target and add this animation to the new target.

Does it make sense? I will implement it by the above rule.

Comment 50

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 5: Support setting KeyframeEffect.target webidl interface. r=smaug

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

Comment 51

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles

SetTarget also need to unpack Nullabe<ElementOrCSSPseudoElement>, so I add this
help function.

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

Comment 52

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles

Handle two cases when the original target is null:
1. new target is non-null. Assign the new target, register new target and
   restyle it.
2. new target is null. Do nothing.

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

Comment 53

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 8: Implement SetTarget(), when the original target is non-null

We have two cases when the original target is non-null:
1. New target is null. We have to make sure the value of the animation
   properties of of the original target is back to the base value.
2. New target is another valid element. Call ResetTarget() to the original
   target and then assign the new target.

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

Comment 54

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 11: Implement animation mutation observer while setting the target. r=birtles

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

Comment 55

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles

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

Comment 56

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 14: Test for our animation mutation observer. r=birtles

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

Comment 57

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles

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

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8744269 [details]
MozReview Request: Bug 1067769 - Part 5: Support setting KeyframeEffect.target webidl interface. r=smaug

https://reviewboard.mozilla.org/r/48427/#review45217

Comment 63

[Brian Birtles (:birtles)]

Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles

https://reviewboard.mozilla.org/r/48429/#review45395

::: dom/animation/KeyframeEffect.cpp:687
(Diff revision 1)
> +static void
> +UnpackTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget,
> +             RefPtr<Element>& aElement,
> +             CSSPseudoElementType& aPseudoType)

Why don't we just make this return a Maybe<NonOwningAnimationTarget> and call it ConvertTarget or something like that?

That seems like it would be simpler and avoid some odd states in this current set up where, for example, if aTarget is null, we initialize aElement but not aPseudoType.

Comment 64

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/48429/#review45399

::: dom/animation/KeyframeEffect.cpp:687
(Diff revision 1)
> +static void
> +UnpackTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget,
> +             RefPtr<Element>& aElement,
> +             CSSPseudoElementType& aPseudoType)

Looking at subsequent patches, it might be better still to introduce an OwningAnimationTarget and return that instead.

Comment 65

[Brian Birtles (:birtles)]

Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles

https://reviewboard.mozilla.org/r/48431/#review45397

::: dom/animation/KeyframeEffect.h:439
(Diff revision 1)
>    void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
>  
>  protected:
>    ~KeyframeEffect() override;
> +
> +private:

Unless there's a reason to make this private, I think protected would be better here, but see my comments later--I think we could probably get away without adding this method.

::: dom/animation/KeyframeEffect.cpp:1374
(Diff revision 1)
> -  // TODO: fix in next patch
> +  RefPtr<Element> newTarget;
> +  CSSPseudoElementType newPseudoType = CSSPseudoElementType::NotPseudo;
> +  UnpackTarget(aTarget, newTarget, newPseudoType);
> +
> +  if (mTarget == newTarget && mPseudoType == newPseudoType) {
> +    // Assign the same target, skip it.
> +    return;
> +  }

(This would become simpler if ConvertTarget returns an Maybe<(Non)OwningAnimationTarget> and we make mTarget itself a Maybe<OwningAnimationTarget> and we use the operator== on that for comparison.)

::: dom/animation/KeyframeEffect.cpp:1383
(Diff revision 1)
> +  if (mTarget) {
> +    // TODO: fix in the next patch
> +    MOZ_ASSERT(false, "not implemented yet");
> +  } else if (newTarget) {
> +    // Replace null target with a valid element
> +    AssignTarget(newTarget, newPseudoType);
> +  }

Looking at this patch and the next, I wonder if it would be more clear if we split out the various utility methods and kept the flow in this method. What do you think about something like the following:

void
KeyframeEffect::SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget)
{
  Maybe<NonOwningAnimationTarget> target = ConvertTarget(target);
  if (target == mTarget) {
    return;
  }

  if (mTarget) {
    UnregisterTarget();
    ResetIsRunningOnCompositor();
    ResetWinsInCascade();

    RequestRestyle(EffectCompositor::RestyleType::Layer);
  }

  mTarget = target;

  if (mTarget) {
    UpdateTargetRegistration();
    MaybeUpdateProperties();

    RequestRestyle(EffectCompositor::RestyleType::Layer);
  }
}

void
KeyframeEffect::UnregisterTarget()
{
  EffectSet* effectSet =
    EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
  if (effectSet) {
    effectSet->RemoveEffect(*this);
    if (effectSet->IsEmpty()) {
      EffectSet::DestroyEffectSet(mTarget, mPseudoType);
    }
  }
}

void
KeyframeEffect::RequestRestyle(RestyleType aRestyleType)
{
  nsPresContext* presContext = GetPresContext();
  if (presContext && mAnimation) {
    presContext->EffectCompositor()->
      RequestRestyle(mTarget->mElement, mTarget->mPseudoType,
                      aRestyleType, mAnimation->CascadeLevel());
  }
}

void
KeyframeEffect::MaybeUpdateProperties()
{
  // We need to be careful to *not* call this when we are processing restyles
  // since calling GetStyleContextForElement for element when we are in the
  // process of building a style context may trigger various forms of infinite
  // recursion.

  nsIDocument* doc = mTarget->mElement->OwnerDoc();
  if (!doc) {
    return;
  }

  nsIAtom* pseudo = mPseudoType < CSSPseudoElementType::Count ?
                    nsCSSPseudoElements::GetPseudoAtom(mPseudoType) :
                    nullptr;
  RefPtr<nsStyleContext> styleContext =
    nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo,
                                                  doc->GetShell());
  if (!styleContext) {
    return;
  }

  UpdateProperties(styleContext);
}

?

::: dom/animation/KeyframeEffect.cpp:1405
(Diff revision 1)
>  
> +void
> +KeyframeEffect::AssignTarget(Element* aNewTarget,
> +                             CSSPseudoElementType aNewPseudoType)
> +{
> +  MOZ_ASSERT(aNewTarget, "Only assign a valid target");

So this is ok because we don't support assigning when the original target is null and we return early when the new target and original are the same, I guess?

::: dom/animation/KeyframeEffect.cpp:1412
(Diff revision 1)
> +  if (mAnimation) {
> +    mAnimation->UpdateRelevance();
> +  }

This isn't needed, right? Relevance doesn't depend on having a target or not, right?

Comment 66

[Brian Birtles (:birtles)]

Comment on attachment 8744273 [details]
MozReview Request: Bug 1067769 - Part 11: Implement animation mutation observer while setting the target. r=birtles

https://reviewboard.mozilla.org/r/48435/#review45411

Comment 67

[Brian Birtles (:birtles)]

Just to summarize my main feedback so far, I think we should:

* Introduce an OwningAnimationTarget type that has a RefPtr<Element> member
  (If needed, we can add operator== and operator= members that take
  a NonOwningAnimationTarget type -- but I think we might not need these)

* Replace the KeyframeEffectReadOnly mTarget/mPseudoType members with
  a Maybe<OwningAnimationTarget> mTarget member.

  We can probably leave KeyframeEffectReadOnly::GetTarget() as returning a
  Maybe<NonOwningAnimationTarget>. So maybe we do need a converting assignment
  operator like the following:

   NonOwningAnimationTarget&
   NonOwningAnimationTarget::operator=(const OwningAnimationTarget& aOther)

  (Maybe we should rename NonOwningAnimationTarget.h to AnimationTarget.h and
   put both NonOwningAnimationTarget and OwningAnimationTarget in the same
   file?)

* Add a ConvertTarget helper method that takes a
  Nullable<ElementOrCSSPseudoElement> argument and returns a
  Maybe<OwningAnimationTarget> object.

Comment 68

[Brian Birtles (:birtles)]

Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles

https://reviewboard.mozilla.org/r/48437/#review45413

This looks good but I'd like to take another look if you decide to simplify the tests at all.

::: testing/web-platform/meta/MANIFEST.json:28881
(Diff revision 1)
> +        "path": "web-animations/keyframe-effect/setTarget.html",
> +        "url": "/web-animations/keyframe-effect/setTarget.html"
> +      },
> +      {

Did you use ./mach web-platform-test --manifest-update for this? I think it normally adds tests to an "added_tests" section of some sort. I'm not sure if that matters.

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:28
(Diff revision 1)
> +  anim.finish();
> +  assert_equals(getComputedStyle(div).marginLeft, '10px',
> +                'Value if finished');

Do we need this test? Isn't the 50% test enough?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:33
(Diff revision 1)
> +test(function(t) {
> +  var div = createDiv(t);
> +  div.style.marginLeft = '10px';
> +  var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> +  anim.currentTime = 50 * MS_PER_SEC;
> +  anim.effect.target = div;
> +  assert_equals(getComputedStyle(div).marginLeft, '50px',
> +                'Value at 50% progress')
> +  anim.finish();
> +  assert_equals(getComputedStyle(div).marginLeft, '10px',
> +                'Value if finished')
> +}, 'Test setting the same target');

Do we need this test?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:56
(Diff revision 1)
> +  assert_equals(getComputedStyle(div).marginLeft, '50px',
> +                'Value at 50% progress after setting new target');

We should probably test getComputedStyle(div).marginLeft immediately before changing the target?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:59
(Diff revision 1)
> +  anim.finish();
> +  assert_equals(getComputedStyle(div).marginLeft, '10px',
> +                'Value if finished')

Again, I'm not sure exactly why this is needed?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:79
(Diff revision 1)
> +  var div = createDiv(t);
> +  var div2 = createDiv(t);

Nit: Call them 'a' and 'b'? 'divA' and 'divB'?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:86
(Diff revision 1)
> +  div.style.marginLeft = '10px';
> +  div2.style.marginLeft = '20px';
> +  var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> +  anim.currentTime = 50 * MS_PER_SEC;
> +  anim.effect.target = div2;

We should probably test the result of getComputedStyle before changing the target?

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:98
(Diff revision 1)
> +  anim.finish();
> +  assert_equals(getComputedStyle(div2).marginLeft, '20px',
> +                'Value of new target if finished')

Again, not sure this is needed?

Comment 69

[Brian Birtles (:birtles)]

Comment on attachment 8744275 [details]
MozReview Request: Bug 1067769 - Part 14: Test for our animation mutation observer. r=birtles

https://reviewboard.mozilla.org/r/48439/#review45415

This is really good but we need to test that redundant changes don't trigger updates:
* null -> null
* target -> target

Comment 70

[Brian Birtles (:birtles)]

Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles

https://reviewboard.mozilla.org/r/48441/#review45417

This is good.

I think we also need to add a test that fails if we forget to call ResetWinsOnCompositor (assuming it is possible to create such a test!).

::: dom/animation/test/chrome/test_running_on_compositor.html:429
(Diff revision 1)
> +                  'on the compositor');
> +
> +    animation.effect.target = div;
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_equals(animation.isRunningOnCompositor, true,

Should this be s/true/omtaEnabled/ ?

::: dom/animation/test/chrome/test_running_on_compositor.html:436
(Diff revision 1)
> +                  'after setting a valid target');
> +  });
> +}, 'animation is added to the compositor when setting a valid target');
> +
> +promise_test(function(t) {
> +  var animation = addDivAndAnimate(t,

Nit: I'd rather we don't use addDivAndAnimate. I think we should remove that function in the future since it obscures what the test is doing.

::: dom/animation/test/chrome/test_running_on_compositor.html:445
(Diff revision 1)
> +  return animation.ready.then(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +                  'Animation reports that it is running on the compositor');
> +
> +    animation.effect.target = null;
> +    return waitForFrame();

I don't think we need to wait for a frame here, right?

Comment 71

[Brian Birtles (:birtles)]

Comment on attachment 8744272 [details]
MozReview Request: Bug 1067769 - Part 8: Implement SetTarget(), when the original target is non-null

https://reviewboard.mozilla.org/r/48433/#review45419

Clearing this review for now, since I think it might be slightly better to do something like the suggestion in comment 65.

Comment 72

[Boris Chiou [:boris]]

Thanks for your review. I will try your suggestion in comment 65. 

(In reply to Brian Birtles (:birtles) from comment #67)
> Just to summarize my main feedback so far, I think we should:
> 
> * Introduce an OwningAnimationTarget type that has a RefPtr<Element> member
>   (If needed, we can add operator== and operator= members that take
>   a NonOwningAnimationTarget type -- but I think we might not need these)

OK. I will introduce this type.

> 
> * Replace the KeyframeEffectReadOnly mTarget/mPseudoType members with
>   a Maybe<OwningAnimationTarget> mTarget member.
> 
>   We can probably leave KeyframeEffectReadOnly::GetTarget() as returning a
>   Maybe<NonOwningAnimationTarget>. So maybe we do need a converting
> assignment
>   operator like the following:
> 
>    NonOwningAnimationTarget&
>    NonOwningAnimationTarget::operator=(const OwningAnimationTarget& aOther)
> 
>   (Maybe we should rename NonOwningAnimationTarget.h to AnimationTarget.h and
>    put both NonOwningAnimationTarget and OwningAnimationTarget in the same
>    file?)

Good. I'd put them together and rename the file, and try to use them in KeyframeEffect.

> 
> * Add a ConvertTarget helper method that takes a
>   Nullable<ElementOrCSSPseudoElement> argument and returns a
>   Maybe<OwningAnimationTarget> object.

OK. I should replace unpackTarget with "ConvertTarget" and revise its API. Thanks.

Comment 73

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48431/#review45397

> This isn't needed, right? Relevance doesn't depend on having a target or not, right?

I added this because I saw UpdateTargetRegistration() might be asserted if the relevance is not up-to-date, and I'm not sure if there is any possiblibliy of resulting in this assertion while calling SetTarget(). Actually, we don't need this while assigning a new target. Thanks.

Comment 74

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48437/#review45413

> Did you use ./mach web-platform-test --manifest-update for this? I think it normally adds tests to an "added_tests" section of some sort. I'm not sure if that matters.

I will re-run --manifest-update and make sure it is in the right place.

Comment 75

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48439/#review45415

OK. I will add an extra test, "set_redundant_animation_target" for these two cases.

Comment 76

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48441/#review45417

This is an interesting problem. We don't have an API for getting the property value, mWinsInCascade. Should I add one for Chrome-only?

> Should this be s/true/omtaEnabled/ ?

OK

> Nit: I'd rather we don't use addDivAndAnimate. I think we should remove that function in the future since it obscures what the test is doing.

OK. I will use addDiv and div.animate().

> I don't think we need to wait for a frame here, right?

Yes, I should remove it.

Comment 77

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #76)
> https://reviewboard.mozilla.org/r/48441/#review45417
> 
> This is an interesting problem. We don't have an API for getting the
> property value, mWinsInCascade. Should I add one for Chrome-only?

No, that value is likely to change when we implement additive animation so I'd rather not add an API for it.

What I mean is we should try to create some other kind of test that would produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if it is possible to create such a test, however. That's because assuming we successfully call UpdateCascadeResults for *both* the old and new target (which I think we will do in UnregisterTarget and UpdateTargetRegistration) I guess this member will be updated then? So maybe we don't even need ResetIsRunningOnCompositor?

Comment 78

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #77)
> What I mean is we should try to create some other kind of test that would
> produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> it is possible to create such a test, however. That's because assuming we
> successfully call UpdateCascadeResults for *both* the old and new target
> (which I think we will do in UnregisterTarget and UpdateTargetRegistration)

I checked the log in this test case (i.e. test_running_on_compositor.html), and we will call EffectCompositor::UpdateCascadeResults() only for _new_ target. We removed the EffectSet of old target, so we will not call UpdateCascadeResults() for it.

> I guess this member will be updated then? So maybe we don't even need
> ResetIsRunningOnCompositor?

If I comment out "ResetIsRunningOnCompositor()" in SetTarget(), the following example fails:

promise_test(function(t) {
  var div = addDiv(t);
  var animation = div.animate({ opacity: [ 0, 1 ] }, 100 * MS_PER_SEC);

  return animation.ready.then(function() {
    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
                  'Animation reports that it is running on the compositor');

    animation.effect.target = null;
    assert_equals(animation.isRunningOnCompositor, false,               // Fail here.
                  'Animation reports that it is NOT running on the ' +
                  'compositor after setting null target');
  });
}, "...")

I think it results from the same reason. We remove the EffectSet of the old target, so the mIsRunningOnCompositor wouldn't be updated. Therefore, we need to call ResetIsRunningOnCompositor() in SetTarget().

Comment 79

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 6: Rename NonOwningAnimationTarget.h to AnimationTarget.h. r=birtles

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

Comment 80

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles

1. Define OwningAnimationTarget.
2. Replace the KeyframeEffectReadOnly::mTarget/mPseudoType members with
   a Maybe<OwningAnimationTarget> mTarget member.

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

Comment 81

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 9: Wrap RequestRestyle and UnregisterTarget. r=birtles

We will need to request a restyle and unregister the current target in
SetTarget(), and there are many duplicate code segments for them now, so wrap
them for reusing the code.

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

Comment 87

[Boris Chiou [:boris]]

Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles

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

Comment 88

[Boris Chiou [:boris]]

Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles

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

Comment 90

[Boris Chiou [:boris]]

Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles

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

Comment 92

[Boris Chiou [:boris]]

Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles

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

Comment 93

[Boris Chiou [:boris]]

Comment on attachment 8745283 [details]
MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles

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

Comment 101

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #78)
> (In reply to Brian Birtles (:birtles) from comment #77)
> > What I mean is we should try to create some other kind of test that would
> > produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> > it is possible to create such a test, however. That's because assuming we
> > successfully call UpdateCascadeResults for *both* the old and new target
> > (which I think we will do in UnregisterTarget and UpdateTargetRegistration)
> 
> I checked the log in this test case (i.e. test_running_on_compositor.html),
> and we will call EffectCompositor::UpdateCascadeResults() only for _new_
> target. We removed the EffectSet of old target, so we will not call
> UpdateCascadeResults() for it.

We should be calling UpdateCascadeResults on the old target. I guess we need to add that.

> > I guess this member will be updated then? So maybe we don't even need
> > ResetIsRunningOnCompositor?
> 
> If I comment out "ResetIsRunningOnCompositor()" in SetTarget(), the
> following example fails:

Sorry, that was a typo on my part. I meant ResetWinsInCascade.

Comment 102

[Brian Birtles (:birtles)]

Comment on attachment 8745282 [details]
MozReview Request: Bug 1067769 - Part 6: Rename NonOwningAnimationTarget.h to AnimationTarget.h. r=birtles

https://reviewboard.mozilla.org/r/48955/#review45889

Comment 103

[Brian Birtles (:birtles)]

Comment on attachment 8745283 [details]
MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles

https://reviewboard.mozilla.org/r/48957/#review45899

Nice.

Comment 104

[Brian Birtles (:birtles)]

Comment on attachment 8744270 [details]
MozReview Request: Bug 1067769 - Part 8: Add ConvertTarget function. r=birtles

https://reviewboard.mozilla.org/r/48429/#review45901

r=me with comments addressed

::: dom/animation/KeyframeEffect.cpp:464
(Diff revision 3)
> -      mPseudoType < CSSPseudoElementType::Count ?
> -      nsCSSPseudoElements::GetPseudoAtom(mPseudoType) : nullptr;
> +      mTarget->mPseudoType < CSSPseudoElementType::Count ?
> +      nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType) : nullptr;
>      styleContext =
> -      nsComputedDOMStyle::GetStyleContextForElement(mTarget, pseudo, shell);
> +      nsComputedDOMStyle::GetStyleContextForElement(mTarget->mElement,
> +                                                    pseudo, shell);

Does this part belong in the previous patch?

::: dom/animation/KeyframeEffect.cpp:739
(Diff revision 3)
> +    RefPtr<Element> temp = target.GetAsCSSPseudoElement().ParentElement();
> +    result.emplace(temp, target.GetAsCSSPseudoElement().GetType());

Is this temporary necessary? Perhaps it is because ParentElement returns an already_AddRefed value. Can we rename it to 'elem' or something other than 'temp' though?

::: dom/animation/KeyframeEffect.cpp:767
(Diff revision 3)
>    RefPtr<KeyframeEffectType> effect =
> -    new KeyframeEffectType(doc, targetElement, pseudoType, timingParams);
> +    new KeyframeEffectType(doc,
> +                           target ? target->mElement.get() : nullptr,
> +                           target ? target->mPseudoType
> +                                  : CSSPseudoElementType::NotPseudo,
> +                           timingParams);

We should probably just change the KeyframeEffect and KeyframeEffectReadOnly constructors to take a Maybe<(Non)OwningAnimationElement> parameter.
 
Otherwise we end up converting:

  OwningAnimationElement -> [Element, PseudoType] -> OwningAnimationElement

We can do that in a follow-up patch/bug, however.

Comment 105

[Brian Birtles (:birtles)]

Comment on attachment 8745284 [details]
MozReview Request: Bug 1067769 - Part 9: Wrap RequestRestyle and UnregisterTarget. r=birtles

https://reviewboard.mozilla.org/r/48959/#review45903

::: dom/animation/KeyframeEffect.h:369
(Diff revision 2)
> +  // Unregister current target, so it removes this effect from mTarget's
> +  // effectSet.

// Remove the current effect target from its EffectSet.

::: dom/animation/KeyframeEffect.h:373
(Diff revision 2)
>  
> +  // Unregister current target, so it removes this effect from mTarget's
> +  // effectSet.
> +  void UnregisterTarget();
> +
> +  // Request a restyle

I'm not sure this comment is really necessary.

Comment 106

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #101)
> (In reply to Boris Chiou [:boris]  from comment #78)
> > (In reply to Brian Birtles (:birtles) from comment #77)
> > > What I mean is we should try to create some other kind of test that would
> > > produce the wrong result if we fail to reset mWinsInCascade. I'm not sure if
> > > it is possible to create such a test, however. That's because assuming we
> > > successfully call UpdateCascadeResults for *both* the old and new target
> > > (which I think we will do in UnregisterTarget and UpdateTargetRegistration)
> > 
> > I checked the log in this test case (i.e. test_running_on_compositor.html),
> > and we will call EffectCompositor::UpdateCascadeResults() only for _new_
> > target. We removed the EffectSet of old target, so we will not call
> > UpdateCascadeResults() for it.
> 
> We should be calling UpdateCascadeResults on the old target. I guess we need
> to add that.

It looks like UnregisterTarget will call EffectSet::RemoveEffect which will call MarkCascadeNeedsUpdate so that should be enough in the case where we stop targetting a particular element but some other animation is still targetting it.

Comment 107

[Brian Birtles (:birtles)]

Comment on attachment 8744271 [details]
MozReview Request: Bug 1067769 - Part 10: Implement SetTarget(). r=birtles

https://reviewboard.mozilla.org/r/48431/#review45907

r=me with comments addressed

::: dom/animation/KeyframeEffect.cpp:1419
(Diff revision 3)
> +
> +  if (mTarget) {
> +    UnregisterTarget();
> +    ResetIsRunningOnCompositor();
> +    ResetWinsInCascade();
> +

I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.

We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:

1. Set mTarget to Nothing()
   (Fail to clear mProgressOnLastCompose)
2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
   with any EffectSet.
3. Update mTarget to some new element.
4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
   NotifyAnimationTimingUpdated) on the new element because
   mProgressOnLastCompose happens to match the current time.

I'm not sure if that can ever happen, however.

At (3) we'll request a restyle so we should be ok I think?

::: dom/animation/KeyframeEffect.cpp:1427
(Diff revision 3)
> +
> +    MaybeUpdateProperties();

Nit: Perhaps put the blank line before the RequestRestyle (or just get rid of it altogether). I only added blank lines in comment 65 to highlight the symmetry between the two branches: both tidy up internal state then request restyle.

::: dom/animation/KeyframeEffect.cpp:1445
(Diff revision 3)
>  }
>  
> +void
> +KeyframeEffect::MaybeUpdateProperties()
> +{
> +  // We need to be careful to *not* call this when we are processing restyles

Perhaps we could reword this somewhat--it's not so much about processing restyles as about building a style context.

  // We need to be careful to *not* call this when we are updating the style
  // context. That's because calling GetStyleContextForElement when we are in
  // the process of building a style context may trigger various forms of
  // infinite recursion.

We should also add a comment to SetTarget to say something like:

  // This method calls MaybeUpdateProperties which is not safe to use when
  // we are in the middle of updating style. If we need to use this when
  // updating style, we should pass the nsStyleContext into this method and use
  // that to update the properties rather than calling
  // GetStyleContextForElement.

Perhaps it's best to add that to the header file even?

::: dom/animation/KeyframeEffect.cpp:1450
(Diff revision 3)
> +  // We need to be careful to *not* call this when we are processing restyles
> +  // since calling GetStyleContextForElement for element when we are in the
> +  // process of building a style context may trigger various forms of infinite
> +  // recursion.
> +
> +  nsIDocument* doc = mTarget->mElement->OwnerDoc();

This should check for !mTarget right?

Comment 108

[Brian Birtles (:birtles)]

Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles

https://reviewboard.mozilla.org/r/48437/#review45909

Looks good but I wonder if we should be updating MANIFEST.json manually like this. We should probably use --manifest-update.

::: testing/web-platform/meta/MANIFEST.json:35307
(Diff revision 3)
> +        "web-animations/keyframe-effect/setTarget.html": [
> +          {
> +            "path": "web-animations/keyframe-effect/setTarget.html",
> +            "url": "/web-animations/keyframe-effect/setTarget.html"
> +          }
> +        ],

Did you use ./mach web-platform-tests --manifest-update ?

See comment 68.

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:18
(Diff revision 3)
> +
> +var gKeyFrames = { 'marginLeft': ['0px', '100px'] };
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  div.style.marginLeft = '10px';

Nit: I think we can drop this.

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:52
(Diff revision 3)
> +  div.style.marginLeft = '10px';
> +  var anim = div.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> +  anim.currentTime = 50 * MS_PER_SEC;
> +  assert_equals(getComputedStyle(div).marginLeft, '50px',
> +                'Value at 50% progress')

Nit: 'Value at 50% progress before clearing the target'

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:56
(Diff revision 3)
> +  assert_equals(getComputedStyle(div).marginLeft, '50px',
> +                'Value at 50% progress')
> +
> +  anim.effect.target = null;
> +  assert_equals(getComputedStyle(div).marginLeft, '10px',
> +                'Value after reset')

Nit: 'Value after clearing the target'

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:68
(Diff revision 3)
> +  b.style.marginLeft = '20px';
> +  var anim = a.animate(gKeyFrames, 100 * MS_PER_SEC);
> +
> +  anim.currentTime = 50 * MS_PER_SEC;
> +  assert_equals(getComputedStyle(a).marginLeft, '50px',
> +                'Value of the 1st target before setting')

Nit: These test messages could be a little more clear.

e.g.

'Value of 1st element (currently targeted) before changing the effect target'
'Value of 2nd element (currently not targeted) before changing the effect target'

::: testing/web-platform/tests/web-animations/keyframe-effect/setTarget.html:77
(Diff revision 3)
> +  anim.currentTime = 75 * MS_PER_SEC;
> +  assert_equals(getComputedStyle(a).marginLeft, '10px',
> +                'Value of the 1st target')
> +  assert_equals(getComputedStyle(b).marginLeft, '75px',
> +                'Value of the 2nd target')

These messages in particular could be a lot clearer. (Also, I wonder if we really need this part, but maybe we do.)

Comment 109

[Brian Birtles (:birtles)]

Comment on attachment 8744276 [details]
MozReview Request: Bug 1067769 - Part 15: Test for setting the target while running on the compositor. r=birtles

https://reviewboard.mozilla.org/r/48441/#review45911

Comment 110

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48429/#review45901

> Does this part belong in the previous patch?

Yes. Looks like I squashed it to the wrong patch. Sorry, I will fix it.

> Is this temporary necessary? Perhaps it is because ParentElement returns an already_AddRefed value. Can we rename it to 'elem' or something other than 'temp' though?

Yes, you're right. It's necessary. I will rename it to elem.

> We should probably just change the KeyframeEffect and KeyframeEffectReadOnly constructors to take a Maybe<(Non)OwningAnimationElement> parameter.
>  
> Otherwise we end up converting:
> 
>   OwningAnimationElement -> [Element, PseudoType] -> OwningAnimationElement
> 
> We can do that in a follow-up patch/bug, however.

OK. I will do it in a follow-up patch. Thanks.

Comment 111

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48431/#review45907

> I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.
> 
> We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:
> 
> 1. Set mTarget to Nothing()
>    (Fail to clear mProgressOnLastCompose)
> 2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
>    with any EffectSet.
> 3. Update mTarget to some new element.
> 4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
>    NotifyAnimationTimingUpdated) on the new element because
>    mProgressOnLastCompose happens to match the current time.
> 
> I'm not sure if that can ever happen, however.
> 
> At (3) we'll request a restyle so we should be ok I think?

For (3), yes. Actually, we have to request a restyle in (3), or we cannot pass the 4th test in setTarget.html.
I tried to reproduce your case, but at the next Animation:Tick(), the GetComputedTiming().mProgress is changed a litte (part of it might be due to my printfs), so we still request a restyle because the current progress is different from last unreset mProgressOnLastCompose. However, I think requesting a restyle in (3) is necessary to make sure we can show the update of effect.target immediately.

Therefore, I think not reseting mProgressOnLastCompose is OK here.

Comment 112

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris]  from comment #111)
> https://reviewboard.mozilla.org/r/48431/#review45907
> 
> > I thought I added a comment to this bug yesterday but I can't find it so I guess I forgot to submit it.
> > 
> > We might need to reset mProgressOnLastCompose here. I wonder if there is some sequence where we:
> > 
> > 1. Set mTarget to Nothing()
> >    (Fail to clear mProgressOnLastCompose)
> > 2. Skip calling KeyframeEffect::ComposeStyle because we're no longer registered
> >    with any EffectSet.
> > 3. Update mTarget to some new element.
> > 4. Fail to RequestRestyle (as part of Animation::Tick -> UpdateEffect ->
> >    NotifyAnimationTimingUpdated) on the new element because
> >    mProgressOnLastCompose happens to match the current time.
> > 
> > I'm not sure if that can ever happen, however.
> > 
> > At (3) we'll request a restyle so we should be ok I think?
> 
> For (3), yes. Actually, we have to request a restyle in (3), or we cannot
> pass the 4th test in setTarget.html.
> I tried to reproduce your case, but at the next Animation:Tick(), the
> GetComputedTiming().mProgress is changed a litte (part of it might be due to
> my printfs), so we still request a restyle because the current progress is
> different from last unreset mProgressOnLastCompose. However, I think
> requesting a restyle in (3) is necessary to make sure we can show the update
> of effect.target immediately.
> 
> Therefore, I think not reseting mProgressOnLastCompose is OK here.

I am guessing the reason is that mTarget check in NotifyAnimationTimingUpdated() is inside GetComputedTiming().mProgress != mProgressOnLastCompose in your part 1 patch.
I think we can also skip calling CanThrottle() if mTarget is null.  If we do the null check outside GetComputedTiming().mProgress != mProgressOnLastCompose check,  there might be some cases mProgressOnLastCompose is not cleared.

For your reference, I've written a reftest for a similar case in bug 1235002.

Comment 113

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48437/#review45909

> Did you use ./mach web-platform-tests --manifest-update ?
> 
> See comment 68.

Yes. I used ./mach web-platform-tests --manifest-update [file path]

My steps are:
1. Comment out this line [1] to prevent it from updating reftests.
2. ./mach web-platform-tests --manifest-update testing/web-platform/tests/web-animations/
3. Revert non-related parts.
e.g.
   "local_changes": {
     "deleted": [
-      "web-animations/keyframe-effect/getComputedTiming-currentIteration.html",
-      "web-animations/keyframe-effect/getComputedTiming-progress.html"
+      "web-animations/keyframe-effect/getComputedTiming-progress.html",
+      "web-animations/keyframe-effect/getComputedTiming-currentIteration.html"
     ],

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/testing/web-platform/tests/tools/manifest/manifest.py#166

Comment 114

[Brian Birtles (:birtles)]

Comment on attachment 8744274 [details]
MozReview Request: Bug 1067769 - Part 13: Test for setting the target in basic cases. r=birtles

https://reviewboard.mozilla.org/r/48437/#review45989

r=me with comments addressed

Comment 115

[Brian Birtles (:birtles)]

https://reviewboard.mozilla.org/r/48437/#review45909

> Yes. I used ./mach web-platform-tests --manifest-update [file path]
> 
> My steps are:
> 1. Comment out this line [1] to prevent it from updating reftests.
> 2. ./mach web-platform-tests --manifest-update testing/web-platform/tests/web-animations/
> 3. Revert non-related parts.
> e.g.
>    "local_changes": {
>      "deleted": [
> -      "web-animations/keyframe-effect/getComputedTiming-currentIteration.html",
> -      "web-animations/keyframe-effect/getComputedTiming-progress.html"
> +      "web-animations/keyframe-effect/getComputedTiming-progress.html",
> +      "web-animations/keyframe-effect/getComputedTiming-currentIteration.html"
>      ],
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/testing/web-platform/tests/tools/manifest/manifest.py#166

Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!

I tend to just do:

  ./mach web-platform-tests --manifest-update yer

To avoid running the tests.

Comment 116

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48437/#review45909

> Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!
> 
> I tend to just do:
> 
>   ./mach web-platform-tests --manifest-update yer
> 
> To avoid running the tests.

I will upload an extra patch for updating MANIFEST.json before applying this patch, so it will be clearer.

Comment 117

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #115)
> I tend to just do:
> 
>   ./mach web-platform-tests --manifest-update yer
> 
> To avoid running the tests.

Thanks. This command is helpful. :)

Comment 118

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris]  from comment #116)
> https://reviewboard.mozilla.org/r/48437/#review45909
> 
> > Oh sorry, you're right. I misread the context. Yeah, that bug in (1) is a really pain!
> > 
> > I tend to just do:
> > 
> >   ./mach web-platform-tests --manifest-update yer
> > 
> > To avoid running the tests.
> 
> I will upload an extra patch for updating MANIFEST.json before applying this
> patch, so it will be clearer.

No need, what you have is fine. It just wasn't obvious from the diff in MozReview that it was the "local_changes" part that we were touching.

Comment 119

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #112)
> I am guessing the reason is that mTarget check in
> NotifyAnimationTimingUpdated() is inside GetComputedTiming().mProgress !=
> mProgressOnLastCompose in your part 1 patch.
> I think we can also skip calling CanThrottle() if mTarget is null.  If we do
> the null check outside GetComputedTiming().mProgress !=
> mProgressOnLastCompose check,  there might be some cases
> mProgressOnLastCompose is not cleared.
> 
> For your reference, I've written a reftest for a similar case in bug 1235002.

Thanks, Hiro. I saw you just filed Bug 1267937 which may handle mProgressOnLastCompose properly.

Comment 120

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/48437/#review45909

> These messages in particular could be a lot clearer. (Also, I wonder if we really need this part, but maybe we do.)

Just want to make sure we change the property value corretly on the new target. However, I can drop the 1st target part. Thanks.

Comment 121

[Boris Chiou [:boris]]

Attachment: MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles

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

Comment 130

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #101)
> We should be calling UpdateCascadeResults on the old target. I guess we need
> to add that.

I forgot to add this in my current patch (part 10). I will add it later.

Comment 131

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #106)
> (In reply to Brian Birtles (:birtles) from comment #101)
> > We should be calling UpdateCascadeResults on the old target. I guess we need
> > to add that.
> 
> It looks like UnregisterTarget will call EffectSet::RemoveEffect which will
> call MarkCascadeNeedsUpdate so that should be enough in the case where we
> stop targetting a particular element but some other animation is still
> targetting it.

OK, so calling EffectSet::RemoveEffect() in UnregisterTarget is enough now. (And I don't have to add UpdateCascadeResults()).

I still have a patch, part 12, which replaces Element/CSSPseudoElementType with Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly)::KeyframeEffect(ReadOnly)(). I use |OwningAnimationTarget| here, so we don't have to write the conversion from Maybe<NonOwningAnimationTarget> to Maybe<OwningAnimationTarget>.

Comment 132

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris]  from comment #131)
> I still have a patch, part 12, which replaces Element/CSSPseudoElementType
> with Maybe<OwningAnimationTarget> in
> KeyframeEffect(ReadOnly)::KeyframeEffect(ReadOnly)(). I use
> |OwningAnimationTarget| here, so we don't have to write the conversion from
> Maybe<NonOwningAnimationTarget> to Maybe<OwningAnimationTarget>.

I'm thinking should I use rvalue reference and Move() for Maybe<OwningAnimationTarget> to reduce the memory copy? However, OwningAnimationTarget is not a big object, so I still use lvalue reference here.

Comment 133

[Brian Birtles (:birtles)]

Comment on attachment 8745939 [details]
MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles

https://reviewboard.mozilla.org/r/49173/#review46171

Looks good. Thanks.

One comment, is there any reason not to pass Maybe<OwningAnimationTarget> as a const ref?

r=me with that change made unless there is a good reason to use a non-const ref.

Comment 134

[Boris Chiou [:boris]]

https://reviewboard.mozilla.org/r/49173/#review46171

I should add _const_. Thanks. I will update them.

Comment 135

[Boris Chiou [:boris]]

Comment on attachment 8745939 [details]
MozReview Request: Bug 1067769 - Part 12: Use Maybe<OwningAnimationTarget> in KeyframeEffect(ReadOnly) constructors. r=birtles

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

Comment 139

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2208569113ee

Comment 140

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59038bfbc218

Comment 141

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8110c10c4079

Comment 142

[Boris Chiou [:boris]]

Try server said: multiple definition of 'mozilla::ImplCycleCollectionUnlink' and 'mozilla::ImplCycleCollectionTraverse"

I think I have to add _inline_ in these functions (in part 7). I will update them and rebase these patch.

Comment 143

[Boris Chiou [:boris]]

Try server said: multiple definition of 'mozilla::ImplCycleCollectionUnlink' and 'mozilla::ImplCycleCollectionTraverse" in many places. I think I have to add _inline_ in these functions (in part 7). I will update them and rebase these patches.

Comment 150

[Boris Chiou [:boris]]

Comment on attachment 8745283 [details]
MozReview Request: Bug 1067769 - Part 7: Define OwningAnimationTarget and use it. r=birtles

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

Comment 170

[Boris Chiou [:boris]]

Sorry for these spams. I have to rebase these patch again tomorrow because there are some conflicts in mozilla-inbound.

Comment 186

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cfa5af78077
https://hg.mozilla.org/integration/mozilla-inbound/rev/558d977d7cc8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f8e27fc1e18
https://hg.mozilla.org/integration/mozilla-inbound/rev/8628768ddf1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78c3f967032
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ce9cec5247
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f88a50a4ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/cff7ac3b0307
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e126b275f8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70f145ab9b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6383fb78ede9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3cd399cf46
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fbe8d41149
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e8827792d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad471499e704

Comment 187

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

https://hg.mozilla.org/mozilla-central/rev/8cfa5af78077
https://hg.mozilla.org/mozilla-central/rev/558d977d7cc8
https://hg.mozilla.org/mozilla-central/rev/9f8e27fc1e18
https://hg.mozilla.org/mozilla-central/rev/8628768ddf1f
https://hg.mozilla.org/mozilla-central/rev/c78c3f967032
https://hg.mozilla.org/mozilla-central/rev/50ce9cec5247
https://hg.mozilla.org/mozilla-central/rev/2f88a50a4ece
https://hg.mozilla.org/mozilla-central/rev/cff7ac3b0307
https://hg.mozilla.org/mozilla-central/rev/7e126b275f8a
https://hg.mozilla.org/mozilla-central/rev/f70f145ab9b8
https://hg.mozilla.org/mozilla-central/rev/6383fb78ede9
https://hg.mozilla.org/mozilla-central/rev/1c3cd399cf46
https://hg.mozilla.org/mozilla-central/rev/89fbe8d41149
https://hg.mozilla.org/mozilla-central/rev/d6e8827792d4
https://hg.mozilla.org/mozilla-central/rev/ad471499e704

Comment 188

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en-US/docs/Web/API/KeyframeEffectReadOnly/target

Submitted PR to update BCD: https://github.com/mdn/browser-compat-data/pull/2441

And updated Firefox 49 for developers.

Comment 189

[Brian Birtles (:birtles)]

This interface is still preffed off by default. I hope to ship in 63.