1226047 - Implement AnimationEffectTiming interface

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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
   });

Comment 2

[Ryo Motozawa [:ryo]]

Attachment: addAnimationEffectTiminginterface

This patch does not contain attributes.
I am going to implement attributes later in other bugs.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 4

[Ryo Motozawa [:ryo]]

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

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 6

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

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

Comment 7

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

Do we support inherit in webidl already?

Comment 8

[Ryo Motozawa [:ryo]]

Attachment: addAnimationEffectTiminginterface v3

I removed a include header and an unnecessary line.

Comment 9

[Ryo Motozawa [:ryo]]

Attachment: addAnimationEffectTiminginterface v4

Modified patch for newest tree.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea5d090fd51

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ea5d090fd51