Last Comment Bug 1226047 - Implement AnimationEffectTiming interface
: Implement AnimationEffectTiming interface
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Animation (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla47
Assigned To: Ryo Motozawa [:ryo]
:
: Brian Birtles (:birtles)
Mentors:
Depends on: 1214536
Blocks: web-animations 1244586 1244633 1244635
  Show dependency treegraph
 
Reported: 2015-11-18 16:37 PST by Brian Birtles (:birtles)
Modified: 2016-03-28 19:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
fixed


Attachments
addAnimationEffectTiminginterface (7.31 KB, patch)
2016-02-01 02:16 PST, Ryo Motozawa [:ryo]
no flags Details | Diff | Splinter Review
addAnimationEffectTiminginterface v2 (7.12 KB, patch)
2016-02-02 18:53 PST, Ryo Motozawa [:ryo]
bbirtles: review+
bugs: review+
Details | Diff | Splinter Review
addAnimationEffectTiminginterface v3 (7.06 KB, patch)
2016-02-04 00:16 PST, Ryo Motozawa [:ryo]
motozawa: review+
Details | Diff | Splinter Review
addAnimationEffectTiminginterface v4 (7.07 KB, patch)
2016-02-11 18:47 PST, Ryo Motozawa [:ryo]
motoryo1: review+
Details | Diff | Splinter Review

Description User image Brian Birtles (:birtles) 2015-11-18 16:37:01 PST
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 User image Brian Birtles (:birtles) 2015-11-18 16:39:49 PST
(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 User image Ryo Motozawa [:ryo] 2016-02-01 02:16:17 PST
Created attachment 8714261 [details] [diff] [review]
addAnimationEffectTiminginterface

This patch does not contain attributes.
I am going to implement attributes later in other bugs.
Comment 3 User image Brian Birtles (:birtles) 2016-02-01 14:54:18 PST
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 User image Ryo Motozawa [:ryo] 2016-02-02 18:53:41 PST
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.
Comment 5 User image Brian Birtles (:birtles) 2016-02-02 20:03:25 PST
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-02-03 08:23:05 PST
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-02-03 08:23:54 PST
Do we support inherit in webidl already?
Comment 8 User image Ryo Motozawa [:ryo] 2016-02-04 00:16:48 PST
Created attachment 8715709 [details] [diff] [review]
addAnimationEffectTiminginterface v3

I removed a include header and an unnecessary line.
Comment 9 User image Ryo Motozawa [:ryo] 2016-02-11 18:47:12 PST
Created attachment 8718686 [details] [diff] [review]
addAnimationEffectTiminginterface v4

Modified patch for newest tree.
Comment 10 User image Hiroyuki Ikezoe (:hiro) 2016-02-11 22:04:24 PST
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 User image Hiroyuki Ikezoe (:hiro) 2016-02-12 13:15:26 PST
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 13 User image Carsten Book [:Tomcat] 2016-02-15 03:20:28 PST
https://hg.mozilla.org/mozilla-central/rev/7ea5d090fd51

Note You need to log in before you can comment on or make changes to this bug.