1244641 - implement AnimationEffectTiming duration

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Comment 1

[Ryo Motozawa [:ryo]]

Attachment: Part1 Add duration implementation in dom/animation/AnimationEffectTiming.cpp

I implement a setter for AnimationEffectTiming duration.

Comment 2

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration tests

Tests for AnimationEffectTiming duration.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720160 [details] [diff] [review]
Part1 Add duration implementation in dom/animation/AnimationEffectTiming.cpp

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

Could you please split this patch into smaller patches?

1. letting AnimationEffectTiming having effect
2. SetDuration
3. notifying timing updated

::: dom/animation/AnimationEffectTiming.cpp
@@ +33,5 @@
> +    }
> +  } else {
> +    mTiming.mDuration.SetAsUnrestrictedDouble() = 0;
> +  }
> +

To me, it seems that we can store |aDuration| as is in mTiming.mDuration.  I will check the spec later

@@ +36,5 @@
> +  }
> +
> +  if (mEffect) {
> +    mEffect->NotifySpecifiedTimingUpdated();
> +  }

When is mEffect null?  Once we have the constructor of AnimationEffectTiming?

::: dom/animation/AnimationEffectTiming.h
@@ +25,5 @@
> +
> +  void SetDuration(const UnrestrictedDoubleOrString& aDuration);
> +
> +private:
> +  KeyframeEffect* mEffect;

I am not confident we do not need to hold a reference here.  Don't we?

I will review rest of parts once the smaller patches are uploaded.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720161 [details] [diff] [review]
Part2 Add duration tests

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

Please this split too.
There is a missing file in layout/styles/test/.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

> @@ +36,5 @@
> > +  }
> > +
> > +  if (mEffect) {
> > +    mEffect->NotifySpecifiedTimingUpdated();
> > +  }
> 
> When is mEffect null?  Once we have the constructor of AnimationEffectTiming?
> 
> ::: dom/animation/AnimationEffectTiming.h
> @@ +25,5 @@
> > +
> > +  void SetDuration(const UnrestrictedDoubleOrString& aDuration);
> > +
> > +private:
> > +  KeyframeEffect* mEffect;
> 
> I am not confident we do not need to hold a reference here.  Don't we?

I did miss that KeyframeEffect has a reference of AnimationEffectTiming as OwningNonNull.

In such a situation,

1) Is it possible that mEffect is null in AnimationEffectTiming instance?  I mean, we should do the null check in the instance or not.
2) Does AnimationEffectTiming really need Unlink explicitly?
3) Is it reasonable to returning the KeyframeEffect as a parent object from AnimationEffectTiming::GetParentObject?  Currently AnimationEffectTiming::GetParentObject returns nullptr (Nobody sets mParent there).  I think we should return an object anyway.

I am not familiar with cycle collector, so I would like to hear advises from experts.
Boris?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> 3) Is it reasonable to returning the KeyframeEffect as a parent object from
> AnimationEffectTiming::GetParentObject?  Currently
> AnimationEffectTiming::GetParentObject returns nullptr (Nobody sets mParent
> there).  I think we should return an object anyway.

I meant AnimationEffectTimingReadOnly.  It's a parent class of AnimationEffectTiming.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So just to make sure I understand the situation:

1) KeyframeEffectReadOnly has an mTiming member that is an OwningNonNull<AnimationEffectTimingReadOnly>.  Both of these are IDL interface types, which means that each one can be kept alive from script, right?

2)  Creation of a KeyframeEffectReadOnly passes "this" to the constructor of AnimationEffectTimingReadOnly, which is holding weak reference to it via mEffect.

Is that correct?  In this situation, it's certainly possible for the KeyframeEffectReadOnly to be GCed before the AnimationEffectTimingReadOnly, and in that case we have a dangling reference in mEffect unless we null it out.  So we need to either do the "null out in destructor" thing in the patch, and then mEffect can be null, or make mEffect a strong reference that is cycle-collected.  The latter course of action is a bit annoying because of the current setup of passing "this" in a constructor, at which point calling AddRef is not necessarily safe...

> Is it reasonable to returning the KeyframeEffect as a parent object from
> AnimationEffectTiming::GetParentObject

Seems plausible, since the KeyframeEffect is what creates the AnimationEffectTiming.  As long as KeyframeEffect::GetParentObject returns something reasonable (i.e. not the AnimationEffectTiming), which it seems to.

Does that help?

Comment 8

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> 1) Is it possible that mEffect is null in AnimationEffectTiming instance?  I
> mean, we should do the null check in the instance or not.

Yes, the idea is that KeyframeEffect has an owning reference to its AnimationEffectTiming instance but the timing instance only has a non-owning reference to its effect. If the KeyframeEffect dies, it calls Unlink on its AnimationEffectTiming object, setting its mEffect pointer to nullptr. At this point, the AnimationEffectTiming object acts like an orphaned object.

Script can keep the AnimationEffectTiming object alive and continue to use it but it doesn't need to keep the KeyframeEffect object alive. Unlink sounds like it is involved in cycle-collection but its not. We don't involve the cycle collector here because mEffect is non-owning.

> 2) Does AnimationEffectTiming really need Unlink explicitly?

Yes, see above. The alternative is to re-use cycle collector machinery for this but that seems unnecessary here.

> 3) Is it reasonable to returning the KeyframeEffect as a parent object from
> AnimationEffectTiming::GetParentObject?  Currently
> AnimationEffectTiming::GetParentObject returns nullptr (Nobody sets mParent
> there).  I think we should return an object anyway.

We should pass in the parent object. That seems like an oversight from bug 1214536.

I think we want the other Boris to comment on that.

Comment 9

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

Also, we should fix bug 1237173 next--which, despite the description, was supposed to involve storing the duration as a Maybe<TimeDuration> (see bug 1214536 comment 148).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you, Boris and Brian for the detailed explanations!

To sum it up, AnimationEffectTimingReadOnly should have a raw pointer of KeyframeEffectReadOnly as a parent object, and KeyframeEffectReadOnly should unlink the mTiming in the destructor. Thanks for the help.

Comment 11

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Thank you, Boris and Brian for the detailed explanations!
> 
> To sum it up, AnimationEffectTimingReadOnly should have a raw pointer of
> KeyframeEffectReadOnly as a parent object, and KeyframeEffectReadOnly should
> unlink the mTiming in the destructor. Thanks for the help.

