This includes: * The AnimationPlayer constructor * Element.animate * All the plumbing needed to make that happen (e.g. a new kind of manager like nsAnimationManager/nsTransitionManager or perhaps simply merging and generalizing the two)

WIP patch. The conversion between options objects can probably be improved still.

Updated patch

> Severity: enhancement → normal Brian, isn't the implementation of a new feature an enhancement? Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #3) > > Severity: enhancement → normal > > Brian, isn't the implementation of a new feature an enhancement? Hmm, maybe some groups do that, but I've never seen that before in platform work.

Cameron, I notice that when we implemented the constructor for KeyframeEffectReadOnly we changed the IDL so that the frames argument is optional.[1][2] I guess that's normal for dictionary arguments since authors shouldn't have to type {}? Is that right? Should I just update the spec to use optional here and for the same argument in the Animatable interface?[3] [1] http://w3c.github.io/web-animations/#the-keyframeeffect-interfaces [2] https://dxr.mozilla.org/mozilla-central/rev/66e07ef46853709e3fa91e7c9ad9fe6abf0d5f06/dom/webidl/KeyframeEffect.webidl#39 [3] http://w3c.github.io/web-animations/#animatable

Yes, I made that optional because at that point the spec was defining the frames argument type as a union that contained a dictionary type, and trailing arguments that include dictionary types must be optional (for the reason you give). Since we now have custom handling of the object that is passed in, and we only conceptually treat it like a dictionary (when you don't pass an iterable thing), we don't have to make it optional if we don't want to. Since the object isn't being used like an options object, I'd say that it maybe doesn't make sense to be optional.

This is to line up with the spec as discussed in: https://bugzilla.mozilla.org/show_bug.cgi?id=1096773#c6

This will allow us to re-use the constructor from Animatable.animate() since the existing type, UnrestrictedDoubleOrKeyframeEffectOptions, is not compatible with UnrestrictedDoubleOrKeyframeAnimationOptions (introduced in the next patch in this series), as used by Animatable.animate()

Comment on attachment 8712402 [details] [diff] [review] part 1 - Make the frames argument to the KeyframeEffectReadOnly constructor no longer optional Oops, the patch title is the opposite of what it should be. Will update when landing.

Comment on attachment 8712402 [details] [diff] [review] part 1 - Make the frames argument to the KeyframeEffectReadOnly constructor no longer optional > Bug 1096773 part 1 - Make the frames argument to the KeyframeEffectReadOnly > constructor optional This is missing a "not" or something, right? >+ const JS::Handle<JSObject*>& aFrames, This should be: JS::Handle<JSObject*> aFrames, without the extra indirection of a reference to a handle. >+ if (!aFrames.get()) { Just "if (!aFrames) {" should work. >+ JS::Rooted<JS::Value> objectValue(aCx, JS::ObjectOrNullValue(aFrames)); I'd do: JS::Rooted<JS::Value> objectValue(aCx, JS::ObjectValue(*aFrames)); since you know it's not null. >+ const JS::Handle<JSObject*>& aFrames, Again, please pass JS::Handle by value, not by reference. r=me

Comment on attachment 8712403 [details] [diff] [review] part 2 - Add a KeyframeEffectReadOnly constructor that takes a TimingParams argument Did you mean to request review from me on this one too?

Comment on attachment 8712405 [details] [diff] [review] part 3 - Implement Animatable.animate() >+ const JS::Handle<JSObject*>& aFrames, Pass Handle by value. >+ KeyframeEffectReadOnly::Constructor(global, this, aFrames, >+ TimingParams::FromOptionsUnion( >+ aOptions), aError); The indentation here is a bit weird; I'd almost prefer that we just move over the second line so it's only indented by 2 spaces from the first or something. But more importantly, this doesn't match how a constructor would normally be invoked. Please add tests for calling this over Xrays which might have caught the issue. For a constructor invocation from bindings, the JSContext would normally enter the compartment of the global involved before calling into the actuall C++ Constructor method. That means entering the compartment of ownerGlobal->GetGlobalJSObject() and wrapping aFrames into it. In the Xray case, this means aFrame would end up as an opaque wrapper unless the caller made a point of creating it in the content compartment, but that seems fairly reasonable in this case... It's not quite how dictionaries work, sadly. I guess the other option is that we could explicitly decide that over Xrays Animate should do something smarter with aFrames, but then we need to carefully audit the constructors it calls to make sure they're OK being called in an unexpected compartment, which seems more fragile. >+ animation->Play(aError, Animation::LimitBehavior::AutoRewind); That's a slightly odd API; normally the ErrorResult would be last... but ok. >+ [Func="nsDocument::IsWebAnimationsEnabled",Throws] Comma before "Throws", please. r=me with the above fixed.

Comment on attachment 8712403 [details] [diff] [review] part 2 - Add a KeyframeEffectReadOnly constructor that takes a TimingParams argument Review of attachment 8712403 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding these constructors for TimingParams.

Comment on attachment 8712406 [details] [diff] [review] part 4 - Add tests for Animatable.animate() r=me, but do add those Xray tests.

Hi Boris, do you mind checking these fixes to part 2 and 4 for testing and handling x-rays? I'll merge them into parts 2 and 4 before landing.

(In reply to Boris Zbarsky [:bz] from comment #14) > Comment on attachment 8712403 [details] [diff] [review] > part 2 - Add a KeyframeEffectReadOnly constructor that takes a TimingParams > argument > > Did you mean to request review from me on this one too? No, Boris Chiou has been working in this area recently and since this was just a simple refactoring I thought I'd ask him to review it. Drive-by comments are always welcome however!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b37dd661b3e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/e091852619a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8c6966bae7 https://hg.mozilla.org/integration/mozilla-inbound/rev/f735dd2711d2

Pushed without waiting for re-review on the changes since there is other work depending on these patches and I already have 'r+ with changes' for these patches. If there are any tweaks required to the changes I'll push follow-ups as needed.

https://hg.mozilla.org/mozilla-central/rev/b37dd661b3e0 https://hg.mozilla.org/mozilla-central/rev/e091852619a3 https://hg.mozilla.org/mozilla-central/rev/8d8c6966bae7 https://hg.mozilla.org/mozilla-central/rev/f735dd2711d2

Great! Thank you, Brian!

> No, Boris Chiou has been working in this area recently Ah, awesome. Was just making sure that name similarity did not confuse things. :)