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

It would be useful to be able to trace back from an Animation to the @keyframes rule used to generate it. This would be particularly useful for the DevTools team.

In Web Animations, the Animation interface has an 'effect' member of type AnimationEffect. One subclass of this is KeyframeEffect.

In discussion with Google, we've agreed to add a 'name' DOMString attribute to AnimationEffect for this. I've yet to add this to the spec, however.

(Google also want a 'name' attribute on Animation which the author can set to whatever they like. The point of standardizing it would be that then developer tools can use this as an informational aid. I'm not totally convinced about this, but anyway.)

For now I think it would be enough to add a [Cached] 'effect' property to Animation that generates a KeyframeEffect with a single readonly attribute 'name' of type DOMString. This KeyframeEffect object, when generated could have a pointer back to its Animation and read its mName.

Currently the mName lives on AnimationPlayer, however. It is used when we regenerate animations to match old players with new players. It probably should stay on the AnimationPlayer for this purpose so perhaps we need to copy it to the Animation? (Since Animations don't have a pointer to their AnimationPlayer by design.)

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

This patch stores the animation name on the Animation object rather than its
AnimationPlayer. This is because Animation objects don't have a reference to
their AnimationPlayer but their AnimationEffect needs access to the animation
name.

This patch also adds an accessor for AnimationPlayer to get the name from its
Animation (since players *do* have a reference to their source animation
content).

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

These patches build on top of the patch from bug 1045994.

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

https://tbpl.mozilla.org/?tree=Try&rev=fdc34544b9b9

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

Notes for review:

Purpose: Expose the animation-name value used to generate animations specified by CSS animations.

It is expected this will be:
* used by DevTools as an informational aid so authors can match up the Animation(Player) objects with their source
* used by Web content and DevTools as they iterate over the values returned by getAnimationPlayers() to find a particular animation

However, I'm not entirely confident this is the "right" place to expose the animation-name.

Web Animations has the following hierarchy:

  AnimationPlayer(0..1)-->(0..1)Animation(0..1)-->(0..1)AnimationEffect

  AnimationEffect is an abstract interface with sub-interfaces KeyframeEffect and MotionPathEffect.

  (There has been some talk about making AnimationEffects shareable through some global effect repository but I don't know where that proposal is up to.)

So where should the animation name be exposed?

(a) AnimationPlayer -- when we update animations internally we match old and new AnimationPlayer objects based on their name so the name needs to be accessible from the AnimationPlayer. Furthermore, if it becomes possible to change the name associated with an Animation or AnimationEffect (either by making the name attribute writeable or by allowing these objects to be replaced) then we'll probably need a copy of the original name on the AnimationPlayer for matching purposes. Putting the name on the AnimationPlayer also probably simplifies the use-cases above.

(b) Animation -- animation-name is a key to a @keyframes rule so the key could possibly be stored in the Animation? (I'm a little concerned that doing so would create the expectation that changing it makes us replace the AnimationEffect by doing a lookup of @keyframes rules based on the new name).

(c) AnimationEffect -- the 'name' labels the @keyframes rule and @keyframes rules map most closely to KeyframeEffect objects (with the slight exception that there may be several unique KeyframeEffect objects corresponding to a single @keyframes rule due to the way animation-timing-function is inherited).

I've gone with (c) since I think it is the most natural place for this property to live. This was the agreement reached with Google.[1] However, I realise it makes using the property more awkward. Specifically you have to write player.source.effect.name.

I haven't added this property to the spec yet because:
(a) Google still have some outstanding actions around this such as determining how it gets specified by the AnimationEffect / Animation constructor and how it gets used for SVG.[1]
(b) Google also want to add a |name| property to the Animation interface that is purely for authors to have a consistent property for labelling their content so that it can be displayed by their DevTools. I'm not entirely sure this is a good idea yet (also because there's no obvious place to specify this in the constructor to an Animation).

All that is to say, "feel free to question whether these patches are doing the right thing at all."

[1] http://lists.w3.org/Archives/Public/public-fx/2014JulSep/0006.html

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 8 • 10 years ago

Comment on attachment 8474984 [details] [diff] [review]
part 1 - Add Animation.effect

AnimationEffect may as well have a private destructor given that
release is the only correct way to destroy it.

Also, I don't think AnimationEffect needs a virtual destructor -- though
you should then make the class MOZ_FINAL.

I don't feel particularly qualified to comment on what the API should
be, but r=dbaron anyway.

A DOM peer needs to review this as well, though.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 9 • 10 years ago

Comment on attachment 8474986 [details] [diff] [review]
part 2 - Move the animation name from AnimationPlayer to Animation

Please prefer the name "nsSubstring" over "nsAString".  
(nsAnimationManager.h, Animation.h, AnimationPlayer.h)

AnimationPlayer::Name() should just return const nsString&, though,
since it is.

Should dom::Animation really have a constructor that doesn't take
a name?  Is that still needed?  It seems preferable not to increase
the number of constructors to maintain, and also not to leave some 
callers missing an argument they should be passing.

Shouldn't you be removing mName from dom::AnimationPlayer?

r=dbaron with that

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 10 • 10 years ago

Comment on attachment 8474987 [details] [diff] [review]
part 3 - Add AnimationEffect.name

It would be good to get this in a spec sooner rather than later if you've agreed on it.

r=dbaron, assuming it gets into a spec sooish

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

Address review feedback.

Boris, can you please check the WebIDL parts of this? Thanks.

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #9)
> Comment on attachment 8474986 [details] [diff] [review]
> part 2 - Move the animation name from AnimationPlayer to Animation
> 
> Please prefer the name "nsSubstring" over "nsAString".  
> (nsAnimationManager.h, Animation.h, AnimationPlayer.h)

Fixed. I wasn't sure about that so thanks for pointing it out.

> AnimationPlayer::Name() should just return const nsString&, though,
> since it is.

Fixed.

> Should dom::Animation really have a constructor that doesn't take
> a name?  Is that still needed?  It seems preferable not to increase
> the number of constructors to maintain, and also not to leave some 
> callers missing an argument they should be passing.

Fixed.

> Shouldn't you be removing mName from dom::AnimationPlayer?

Yes, sorry, that's sloppy work on my part. I've been trying to get too many things done this month.

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

Rebase

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #10)
> Comment on attachment 8474987 [details] [diff] [review]
> part 3 - Add AnimationEffect.name
> 
> It would be good to get this in a spec sooner rather than later if you've
> agreed on it.

Added:
https://github.com/web-animations/web-animations-spec/commit/3c57d6e81e2b59315a1609a2cdd82ce3a6ab37ca

(This is the bikeshed/github version of the spec which we're switching to as soon as the Google guys get around to doing it.)

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=c1981a0c7d33

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

Comment on attachment 8476560 [details] [diff] [review]
part 3 - Add AnimationEffect.name

Hi Boris, can I ask you to review the WebIDL parts of this too.

(I seem to be asking you for a lot of these kind of reviews. Is there someone else you prefer I send them too?)

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

Comment on attachment 8476557 [details] [diff] [review]
part 1 - Add AnimationEffect interface and Animation.effect member

Why not just make the method non-const instead of doing the weird casting?

Will anyone use this API from C++?  The behavior difference wrt JS (always returning the same thing in JS, vs a new object each time in C++) might be worth a comment in the header at least.

r=me

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

Comment on attachment 8476560 [details] [diff] [review]
part 3 - Add AnimationEffect.name

> Is there someone else you prefer I send them too?

I'm ok doing them; they're pretty quick.  Other options if you want quick turnaround are basically "smaug".  If you're ok with slower turnaround, bholley or khuey or peterv or jst or mrbkap are viable options.  The full list of whitelisted IDL reviewers as of today is at http://hg.mozilla.org/hgcustom/hghooks/file/1da6c5dff8e2/mozhghooks/prevent_webidl_changes.py#l36

r=me

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

(In reply to Boris Zbarsky [:bz] from comment #17)
> Comment on attachment 8476557 [details] [diff] [review]
> part 1 - Add AnimationEffect interface and Animation.effect member
> 
> Why not just make the method non-const instead of doing the weird casting?

I didn't realise the bindings let you do that. I thought the method signature generated by 'mach webidl-example' was final.
 
> Will anyone use this API from C++?  The behavior difference wrt JS (always
> returning the same thing in JS, vs a new object each time in C++) might be
> worth a comment in the header at least.

No-one will use it from C++ yet. Later on they will (when we move keyframes into that object) but at that point we'll also generate the object at the outset and manage it on the Animation, not via [Cached]. I'll add a comment.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29eb95ad8ee2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9140cc470a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/81bfd63430a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2312266dfc01

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

> I thought the method signature generated by 'mach webidl-example' was final.

Ah.  One of the major points of the bindings was that the callee can choose stuff about their function signature like constness, ABI, etc.  The caller is just C++ calling into the callee, so as long as the name matches and the arguments end up compatible and the return value ends up compatible everything is fine.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/29eb95ad8ee2
https://hg.mozilla.org/mozilla-central/rev/f9140cc470a2
https://hg.mozilla.org/mozilla-central/rev/81bfd63430a4
https://hg.mozilla.org/mozilla-central/rev/2312266dfc01