We should probably annotate mEffect with MOZ_NON_OWNING_REF, I guess.

Comment 12

[Ryo Motozawa [:ryo]]

Attachment: Part1 Let AnimationEffectTiming having effect

I splited patch 8720160 to three part.

Comment 13

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp

I splited patch 8720160 to three part.

Comment 14

[Ryo Motozawa [:ryo]]

Attachment: Part3 Added notificater in AnimationEffectTiming::SetDuration

I splited patch 8720160 to three part.

Comment 15

[Ryo Motozawa [:ryo]]

Attachment: Part4 Add duration tests in dom/animation/test/chrome

I splited patch 8720161 to three part.

Comment 16

[Ryo Motozawa [:ryo]]

Attachment: Part5 Add duration tests in layout/style/test

I splited patch 8720160 to three part.

Comment 17

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations

I splited patch 8720161 to three part.

Comment 18

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

Comment on attachment 8720656 [details] [diff] [review]
Part1 Let AnimationEffectTiming having effect

Just a few drive-by comments:

> class AnimationEffectTiming : public AnimationEffectTimingReadOnly
> {
> public:
>-  explicit AnimationEffectTiming(const TimingParams& aTiming)
>-    : AnimationEffectTimingReadOnly(aTiming) { }
>+  explicit AnimationEffectTiming(const TimingParams& aTiming, KeyframeEffect* aEffect)
>+    : AnimationEffectTimingReadOnly(aTiming)
>+    , mEffect(aEffect) { }

You only need explicit when there is only one argument to the constructor.

>+private:
>+  KeyframeEffect* mEffect;

I think we can use:

  KeyframeEffect* MOZ_NON_OWNING_REF mEffect;

(You'll need to #include "mozilla/Attributes.h" I think)

>+protected:
>+  ~KeyframeEffect();
> };

We should make this:

  ~KeyframeEffect() override;

If we drop the 'virtual' keyword on the superclass we want to know about it. Non-virtual destructors can cause really nasty bugs.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720656 [details] [diff] [review]
Part1 Let AnimationEffectTiming having effect

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

::: dom/animation/AnimationEffectTiming.h
@@ +12,5 @@
>  namespace dom {
>  class AnimationEffectTiming : public AnimationEffectTimingReadOnly
>  {
>  public:
> +  explicit AnimationEffectTiming(const TimingParams& aTiming, KeyframeEffect* aEffect)

You can remove 'explicit' here since the number of the argutmens is not one any more.  And I'd prefer KeyframeEffect* is the first argument.

I think AnimationEffectTimingReadOnly (not AnimationEffectTiming) can have the KeyframeEffect and GetParent can return it.  Are there any reason we can not?

::: dom/animation/KeyframeEffect.cpp
@@ +17,5 @@
>  #include "nsCSSParser.h"
>  #include "nsCSSPropertySet.h"
>  #include "nsCSSProps.h" // For nsCSSProps::PropHasFlags
>  #include "nsCSSValue.h"
> +#include "nsDOMMutationObserver.h" // For nsAutoAnimationMutationBatch

This should be moved into a subsequent patch.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720658 [details] [diff] [review]
Part3 Added notificater in AnimationEffectTiming::SetDuration

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

I need Brian's eyes.

::: dom/animation/AnimationEffectTiming.cpp
@@ +36,5 @@
>    }
> +
> +  if (mEffect) {
> +    mEffect->NotifySpecifiedTimingUpdated();
> +  }

I wonder we can add a utility method to call this notification method with the null check of mEffect.  Otherwise this null check will be needed in all other methods?

::: dom/animation/KeyframeEffect.cpp
@@ +2143,5 @@
> +{
> +  nsIDocument* doc = nullptr;
> +  if (mTarget &&
> +      mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> +    doc = mTarget->OwnerDoc();

I think KeyframeEffect can already handle pseudo elements.  We don't need to below process for pseudo elements?

@@ +2147,5 @@
> +    doc = mTarget->OwnerDoc();
> +  }
> +
> +  nsAutoAnimationMutationBatch mb(doc);
> +

What will happen if 'doc' is nullptr?
I think if the document is null, we don't need to notify the change to observers and update layer.

@@ +2148,5 @@
> +  }
> +
> +  nsAutoAnimationMutationBatch mb(doc);
> +
> +  if(mAnimation){

Nit: a space is needed before and after parenthesis.

@@ +2160,5 @@
> +    if (presContext) {
> +      presContext->EffectCompositor()->
> +        RequestRestyle(mTarget, mPseudoType, EffectCompositor::RestyleType::Layer,
> +                       mAnimation->CascadeLevel());
> +    }

I am not sure but Animation::NotifyEffectTimingUpdated() does not cause the layer update?

Comment 21

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #20)
> > +  if (mEffect) {
> > +    mEffect->NotifySpecifiedTimingUpdated();
> > +  }
> 
> I wonder we can add a utility method to call this notification method with
> the null check of mEffect.  Otherwise this null check will be needed in all
> other methods?

I think we can add the utility method once we have more than one call site?

> ::: dom/animation/KeyframeEffect.cpp
> @@ +2143,5 @@
> > +{
> > +  nsIDocument* doc = nullptr;
> > +  if (mTarget &&
> > +      mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {
> > +    doc = mTarget->OwnerDoc();
> 
> I think KeyframeEffect can already handle pseudo elements.  We don't need to
> below process for pseudo elements?

The trouble is the animation mutation observer setup doesn't work with pseudo elements yet. We should file a bug for that and add a comment about that here. CC'ing Boris since I'm not sure if he's aware about this yet.

> @@ +2147,5 @@
> > +    doc = mTarget->OwnerDoc();
> > +  }
> > +
> > +  nsAutoAnimationMutationBatch mb(doc);
> > +
> 
> What will happen if 'doc' is nullptr?
> I think if the document is null, we don't need to notify the change to
> observers and update layer.

Yeah, I looked at this yesterday and nsAutoAnimationMutationBatch seems to check for and return early if doc is nullptr. In Animation, however, we have a special helper class that uses Maybe and emplace() to to only create the nsAutoAnimationMutationBatch if doc is non-null. I don't know if that's necessary here, however.

> @@ +2160,5 @@
> > +    if (presContext) {
> > +      presContext->EffectCompositor()->
> > +        RequestRestyle(mTarget, mPseudoType, EffectCompositor::RestyleType::Layer,
> > +                       mAnimation->CascadeLevel());
> > +    }
> 
> I am not sure but Animation::NotifyEffectTimingUpdated() does not cause the
> layer update?

No, it only ever triggers a throttled or standard restyle. We should make sure we have a test that fails without this call, however.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720661 [details] [diff] [review]
Part4 Add duration tests in dom/animation/test/chrome

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

I'd punt a review to my English teacher. Brian?

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1471,5 @@
> +addAsyncAnimTest("change_duration_and_currenttime", { observe: div, subtree: true }, function*() {
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "animation is added");
> +

I am surprised there is no test case for Element.animate().

@@ +1472,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "animation is added");
> +
> +  yield await_frame();

Do we really need a frame here?

@@ +1477,5 @@
> +  anim.effect.timing.duration = 10000;
> +  yield await_frame();
> +
> +  assert_records([{ added: [], changed: [anim], removed: [] }], "records after is changed");
> +

"after duration is changed"?

@@ +1483,5 @@
> +  yield await_frame();
> +  assert_records([], "records after assigning same value");
> +
> +  anim.currentTime = 50000;
> +  anim.effect.timing.duration = 10000;

Setting duration to the same value here is really necessary?
And I think this is a test for currentTime, isn't it?

::: dom/animation/test/chrome/test_restyles.html
@@ +357,5 @@
> +
> +    animation.effect.timing.duration = 100000;
> +    yield waitForFrame();
> +    ok(animation.isRunningOnCompositor);
> +

This test does not check the stylings at all. :-)
Need observingStyling() somewhere.

::: dom/animation/test/chrome/test_running_on_compositor.html
@@ +288,5 @@
> +  var div = addDiv(t);
> +  var animation = div.animate({ opacity: [ 0, 1 ] }, 100000);
> +
> +  return animation.ready.then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled, '');

Need a description.

@@ +295,5 @@
> +    animation.effect.timing.duration = 10000;
> +
> +    assert_equals(animation.isRunningOnCompositor, false,
> +       'Animation reports that it is NOT running on the compositor'
> +       + ' when paused');

"when the animation is set a shorter duration than current time" or something...

@@ +304,5 @@
> +  var div = addDiv(t);
> +  var animation = div.animate({ opacity: [ 0, 1 ] }, 10000);
> +
> +  return animation.ready.then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled, '');

Description.

@@ +310,5 @@
> +    animation.currentTime = 50000;
> +
> +    assert_equals(animation.isRunningOnCompositor, false,
> +      'Animation reports that it is NOT running on the compositor'
> +      + ' when paused');

finished?

@@ +315,5 @@
> +
> +    animation.effect.timing.duration = 100000;
> +    return waitForFrame();
> +  })).then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled, '');

