Closed Bug 1096773 Opened 5 years ago Closed 4 years ago

Implement Animatable.animate()

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 3 obsolete files)

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)
Depends on: 1096774
Component: DOM → DOM: Animation
Depends on: 1151731
Depends on: 1198705
Summary: Implement ability to create Web animations from script → Implement Animatable.animate()
Blocks: 1228915
Depends on: 1237467
Attached patch Implement Animatable.animate() (obsolete) — Splinter Review
WIP patch. The conversion between options objects can probably be improved still.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Severity: normal → enhancement
Keywords: dev-doc-needed
Attached patch Implement Animatable.animate() (obsolete) — Splinter Review
Updated patch
Attachment #8709857 - Attachment is obsolete: true
Severity: enhancement → normal
Keywords: meta
> 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
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
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()
Attachment #8712403 - Flags: review?(boris.chiou)
Attachment #8712405 - Flags: review?(bzbarsky)
Attachment #8710298 - Attachment is obsolete: true
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.
Attachment #8712402 - Attachment description: part 1 - Make the frames argument to the KeyframeEffectReadOnly constructor optional → part 1 - Make the frames argument to the KeyframeEffectReadOnly constructor no longer optional
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
Attachment #8712402 - Flags: review?(bzbarsky) → review+
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?
Flags: needinfo?(bbirtles)
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.
Attachment #8712405 - Flags: review?(bzbarsky) → review+
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.
Attachment #8712403 - Flags: review?(boris.chiou) → review+
Comment on attachment 8712406 [details] [diff] [review]
part 4 - Add tests for Animatable.animate()

r=me, but do add those Xray tests.
Attachment #8712406 - Flags: review?(bzbarsky) → review+
Blocks: 1241784
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.
Attachment #8713134 - Flags: review?(bzbarsky)
(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!
Flags: needinfo?(bbirtles)
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.
Attachment #8713134 - Attachment is obsolete: true
Attachment #8713134 - Flags: review?(bzbarsky)
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.  :)
Blocks: 1245000
You need to log in before you can comment on or make changes to this bug.