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

      No description provided.

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

Comment on attachment 8489753 [details] [diff] [review]
Patch

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

r=birtles with feedback addressed

::: dom/animation/Animation.h
@@ +164,5 @@
> +  }
> +
> +  void SetTarget(Element* aTarget) {
> +    mTarget = aTarget;
> +  }

I'd rather we left this out and tackled it in a separate bug. The steps involved in correctly switching targets are pretty complicated. For a start, we need to make sure the animation effect gets cleared from the old target. Then we also need to do all these steps:

  http://w3c.github.io/web-animations/#setting-the-source-content

Just make the IDL readonly and file a bug for making it writeable.

::: dom/animation/test/css-integration/test_element-get-animation-players.html
@@ +41,5 @@
>    var players = div.getAnimationPlayers();
>    assert_equals(players.length, 1,
>      'getAnimationPlayers returns a player running CSS Animations');
> +  assert_equals(players[0].source.target, div,
> +    'Animation.target is the animatable div');

We try to keep these tests as targeted as possible or at least that's the feedback I got when I submitted some tests to web-platform-tests. So I'd prefer if we just factored out another test method just to check this.

::: dom/webidl/Animation.webidl
@@ +15,5 @@
>    // FIXME: |effect| should have type (AnimationEffect or EffectCallback)?
>    // but we haven't implemented EffectCallback yet.
> +  [Cached,Pure]
> +  readonly attribute AnimationEffect? effect;
> +           attribute Element?         target;

As mentioned, make target readonly and add a FIXME/bug reference.

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

Hi David, I went ahead and applied my review feedback to your patch because I'm building a lot of stuff that depends on this patch.

If this looks good to you, feel free to transfer my r+ and land it.

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

Comment on attachment 8497950 [details] [diff] [review]
Updated patch

Boris, would you be able to review the WebIDL parts of this patch?

(It's David's patch, but I incorporated my own review feedback since I want to land it soon. I'll let David check those changes, but I'd like to get your review of this version of the patch since the WebIDL parts of this patch are slightly different to the one I reviewed.)

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

Comment on attachment 8497950 [details] [diff] [review]
Updated patch

r=me, but the pseudotype version of GetTarget doesn't seem relevant to this patch.

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

(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8497950 [details] [diff] [review]
> Updated patch
> 
> r=me, but the pseudotype version of GetTarget doesn't seem relevant to this
> patch.

Removed. I'll add it in a later bug where it's needed.

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

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

Got the initialization order wrong in the previous patch (and MSVC doesn't care about this).

Running this through try with some other changesets that will fail a few newly added tests:

  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f9fb59908912

	David Zbarsky (:dzbarsky) Assignee
	Comment 8 • 10 years ago

Comment on attachment 8497999 [details] [diff] [review]
Updated patch

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

::: dom/animation/Animation.h
@@ +169,5 @@
> +    // returns animations targetting Element so we should this should never
> +    // be called for an animation that targets a pseudo-element.
> +    MOZ_ASSERT(mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement,
> +               "Requesting the target of an Animation that targets a"
> +               " pseudo-element is not yet supported.");

So this is OK because we will call GetTarget from Play() (for restyling) but not from PlayInternal, right?

::: dom/webidl/Animation.webidl
@@ +15,5 @@
>    // FIXME: |effect| should have type (AnimationEffect or EffectCallback)?
>    // but we haven't implemented EffectCallback yet.
> +  [Cached,Pure]
> +  readonly attribute AnimationEffect? effect;
> +  // FIXME: This should be writeable

reference bug 1067769?

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/28519d825a23

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/28519d825a23