Description.

@@ +318,5 @@
> +  })).then(t.step_func(function() {
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled, '');
> +  }));
> +}, 'animation is added from compositor');
> +

to compositor?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720662 [details] [diff] [review]
Part5 Add duration tests in layout/style/test

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

::: layout/style/test/file_animations_effect_timing_duration.html
@@ +38,5 @@
> +  });
> +}, finish, opener.SpecialPowers);
> +
> +addAsyncAnimTest(function *() {
> +  var [ div ] = new_div("");

We should remove ok in new_element, which checks the argument for new_div(), for scriptable animations in a follow up bug or somewhere else.

@@ +43,5 @@
> +  var animation = div.animate({ opacity: [ 0, 1 ], easing: "steps(2, start)"}, 200000);
> +  yield waitForPaints();
> +
> +  omta_is(div, "opacity", 0, RunningOn.Compositor,
> +          "Animation is running on compositor");

I suspect this check will fail once bug 1248532 is fixed.
Can we change using steps(end) instead of steps(start)?

Comment 24

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

Comment on attachment 8720658 [details] [diff] [review]
Part3 Added notificater in AnimationEffectTiming::SetDuration

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

> Part3 Added notificater in AnimationEffectTiming::SetDuration r?hiro

"Notify animation mutation observers from AnimationEffectTiming::SetDuration"

I think this is ok with:

* The commit message fixed
* A comment about bug 1249219 in NotifySpecifiedTimingUpdated
* A test that fails if we drop the call to RequestRestyle

::: dom/animation/KeyframeEffect.cpp
@@ +2142,5 @@
> +void KeyframeEffect::NotifySpecifiedTimingUpdated()
> +{
> +  nsIDocument* doc = nullptr;
> +  if (mTarget &&
> +      mPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement) {

I filed bug 1249219 for making animation mutation observers work with pseudo elements so we should add a comment here:

// Bug 1249219: We don't support animation mutation observers on pseudo-elements yet.

@@ +2147,5 @@
> +    doc = mTarget->OwnerDoc();
> +  }
> +
> +  nsAutoAnimationMutationBatch mb(doc);
> +

Hiro is right, if doc is null we could skip most of this work. However, I think that's a fairly rare case and we probably won't have a pres context either so we will end up skipping this work. So I think this is ok.

@@ +2159,5 @@
> +    nsPresContext* presContext = GetPresContext();
> +    if (presContext) {
> +      presContext->EffectCompositor()->
> +        RequestRestyle(mTarget, mPseudoType, EffectCompositor::RestyleType::Layer,
> +                       mAnimation->CascadeLevel());

If you delete this call to RequestRestyle, do any of the tests fail?

::: dom/animation/KeyframeEffect.h
@@ +423,5 @@
>    {
>      return ConstructKeyframeEffect<KeyframeEffect>(aGlobal, aTarget, aFrames,
>                                                     aTiming, aRv);
>    }
> +  void NotifySpecifiedTimingUpdated();

Nit: Maybe add a blank line before this since it's not really related to the constructor at all (and the constructor is inline so it probably should sit alone).

Comment 25

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

Comment on attachment 8720661 [details] [diff] [review]
Part4 Add duration tests in dom/animation/test/chrome

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

This is looking good but still needs some more work.

I haven't looked at everything yet but I think that's enough feedback for now.

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1467,5 @@
>    div.style = "";
>    childA.remove();
>    childB.remove();
>  });
> +addAsyncAnimTest("change_duration_and_currenttime", { observe: div, subtree: true }, function*() {

Nit: Need a blank link before this.

@@ +1468,5 @@
>    childA.remove();
>    childB.remove();
>  });
> +addAsyncAnimTest("change_duration_and_currenttime", { observe: div, subtree: true }, function*() {
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);

Can we make this longer, like 100000?

If we are on a slow machine the animation might finish running (when we call await_frame below) then we might end up with a removed event instead.

@@ +1472,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "animation is added");
> +
> +  yield await_frame();

Like Hiro said, we can drop the await_frame() here I think.

@@ +1477,5 @@
> +  anim.effect.timing.duration = 10000;
> +  yield await_frame();
> +
> +  assert_records([{ added: [], changed: [anim], removed: [] }], "records after is changed");
> +

Like Hiro said, "after duration is changed".

@@ +1483,5 @@
> +  yield await_frame();
> +  assert_records([], "records after assigning same value");
> +
> +  anim.currentTime = 50000;
> +  anim.effect.timing.duration = 10000;

Like Hiro said, we don't need to set the duration again here.

(Regarding whether this is a test for currentTime or not, I think we do need this test because it tests that the animation batching works as expected. I believe that without the batching code some of these tests will fail.)

@@ +1485,5 @@
> +
> +  anim.currentTime = 50000;
> +  anim.effect.timing.duration = 10000;
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [], removed: [anim] }], "records after animation finished");

