Closed Bug 1211783 Opened 6 years ago Closed 5 years ago

add the KeyframeEffect interface

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- affected
firefox47 --- fixed

People

(Reporter: heycam, Assigned: motozawa)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Depends on: 1215406
Blocks: web-animations
No longer blocks: 1198705
Blocks: 1244586
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.
Assignee: nobody → motozawa
Blocks: 1244591
Blocks: 1244590
Attachment #8714224 - Flags: review?(bugs)
Attachment #8714224 - Flags: review?(bbirtles)
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.
Attachment #8714224 - Flags: review?(bbirtles)
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.
Attachment #8714224 - Flags: review?(bugs) → review+
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?
Attachment #8714609 - Flags: review?(bbirtles)
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.
Attachment #8714653 - Flags: review?(bbirtles)
Attachment #8714653 - Attachment is patch: true
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!
Attachment #8714653 - Flags: review?(bbirtles) → review+
Attachment #8714609 - Flags: review?(bbirtles)
Attachment #8714224 - Attachment is obsolete: true
Attachment #8714609 - Attachment is obsolete: true
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d9109546d708
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1249277
Depends on: 1249278
No longer depends on: 1249277
You need to log in before you can comment on or make changes to this bug.