1211783 - add the KeyframeEffect interface

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Ryo Motozawa [:ryo]]

I'm going to take this, and use this to add empty KeyframeEffect interface.
All of attributes of the interface will be implemented in bug 1244586 1067769 etc.

Comment 2

[Ryo Motozawa [:ryo]]

Attachment: add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl

Comment 3

[Brian Birtles (:birtles)]

Comment on attachment 8714224 [details] [diff] [review]
add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl

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

This looks mostly good to me but I'd like to have another check with the following changes made.

::: dom/animation/KeyframeEffect.h
@@ +394,5 @@
> +  KeyframeEffect(nsIDocument* aDocument,
> +                 Element* aTarget,
> +                 nsCSSPseudoElements::Type aPseudoType,
> +                 const TimingParams& aTiming)
> +  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)

This line should be indented.

@@ +396,5 @@
> +                 nsCSSPseudoElements::Type aPseudoType,
> +                 const TimingParams& aTiming)
> +  : KeyframeEffectReadOnly(aDocument, aTarget, aPseudoType, aTiming)
> +  {
> +  }

Nit: Probably put these braces on the one line and add a line between the constructor and the NS_DECL_* section.

@@ +401,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(KeyframeEffect,
> +                                                         KeyframeEffectReadOnly)
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;

The current coding style says we should only use one of virtual or override[1] so we should drop 'virtual' here.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

@@ +403,5 @@
> +                                                         KeyframeEffectReadOnly)
> +  virtual JSObject* WrapObject(JSContext* aCx,
> +                               JS::Handle<JSObject*> aGivenProto) override;
> +private:
> +  ~KeyframeEffect() = default;

We should drop this (and if we *do* need to add a destructor we should mark it virtual).

::: dom/webidl/KeyframeEffect.webidl
@@ +45,5 @@
> +
> +// Bug 1211783 Implement KeyframeEffect constructor
> + // [Constructor (Animatable? target,
> +  //                object? frames,
> +  //                optional (unrestricted double or KeyframeEffectOptions) options)]

Spacing here is off. Drop the leading whitespace and line up the parameters.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8714224 [details] [diff] [review]
add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl

r+ adding the .webidl interface, but IMO this shouldn't land before the interface has at least some functionality.
And what brian said about spacing.

Comment 5

[Ryo Motozawa [:ryo]]

Attachment: Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v2

I made fixes in dom/webidl/KeyframeEffect.webidl and dom/animation/KeyframeEffect.h. Brian, should I implement an attribute in KeyframeEffect before landing this patch? What do you think?

Comment 6

[Brian Birtles (:birtles)]

Comment on attachment 8714609 [details] [diff] [review]
Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v2

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

::: dom/animation/KeyframeEffect.h
@@ +399,5 @@
> +
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(
> +    KeyframeEffect,
> +    KeyframeEffectReadOnly)

What do we need these lines for? And where is the corresponding implementation?

Comment 7

[Brian Birtles (:birtles)]

(In reply to Ryo Motozawa [:ryo] from comment #5)
> Created attachment 8714609 [details] [diff] [review]
> Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v2
> 
> I made fixes in dom/webidl/KeyframeEffect.webidl and
> dom/animation/KeyframeEffect.h. Brian, should I implement an attribute in
> KeyframeEffect before landing this patch? What do you think?

I think we should implement the KeyframeEffect constructor, add tests for it, and land it at the same time as this patch.

When we implement the KeyframeEffect constructor, in a separate patch we should also use it in Element::Animate and update the tests.

Comment 8

[Ryo Motozawa [:ryo]]

Attachment: Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v3

Removed macro declarations. I forgot to remove these declarations while editing.
At first I wrote corresponding macro declarations in KeyframeEffect.cpp, but I realized they are not need.

I agree with you. I will implement KeyframeEffect constructor.

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8714653 [details] [diff] [review]
Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v3

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

Thank you!

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Hello sheriffs,
Could you please land patches in this bug, bug 1226047 and bug 1244586 in a push at once?
Please be careful these dependencies.
Thank you,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=692bd370ea37

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9109546d708

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9109546d708