Please wrap this to < 80 chars.

@@ +1489,5 @@
> +  assert_records([{ added: [], changed: [], removed: [anim] }], "records after animation finished");
> +
> +  anim.effect.timing.duration = 100000;
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "records after animation started");

Please wrap to < 80 chars.

@@ +1492,5 @@
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "records after animation started");
> +
> +  anim.cancel();
> +  yield await_frame();

We should probably wait for the final removal notification just to be consistent.

@@ +1493,5 @@
> +  assert_records([{ added: [anim], changed: [], removed: [] }], "records after animation started");
> +
> +  anim.cancel();
> +  yield await_frame();
> +});

We need to add a test for "auto":

* Check that we get a change
* Check that setting it twice does *not* produce a change

(In future we should also test that setting some bad string does not generate a change but I need to change the way that timing interface works first.)

::: dom/animation/test/chrome/test_restyles.html
@@ +343,5 @@
>         'Bug 1235478: Animations running on the compositor should only once ' +
>         'update style when currentTime is set to middle of duration time');
>      yield ensureElementRemoval(div);
>    });
> +  add_task_if_omta_enabled(function* change_duration_and_currenttime() {

Nit: blank line before this

Also, do we need this test? If we do, then, like Hiro says we should check the number of restyles.

::: dom/animation/test/chrome/test_running_on_compositor.html
@@ +283,5 @@
>         + 'still report that it is running on the compositor');
>    }));
>  }, 'isRunningOnCompositor is true when a property that would otherwise block ' +
>     'running on the compositor is overridden in the CSS cascade');
> +promise_test(function(t) {

Nit: Blank line before this.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720657 [details] [diff] [review]
Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp

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

::: dom/animation/AnimationEffectTiming.cpp
@@ +30,5 @@
> +      mTiming.mDuration.SetAsUnrestrictedDouble() = 0;
> +    }
> +  } else {
> +    mTiming.mDuration.SetAsUnrestrictedDouble() = 0;
> +  }

I am not still confident we should convert the duration value in this setter because the value passed to KeyframeEffect constructor is returned as it is.

We are going to change the behavior in bug 1237173?

If so, can we change these codes like below?

1. convert aDuration to TimeDuration or something
2. compare it with the previous value
3. early return if it's the same
4. set new duration

With the current patch, we will call NotifySpecifiedTimingUpdated when set duration 0 after setting duration "auto" or some other cases.

Comment 27

[Ryo Motozawa [:ryo]]

Attachment: Part1 Let AnimationEffectTiming having effect v2

Removed "explicit" and unnecessary include header.
Added "MOZ_NON_OWNING_REF" to mEffect and added "override" to KeyframeEffect destructor.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8721119 [details] [diff] [review]
Part1 Let AnimationEffectTiming having effect v2

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

::: dom/animation/AnimationEffectTiming.h
@@ +12,1 @@
>  

Nit: For MOZ_NON_OWNING_REF.

@@ +15,5 @@
>  
>  class AnimationEffectTiming : public AnimationEffectTimingReadOnly
>  {
>  public:
> +  AnimationEffectTiming(KeyframeEffect* aEffect, const TimingParams& aTiming)

I said I'd prefer KeyframeEffect is the first argument, but it might be good to be in the second position if we can't use the effect as a parent object.  I am sorry for the confusion.

::: dom/animation/KeyframeEffect.h
@@ +430,1 @@
>  };

I did not notice KeyframEffectReadOnly has explicit destructor.

Comment 29

[Ryo Motozawa [:ryo]]

Attachment: Part1 Let AnimationEffectTiming having effect v3

I fixed comment and changed arguments order.

Comment 30

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v2

I modified implementation of SerDuration.

Comment 31

[Ryo Motozawa [:ryo]]

Attachment: Part3 Added notificater in AnimationEffectTiming::SetDuration v2

I fixed commit message and add a comment about bug 1249219.
RequestRestyle is required. I'm going to add a test about this in part5 patch.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8721163 [details] [diff] [review]
Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v2

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

Thanks for the changing.

