David Zbarsky (:dzbarsky) Reporter
	Description • 10 years ago

      No description provided.

	Boris Chiou [:boris] Assignee
	Comment 1 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 5 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 6 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 7 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 8 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 9 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 10 • 8 years ago

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+

	Boris Chiou [:boris] Assignee
	Comment 11 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 12 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 13 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 14 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 18 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 19 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 20 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 21 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 22 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 23 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 27 • 8 years ago

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="boris.chiou@gmail.com">

We're not adding author lines anymore.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 28 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 29 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 30 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 31 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 34 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 37 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 38 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 39 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 41 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 42 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 43 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 44 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 46 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 47 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 48 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 49 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 50 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 54 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 56 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 62 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 63 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 64 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 65 • 8 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 66 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 67 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 68 • 8 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 69 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 70 • 8 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 71 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 72 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 73 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 74 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 75 • 8 years ago

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

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

	Boris Chiou [:boris] Assignee
	Comment 76 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 77 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 78 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 79 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 81 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 87 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 88 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 90 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 92 • 8 years ago

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/

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 101 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 102 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 103 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 104 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 105 • 8 years ago

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.

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 106 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 107 • 8 years ago

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?

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 108 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 109 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 110 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 111 • 8 years ago

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.

	Hiroyuki Ikezoe (:hiro)
	Comment 112 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 113 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 114 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 115 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 116 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 117 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 118 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 119 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 120 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 131 • 8 years ago

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

	Boris Chiou [:boris] Assignee
	Comment 132 • 8 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave)
	Comment 133 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 134 • 8 years ago

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

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

	Boris Chiou [:boris] Assignee
	Comment 135 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 143 • 8 years ago

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.

	Boris Chiou [:boris] Assignee
	Comment 150 • 8 years ago

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/

	Boris Chiou [:boris] Assignee
	Comment 170 • 8 years ago

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

	Pulsebot
	Comment 186 • 8 years ago

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