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

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 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?

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

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 on attachment 8714653 [details] [diff] [review] Add interface KeyframeEffect in dom/webidl/KeyframeEffect.webidl. v3 Review of attachment 8714653 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

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