::: dom/animation/AnimationEffectTiming.cpp
@@ +21,5 @@
> +void
> +AnimationEffectTiming::SetDuration(const UnrestrictedDoubleOrString& aDuration)
> +{
> +  OwningUnrestrictedDoubleOrString pd;
> +  this->GetDuration(pd);

'this' is unnecessary in this instance here.
Can't we use mTiming directly here?

@@ +22,5 @@
> +AnimationEffectTiming::SetDuration(const UnrestrictedDoubleOrString& aDuration)
> +{
> +  OwningUnrestrictedDoubleOrString pd;
> +  this->GetDuration(pd);
> +  double prevDuration = pd.GetAsUnrestrictedDouble();

I'd prefer to check the previous duration type here even if it will be changed in bug 1237173, but it's up to you.

@@ +25,5 @@
> +  this->GetDuration(pd);
> +  double prevDuration = pd.GetAsUnrestrictedDouble();
> +  double duration;
> +  if (aDuration.IsUnrestrictedDouble() && aDuration.GetAsUnrestrictedDouble() >= 0) {
> +    duration = aDuration.GetAsUnrestrictedDouble();

Nit: over 80 chars.

@@ +34,5 @@
> +  if (duration == prevDuration) {
> +    return;
> +  } else {
> +    mTiming.mDuration.SetAsUnrestrictedDouble() = duration;
> +  }

If (duration == prevDuration) {
  return;
}

mTiming.mDurarion....

From http://www-archive.mozilla.org/hacking/mozilla-style-guide.html#General
"Don't put an else right after a return. Delete the else, it's unnecessary and increases indentation level"

r=me with these changes.  This patch needs to be reviewed by a DOM peer.

Thank you,

Comment 33

[Ryo Motozawa [:ryo]]

Attachment: Part3 Added notificater in AnimationEffectTiming::SetDuration v3

Removed the modification should be in other patch.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8720663 [details] [diff] [review]
Part6 Add duration tests in testing/web-platform/tests/web-animations

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

animatable/functional.html should be moved into animation-effect-timing/duration.html, isn't it?
And I think getComputedStyle.html should be also moved into animation-effect-timing/.
What do you think?

I am guessing bunch of tests for getAnimations and getComputedStyle for AnimationEffectTiming attributes could be written similar manners. 

I'd like to review again once this patch is updated.
Thank you,

::: testing/web-platform/tests/web-animations/animatable/functional.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>functional tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animatable-functional">

Should link to AnimationEffectTiming.duration

@@ +9,5 @@
> +<link rel="stylesheet" href="/resources/testharness.css">
> +<body>
> +<div id="log"></div>
> +<iframe src="data:text/html;charset=utf-8," width="10" height="10"
> +  id="iframe"></iframe>

This iframe (also in other files) should be removed.

@@ +19,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  anim.effect.timing.duration = 123.45;
> +  assert_equals(anim.effect.getComputedTiming().duration, 123.45, "set duration 123.45");
> +  anim.effect.timing.duration = "auto";
> +  assert_equals(anim.effect.getComputedTiming().duration, 0, "set duration \"auto\"");

Would you like to use single quote instead of double one in this file?

@@ +27,5 @@
> +
> +  anim.effect.timing.duration = "100";
> +  assert_equals(anim.effect.getComputedTiming().duration, 0, "set duration \"100\"");
> +  anim.effect.timing.duration = "abc";
> +  assert_equals(anim.effect.getComputedTiming().duration, 0 , "set duration \"abc\"");

I'd love to see Infinity case.

@@ +36,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  anim.finish();
> +  assert_equals(anim.playState, 'finished', "anim.playState should be finished");
> +}, "event test");
> +

Is this test necessary?

@@ +37,5 @@
> +  anim.finish();
> +  assert_equals(anim.playState, 'finished', "anim.playState should be finished");
> +}, "event test");
> +
> +done();

done() is not necessary if there is no async_test.

::: testing/web-platform/tests/web-animations/animatable/getAnimations.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Element.getAnimations tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animatable-getAnimations">

Use correct link.

@@ +21,5 @@
> +  assert_equals(div.getAnimations().length, 0, "finish");
> +  anim.effect.timing.duration += 100000;
> +  assert_equals(div.getAnimations()[0], anim, "change duration");
> +  anim.effect.timing.duration = 0;
> +  assert_equals(div.getAnimations().length, 0, "change duration");

You should change descriptions respectively, otherwise you can not easily find which assertion fails at first glance if the test fails.

@@ +25,5 @@
> +  assert_equals(div.getAnimations().length, 0, "change duration");
> +  anim.effect.timing.duration = "auto";
> +  assert_equals(div.getAnimations().length, 0, "auto");
> +}, "getAnimation test");
> +

This html file is for 'Element.getAnimations() tests', so we should name each test properly.

@@ +26,5 @@
> +  anim.effect.timing.duration = "auto";
> +  assert_equals(div.getAnimations().length, 0, "auto");
> +}, "getAnimation test");
> +
> +done();

This should be removed too.

::: testing/web-platform/tests/web-animations/animatable/getComputedStyle.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>getComputedStyle tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animatable-getComputedStyle">

If we move this file into animation-effect-timing/, link to the interface here.

@@ +18,5 @@
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100000);
> +  anim.finish();
> +  assert_equals(getComputedStyle(div).opacity, "1", "finish");
> +  anim.effect.timing.duration += 100000;

*= 2 is easier to understand?

@@ +21,5 @@
> +  assert_equals(getComputedStyle(div).opacity, "1", "finish");
> +  anim.effect.timing.duration += 100000;
> +  assert_equals(getComputedStyle(div).opacity, "0.5", "change duration");
> +  anim.effect.timing.duration = 0;
> +  assert_equals(getComputedStyle(div).opacity, "1", "change duration");

Need a different description.

@@ +24,5 @@
> +  anim.effect.timing.duration = 0;
> +  assert_equals(getComputedStyle(div).opacity, "1", "change duration");
> +  anim.effect.timing.duration = "auto";
> +  assert_equals(getComputedStyle(div).opacity, "1", "auto");
> +}, "duration immediately changed");

Changind duration immediately updates its computed styles?

Comment 35

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v3

I modified implementation of AnimationEffectTiming::SetDuration to check previous duration.
Could you review again?

Comment 36

[Hiroyuki Ikezoe (:hiro)]

