Implement AnimationEffectTiming interface

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Animation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: birtles, Assigned: ryo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox47 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
This builds on bug 1214536 to make these timing properties writeable.

When we do this, we need to add a test for the following:

  elem.style.animation = 'anim 100s 2';
  var anim = elem.getAnimations()[0];
  anim.finish();
  anim.ready.then(() => {
    anim.effect.timing.iterations = 1.5;
    // Assert that the computedStyle now reflects the value half-way
    // through the interval
  });

This is to make sure the optimization from bug 1219236 doesn't cause us to skip updating while we are in the finished state.
(Reporter)

Comment 1

a year ago
(In reply to Brian Birtles (:birtles) from comment #0)
>   elem.style.animation = 'anim 100s 2';
>   var anim = elem.getAnimations()[0];
>   anim.finish();
>   anim.ready.then(() => {
>     anim.effect.timing.iterations = 1.5;
>     // Assert that the computedStyle now reflects the value half-way
>     // through the interval
>   });

Actually, that's wrong, unless we decide to make CSSAnimation objects have mutable KeyframeEffects after all (something I want to bring up on the list soon). This should, instead, be something like:

   var anim = elem.animate({ ... }, { duration: 100, iterations: 2});
   anim.finish();
   anim.ready.then(() => {
     anim.effect.timing.iterations = 1.5;
     // Assert that the computedStyle now reflects the value half-way
     // through the interval
   });

Updated

a year ago
Blocks: 1244633

Updated

a year ago
Blocks: 1244635
Created attachment 8714261 [details] [diff] [review]
addAnimationEffectTiminginterface

This patch does not contain attributes.
I am going to implement attributes later in other bugs.
Attachment #8714261 - Flags: review?(bugs)
Attachment #8714261 - Flags: review?(bbirtles)
(Reporter)

Comment 3

a year ago
Comment on attachment 8714261 [details] [diff] [review]
addAnimationEffectTiminginterface

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

::: dom/animation/AnimationEffectTiming.h
@@ +15,5 @@
> +
> +class AnimationEffectTiming : public AnimationEffectTimingReadOnly
> +{
> +public:
> +  AnimationEffectTiming() = default;

Why is this needed?

@@ +17,5 @@
> +{
> +public:
> +  AnimationEffectTiming() = default;
> +
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(AnimationEffectTiming)

We don't need this right? AnimationEffectTimingReadOnly implements ref-counting for us.

@@ +18,5 @@
> +public:
> +  AnimationEffectTiming() = default;
> +
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(AnimationEffectTiming)
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(AnimationEffectTiming)

I don't think we need this, yet, either. (And when we do we should use NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(AnimationEffectTiming, AnimationEffectTimingReadOnly).)

@@ +21,5 @@
> +  NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING(AnimationEffectTiming)
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS(AnimationEffectTiming)
> +
> +private:
> +  ~AnimationEffectTiming() = default;

Why is this needed?

::: dom/webidl/AnimationEffectTiming.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://w3c.github.io/web-animations/#animationeffecttiming

Please use the https URL here.

@@ +10,5 @@
> + * liability, trademark and document use rules apply.
> + */
> +
> +[Func="nsDocument::IsWebAnimationsEnabled"]
> +//Bug 1226047 - Implement AnimationEffectTiming interface

No need for this comment. We only add bug references for the missing/faulty functionality.
Attachment #8714261 - Flags: review?(bugs)
Attachment #8714261 - Flags: review?(bbirtles)

Updated

a year ago
Blocks: 1244586
Created attachment 8715125 [details] [diff] [review]
addAnimationEffectTiminginterface v2

Removed macro declarations and fixed constructor declaration in dom/animation/AnimationEffectTiming.h.

Changed to use https URL and remove unnecessary comment in dom/webidl/AnimationEffectTiming.webidl.
Attachment #8714261 - Attachment is obsolete: true
Attachment #8715125 - Flags: review?(bugs)
Attachment #8715125 - Flags: review?(bbirtles)
(Reporter)

Comment 5

a year ago
Comment on attachment 8715125 [details] [diff] [review]
addAnimationEffectTiminginterface v2

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

r=birtles with comments addressed

::: dom/animation/AnimationEffectTiming.cpp
@@ +7,5 @@
> +#include "mozilla/dom/AnimationEffectTiming.h"
> +
> +#include "mozilla/dom/AnimatableBinding.h"
> +#include "mozilla/dom/AnimationEffectTimingBinding.h"
> +#include "mozilla/dom/KeyframeEffectBinding.h"

Are AnimatableBinding.h and KeyframeEffectBinding.h needed?

::: dom/animation/AnimationEffectTiming.h
@@ +8,5 @@
> +#define mozilla_dom_AnimationEffectTiming_h
> +
> +#include "mozilla/dom/AnimationEffectTimingReadOnly.h"
> +
> +

Nit: Probably don't need two lines of whitespace here.

@@ +24,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif // mozilla_dom_AnimationEffectTimingReadOnly_h

This should be mozilla_dom_AnimationEffectTiming_h
Attachment #8715125 - Flags: review?(bbirtles) → review+
Comment on attachment 8715125 [details] [diff] [review]
addAnimationEffectTiminginterface v2

r+ for the .webidl, but probably better to land this when there is at least some functionality exposed via AnimationEffectTiming
Attachment #8715125 - Flags: review?(bugs) → review+
Do we support inherit in webidl already?
Created attachment 8715709 [details] [diff] [review]
addAnimationEffectTiminginterface v3

I removed a include header and an unnecessary line.
Attachment #8715125 - Attachment is obsolete: true
Attachment #8715709 - Flags: review+
(Assignee)

Comment 9

a year ago
Created attachment 8718686 [details] [diff] [review]
addAnimationEffectTiminginterface v4

Modified patch for newest tree.
Attachment #8718686 - Flags: review+
Attachment #8715709 - Attachment is obsolete: true
We are going to land this interface even though it's empty because this interface is necessary for KeyframeEffect constructor (bug 1244586) and implementations of each attributes of this interface would take more time, and most of these functionalities have been covered by AnimationEffectTimingReadOnly.
Thanks for caring.
Hello sheriffs,
Could you please land patches in bug1211783, this bug 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

Comment 12

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea5d090fd51
Keywords: checkin-needed

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ea5d090fd51
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Reporter)

Updated

a year ago
Assignee: nobody → motoryo1
You need to log in before you can comment on or make changes to this bug.