Brian Birtles (:birtles) Assignee
	Description • 10 years ago

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)

	Brian Birtles (:birtles) Assignee
	Comment 1 • 9 years ago

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

	Brian Birtles (:birtles) Assignee
	Comment 2 • 9 years ago

Updated patch

	Sebastian Zartner [:sebo]
	Comment 3 • 9 years ago

> Severity: enhancement → normal

Brian, isn't the implementation of a new feature an enhancement?

Sebastian

	Brian Birtles (:birtles) Assignee
	Comment 4 • 9 years ago

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

	Brian Birtles (:birtles) Assignee
	Comment 5 • 9 years ago

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

	Cameron McCormack (:heycam)
	Comment 6 • 9 years ago

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.

	Brian Birtles (:birtles) Assignee
	Comment 8 • 9 years ago

This is to line up with the spec as discussed in:

  https://bugzilla.mozilla.org/show_bug.cgi?id=1096773#c6

	Brian Birtles (:birtles) Assignee
	Comment 9 • 9 years ago

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

	Brian Birtles (:birtles) Assignee
	Comment 12 • 9 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 9 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 9 years ago

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.

	Boris Chiou [:boris]
	Comment 16 • 9 years ago

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.

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

Comment on attachment 8712406 [details] [diff] [review]
part 4 - Add tests for Animatable.animate()

r=me, but do add those Xray tests.

	Brian Birtles (:birtles) Assignee
	Comment 18 • 9 years ago

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.

	Brian Birtles (:birtles) Assignee
	Comment 19 • 9 years ago

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

	Pulsebot
	Comment 20 • 9 years ago

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

	Brian Birtles (:birtles) Assignee
	Comment 21 • 9 years ago

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.

	Carsten Book [:Tomcat]
	Comment 22 • 9 years ago
bugherder

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

	Hiroyuki Ikezoe (:hiro)
	Comment 23 • 9 years ago

Great! Thank you, Brian!

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 9 years ago

> No, Boris Chiou has been working in this area recently

Ah, awesome.  Was just making sure that name similarity did not confuse things.  :)