(In reply to Ryo Motozawa [:ryo] from comment #35)
> Created attachment 8721874 [details] [diff] [review]
> Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp
> v3
> 
> I modified implementation of AnimationEffectTiming::SetDuration to check
> previous duration.
> Could you review again?

I am a bit confused.  Have you changed your mind to store the duration as it is in this bug?

Comment 37

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> (In reply to Ryo Motozawa [:ryo] from comment #35)
> > Created attachment 8721874 [details] [diff] [review]
> > Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp
> > v3
> > 
> > I modified implementation of AnimationEffectTiming::SetDuration to check
> > previous duration.
> > Could you review again?
> 
> I am a bit confused.  Have you changed your mind to store the duration as it
> is in this bug?

I suggested we shouldn't try to change the behavior in this bug but leave that until bug 1237173 so that is my fault. However, this patch still clamps negative durations which I don't think is necessary.

Comment 38

[Ryo Motozawa [:ryo]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> (In reply to Ryo Motozawa [:ryo] from comment #35)
> > Created attachment 8721874 [details] [diff] [review]
> > Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp
> > v3
> > 
> > I modified implementation of AnimationEffectTiming::SetDuration to check
> > previous duration.
> > Could you review again?
> 
> I am a bit confused.  Have you changed your mind to store the duration as it
> is in this bug?

Yes, I changed behavior of function because of bug 1237173.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8721874 [details] [diff] [review]
Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v3

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

If we store the duration as it is, we can now use operator= [1].

[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/mozilla/dom/UnionTypes.h#13335

::: dom/animation/AnimationEffectTiming.cpp
@@ +37,5 @@
> +      }
> +    }
> +    mTiming.mDuration.SetAsString() = aDuration.GetAsString();
> +  }
> +}

I don't know why OwningUnrestrictedDoubleOrString does not have operator==, but anyway I'd prefer like this:

  if (mTiming.mDuration.IsUnrestrictedDouble() == aDuration.IsUnrestrictedDouble() &&
      mTiming.mDuration.GetAsUnrestrictedDouble() == aDuration.GetAsUnrestrictedDouble()) {
    return;
  }

  if (mTiming.mDuration.IsString() == aDuration.IsString() &&
      mTiming.mDuration.GetAsString() == aDuration.GetAsString()) {
    return;
  }

This causes a redundant IsString() check, but it's inlined. 

r=me with the changes.

Comment 40

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #39)
> Comment on attachment 8721874 [details] [diff] [review]
> Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp
> v3
> 
> Review of attachment 8721874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we store the duration as it is, we can now use operator= [1].
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/
> dist/include/mozilla/dom/UnionTypes.h#13335
> 
> ::: dom/animation/AnimationEffectTiming.cpp
> @@ +37,5 @@
> > +      }
> > +    }
> > +    mTiming.mDuration.SetAsString() = aDuration.GetAsString();
> > +  }
> > +}
> 
> I don't know why OwningUnrestrictedDoubleOrString does not have operator==,
> but anyway I'd prefer like this:
> 
>   if (mTiming.mDuration.IsUnrestrictedDouble() ==
> aDuration.IsUnrestrictedDouble() &&
>       mTiming.mDuration.GetAsUnrestrictedDouble() ==
> aDuration.GetAsUnrestrictedDouble()) {
>     return;
>   }
> 
>   if (mTiming.mDuration.IsString() == aDuration.IsString() &&
>       mTiming.mDuration.GetAsString() == aDuration.GetAsString()) {
>     return;
>   }

Ah, this will end up calling NotifySpecifiedTimingUpdated, when we change the duration value from "string" to "other" something.  We should avoid it.

Comment 41

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

(In reply to Brian Birtles (:birtles) from comment #37)
> I suggested we shouldn't try to change the behavior in this bug but leave
> that until bug 1237173 so that is my fault. However, this patch still clamps
> negative durations which I don't think is necessary.

Just to clarify, if you pass in a string or negative duration to the constructor or element.animate(), we currently don't normalize or clamp it or anything like that. So I think setting the duration should be consistent. Then, in a separate bug, we can change the behavior of all setter functions / constructors at the same time.

Comment 42

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v4

We don't normalize or clamp duration in this patch as stated in the comment #41.

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8721937 [details] [diff] [review]
Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v4

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

::: dom/animation/AnimationEffectTiming.cpp
@@ +38,5 @@
> +      aDuration.GetAsUnrestrictedDouble();
> +  } else {
> +    mTiming.mDuration.SetAsString() = aDuration.GetAsString();
> +  }
> +}

Is there any reason why we can't use operator=?

Comment 44

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

What do you refer to with that question?

Comment 45

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

Comment on attachment 8721937 [details] [diff] [review]
Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v4

>+AnimationEffectTiming::SetDuration(const UnrestrictedDoubleOrString& aDuration)
>+{
>+  if (mTiming.mDuration.IsUnrestrictedDouble() &&
>+        aDuration.IsUnrestrictedDouble() &&
odd indentation here. aDuration should be below mTiming

Comment 46

[Hiroyuki Ikezoe (:hiro)]

I was just wondering if we can write 'mTiming.mDuration = aDuration' at the last of the function.

Comment 47

[Ryo Motozawa [:ryo]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> I was just wondering if we can write 'mTiming.mDuration = aDuration' at the
> last of the function.

aDuration is UnrestrictedDoubleOrString but mTiming.mDuration is OwningUnrestrictedDoubleOrString.
We can't convert these types.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

I am sorry for my misreading..  I am surprised there is no such operator, then.  Nobody does not want to use the operator?  I could not find any bug for such request. Interesting.

Comment 49

[Ryo Motozawa [:ryo]]

Attachment: Part2 Add duration implementation in dom/animation/AnimationEffectTiming.cpp v5

I fixed indentation.

Comment 50

[Ryo Motozawa [:ryo]]

Attachment: Part4 Add duration tests in dom/animation/test/chrome v2

I added "auto" test and fixed comment in dom/animation/test/chrome/test_animation_observers.html.
I added observeStyling() test in dom/animation/test/chrome/test_restyles.html.
I fixed comment in dom/animation/test/chrome/test_running_on_compositor.html.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8722328 [details] [diff] [review]
Part4 Add duration tests in dom/animation/test/chrome v2

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

::: dom/animation/test/chrome/test_animation_observers.html
@@ +1490,5 @@
> +
> +  anim.currentTime = 50000;
> +  yield await_frame();
> +  assert_records([{ added: [], changed: [], removed: [anim] }],
> +                 "records after animation finished");

Let's use "animation end" for consistency with other tests.

@@ +1495,5 @@
> +
> +  anim.effect.timing.duration = 100000;
> +  yield await_frame();
> +  assert_records([{ added: [anim], changed: [], removed: [] }],
> +                 "records after animation started");

restarted?

I am pretty sure below code notifies one addedAnimations and one removedAnimations.

anim.effect.timing.duration = 100000;
anim.effect.timing.duration = 10;

Brian, should we file a new bug?  I think it's basically harmless though because devtools is the only one user of the observer.

@@ +1508,5 @@
> +  assert_records([], "records after assigning same value \"auto\"");
> +
> +  anim.cancel();
> +  yield await_frame();
> +  assert_records([], "records after animation end");

I don't think this check is necessary for this test case.

::: dom/animation/test/chrome/test_restyles.html
@@ +362,5 @@
> +    ok(animation.isRunningOnCompositor);
> +    var markers = yield observeStyling(5);
> +    is(markers.length, 0,
> +       'Animations running on the compositor should not update style');
> +

You don't need wait a frame here.  You can do like this:

animation.effect.timing.duration = 100000;
var markers = yield observeStyling(5)
is(markers.length, 1, ...);

I think we eventually this kind of test cases should run one and the same function against various test cases. e.g. setting currentTime, duration, etc.

::: dom/animation/test/chrome/test_running_on_compositor.html
@@ +188,5 @@
>      }));
>    }));
>  }, 'isRunningOnCompositor is true in requestAnimationFrame callback');
>  
> +

Nit: remove a line.

@@ +300,5 @@
> +    assert_equals(animation.isRunningOnCompositor, false,
> +       'Animation reports that it is NOT running on the compositor'
> +       + ' when the animation is set a shorter duration than current time');
> +  }));
> +}, 'animation is immediately removed from compositor');

removed from compositor when timing.duration is made shorter than the current time.

@@ +323,5 @@
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +      'Animation reports that it is running on the compositor'
> +      + ' when restarted');
> +  }));
> +}, 'animation is added to compositor');

added to compositor when timing.duration is made longer than the current time.

Comment 52

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #51)
> @@ +1495,5 @@
> > +
> > +  anim.effect.timing.duration = 100000;
> > +  yield await_frame();
> > +  assert_records([{ added: [anim], changed: [], removed: [] }],
> > +                 "records after animation started");
> 
> restarted?
> 
> I am pretty sure below code notifies one addedAnimations and one
> removedAnimations.
> 
> anim.effect.timing.duration = 100000;
> anim.effect.timing.duration = 10;

Assuming currentTime >= 10 and < 1000000, yes, that's right. We don't batch mutation observer records across Javascript calls.

> Brian, should we file a new bug?  I think it's basically harmless though
> because devtools is the only one user of the observer.

I think this is consistent with what we do elsewhere for script-manipulated animations so I think it's ok.

e.g. https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/dom/animation/test/chrome/test_animation_observers.html#1001

Comment 53

[Ryo Motozawa [:ryo]]

Attachment: Part4 Add duration tests in dom/animation/test/chrome v3

I fixed comments and removed unnecessary sentence.

Comment 54

[Ryo Motozawa [:ryo]]

Attachment: Part5 Add duration tests in layout/style/test v2

I added test that fails if we drop the call to RequestRestyle.
Modified to avoid effect of bug 1248532.

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8722360 [details] [diff] [review]
Part5 Add duration tests in layout/style/test v2

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

Great!

::: layout/style/test/file_animations_effect_timing_duration.html
@@ +52,5 @@
> +
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 50 }, RunningOn.Compositor,
> +          "Animation remains on compositor");
> +

I'd like to see the result at the switching point, say, setting the duration to 2000 when the current time is 1000.  To write this case, you might want to use steps(end).  Is the case covered by other tests?

::: layout/style/test/mochitest.ini
@@ +43,5 @@
>  support-files = ../../reftests/fonts/Ahem.ttf file_animations_async_tests.html
>  [test_animations_dynamic_changes.html]
>  [test_animations_event_order.html]
> +[test_animations_effect_timing_duration.html]
> +support-files = file_animations_effect_timing_duration.html

Nit: alphabetical order

Comment 56

[Ryo Motozawa [:ryo]]

Attachment: Part5 Add duration tests in layout/style/test v3

I added new test case.
Fixed order of layout/style/test/mochitest.ini.

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8722373 [details] [diff] [review]
Part5 Add duration tests in layout/style/test v3

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

::: layout/style/test/file_animations_effect_timing_duration.html
@@ +63,5 @@
> +
> +addAsyncAnimTest(function *() {
> +  var [ div ] = new_div("");
> +  var animation = div.animate(
> +    [ { transform: 'translate(0px)', easing: "steps(1, end)" },

You should use steps(2,end).

@@ +71,5 @@
> +  advance_clock(1000);
> +  animation.effect.timing.duration = 2000;
> +  yield waitForPaints();
> +  omta_is(div, "transform", { tx: 0 }, RunningOn.Compositor,
> +          "Animation is running on compositor");

tx will be 100 at this point if you use steps(2,end).

Comment 58

[Ryo Motozawa [:ryo]]

Attachment: Part5 Add duration tests in layout/style/test v4

I fixed the test to use steps(2,end).

Comment 59

[Ryo Motozawa [:ryo]]

Attachment: Part5 Add duration tests in layout/style/test v5

I forgot to do qrefresh.

Comment 60

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations v2

Comment 61

[Ryo Motozawa [:ryo]]

Comment on attachment 8722412 [details] [diff] [review]
Part6 Add duration tests in testing/web-platform/tests/web-animations v2

I moved testing/web-platform/tests/web-animations/animatable/functional.html to ../web-animations/animation-effect-timing/duration.html 
and /web-animations/animatable/getAnimations.html to /web-animations/animation-effect-timing/getAnimations.html.

Added Infinity test in duration.html.
Fixed comment, to use single quote, removed unnecessary sentence.

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8722412 [details] [diff] [review]
Part6 Add duration tests in testing/web-platform/tests/web-animations v2

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

I still think we should move getComputedStyle.html in animation-effect-timing/.

Basically this patch is OK, I'd like to know the answer about commented out test.

::: testing/web-platform/meta/MANIFEST.json
@@ +27422,5 @@
> +      {
> +        "path": "web-animations/animatable/getComputedStyle.html",
> +        "url": "/web-animations/animatable/getComputedStyle.html"
> +      },
> +      {

You need to fix these paths too.

::: testing/web-platform/tests/web-animations/animatable/getComputedStyle.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>getComputedStyle tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-duration">

http://w3c.github.io/web-animations/#animationeffecttiming

@@ +17,5 @@
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 100000);
> +  anim.finish();
> +  assert_equals(getComputedStyle(div).opacity, '1', 'animation finished');
> +  anim.effect.timing.duration *= 2;
> +  assert_equals(getComputedStyle(div).opacity, '0.5', 'set duration 200000');

'double duration'

@@ +23,5 @@
> +  assert_equals(getComputedStyle(div).opacity, '1', 'set duration 0');
> +  anim.effect.timing.duration = 'auto';
> +  assert_equals(getComputedStyle(div).opacity, '1', 'set duration \'auto\'');
> +}, 'changed duration immediately updates its computed styles');
> +

This description is really good!

::: testing/web-platform/tests/web-animations/animation-effect-timing/duration.html
@@ +23,5 @@
> +                'set duration \'auto\'');
> +  // anim.effect.timing.duration = -100;
> +  // assert_equals(anim.effect.timing.duration, 0);
> +  // assert_equals(anim.effect.getComputedTiming().duration, 0,
> +  //               'set duration -100');

What do you imply by this commented out test?

@@ +35,5 @@
> +
> +  anim.effect.timing.duration = Infinity;
> +  assert_equals(anim.effect.getComputedTiming().duration, Infinity,
> +                'set duration Infinity');
> +}, 'functional test');

This test does not check returned values of timing.duration at all.  Shouldn't we do it?

@@ +44,5 @@
> +  anim.finish();
> +  assert_equals(anim.playState, 'finished',
> +                'anim.playState should be finished');
> +}, 'event test');
> +

This test is not related to duration, right?

::: testing/web-platform/tests/web-animations/animation-effect-timing/getAnimations.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Element.getAnimations tests</title>
> +<link rel="help" href="http://w3c.github.io/web-animations/#dom-animationeffecttiming-duration">

Please use http://w3c.github.io/web-animations/#animationeffecttiming
This file will be used for other attributes of AnimationEffectTiming, I think.

@@ +23,5 @@
> +  assert_equals(div.getAnimations().length, 0, 'set duration 0');
> +  anim.effect.timing.duration = 'auto';
> +  assert_equals(div.getAnimations().length, 0, 'set duration \'auto\'');
> +}, 'getAnimation test');
> +

'when duration is changed'

Comment 63

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations v3

Moved getComputedStyle.html to testing/web-platform/tests/web-animations/animation-effect-timing.
Added .ini file for duration.html.
Modified testing/web-platform/meta/MANIFEST.json.

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8722824 [details] [diff] [review]
Part6 Add duration tests in testing/web-platform/tests/web-animations v3

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

::: testing/web-platform/tests/web-animations/animation-effect-timing/duration.html
@@ +15,5 @@
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
> +  anim.effect.timing.duration = 123.45;
> +  assert_equals(anim.effect.timing.duration, 123.45);

This assertion (in other tests too) should have description respectively.

Comment 65

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations v4

I added comments.

Comment 66

[Ryo Motozawa [:ryo]]

Attachment: Part3 Added notificater in AnimationEffectTiming::SetDuration v5

I changed to use CSSPseudoElementType::NotPseudo instead of nsCSSPseudoElements::ePseudo_NotPseudoElement.

Comment 67

[Ryo Motozawa [:ryo]]

Attachment: Part1 Let AnimationEffectTiming having effect v4

Solved conflict in newest tree.

Comment 68

[Hiroyuki Ikezoe (:hiro)]

Pushed to try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d3115bb90b4

Comment 69

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

I looked into the Windows failure:

  assert_equals: getComputedTiming() after set duration 123.45 expected 123.45 but got 123.44999978006251

It comes about when we store the computed duration as a TimeDuration. We convert 123.45 to a number of ticks using ms2mt which gives 336777525.60000002. However, we then go and store that as a 64-bit integer which gives 336777525.

Later when we go to retrieve the value we do the reverse conversion in BaseTimeDurationPlatformUtils::ToSeconds(int64_t aTicks) where we do:

  return double(aTicks) / (double(sFrequencyPerSec) * 1000.0);

Here sFrequencyPerSec = 2728048 (int64), so we have:

  return double(336777525) / (double(2728048) * 1000.0)
       = 336777525.0 / (2728048.0 * 1000.0)
       = 0.12344999978006252

Converting back to milliseconds gives 123.44999978006253　which gives an error of 0.00000021993747 milliseconds. Web Animations only recommends microsecond precision[1] so this is well within implementation specifications.

Note that if we round when storing as an int, we get 123.45000014662499 as the result which gives us an error of 0.00000014662499 milliseconds so, roughly 2/3rds of the error.

I think it is fine to make this test use assert_approx_equals since we shouldn't be expecting implementations to have better than microsecond precision.

[1] https://w3c.github.io/web-animations/#precision-of-time-values

Comment 70

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations v5

Changed to use assert_approx_equals.

Comment 71

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

Comment on attachment 8723415 [details] [diff] [review]
Part6 Add duration tests in testing/web-platform/tests/web-animations v5

>+test(function(t) {
>+  var div = createDiv(t);
>+  var anim = div.animate({ opacity: [ 0, 1 ] }, 2000);
>+  anim.effect.timing.duration = 123.45;
>+  assert_approx_equals(anim.effect.timing.duration, 123.45, 0.000001,
>+                       'set duration 123.45');
>+  assert_equals(anim.effect.getComputedTiming().duration, 123.45,
>+                'getComputedTiming() after set duration 123.45');
>+}, 'set duration 123.45');

I think this is around the wrong way? It's getComputedTiming() where we have precision issues.

But we should make both tests use approx_equals I think.

Comment 72

[Ryo Motozawa [:ryo]]

Attachment: Part6 Add duration tests in testing/web-platform/tests/web-animations v6

Changed to use assert_approx_equals in getComputedTiming() assertion.

Comment 73

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79e94c6596fd

Comment 74

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72317916a0f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb39a42909ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/0907eee5c44a
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a37a771e9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/adc86cbfeb50
https://hg.mozilla.org/integration/mozilla-inbound/rev/3374f3adff0d

Comment 75

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72317916a0f3
https://hg.mozilla.org/mozilla-central/rev/bb39a42909ea
https://hg.mozilla.org/mozilla-central/rev/0907eee5c44a
https://hg.mozilla.org/mozilla-central/rev/57a37a771e9a
https://hg.mozilla.org/mozilla-central/rev/adc86cbfeb50
https://hg.mozilla.org/mozilla-central/rev/3374f3adff0d