Closed Bug 1127380 Opened 5 years ago Closed 4 years ago

Implement AnimationPlayer.playbackRate

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 11 obsolete files)

9.26 KB, patch
birtles
: review+
smaug
: review+
hiro
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
birtles
: review+
hiro
: checkin+
Details | Diff | Splinter Review
5.52 KB, patch
birtles
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 1070744
Assignee: nobody → jwatt
Attached patch patch (obsolete) — Splinter Review
Attachment #8572669 - Flags: review?(bbirtles)
Comment on attachment 8572669 [details] [diff] [review]
patch

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

This looks good but I'd like to check DoPlay again after you've updated it.

Also, ResumeAt needs to be updated to check for a zero playback rate (see http://w3c.github.io/web-animations/#play-a-player).

And this needs tests.

::: dom/animation/AnimationPlayer.cpp
@@ +168,5 @@
> +  if (!previousTime.IsNull()) {
> +    SilentlySetCurrentTime(previousTime);
> +  }
> +}
> +

I don't think SilentSetPlaybackRate is used anywhere yet so can we drop it for now?

@@ +415,5 @@
>    // animation-play-state we *don't* trigger finishing behavior.
>  
> +  // XXX why does this not seem to match very well to
> +  // http://w3c.github.io/web-animations/#playing-a-player-section ???
> +  // Go back over that section and apply the 'rate' changes.

I don't understand this comment.

@@ +421,5 @@
>    Nullable<TimeDuration> currentTime = GetCurrentTime();
> +  if (mPlaybackRate > 0 &&
> +      (currentTime.IsNull() ||
> +       currentTime < 0 ||
> +       currentTime ≥ SourceContentEnd())) {

This is partially introducing finishing behavior (bug 1074630). I'm not sure we want to partially do it here since we'll probably end up with odd behavior where it partially works. In particular, as per the comment at the start of this method, we should *not* do finishing behavior when called due to a change in animation-play-state.

It's probably better to do this all properly in bug 1074630.

@@ +426,5 @@
>      mHoldTime.SetValue(TimeDuration(0));
> +  } else if (mPlaybackRate < 0 &&
> +             (mHoldTime.IsNull() ||
> +              currentTime <= 0 ||
> +              currentTime > SourceContentEnd())) {

Likewise, here too.

@@ +431,1 @@
>      // If the hold time is null, we are already playing normally

I'm not sure this comment still makes sense if we're changing the condition above. We should probably just change this logic to match the pseudocode in the spec.

@@ +438,5 @@
> +
> +  if (mHoldTime.IsNull()) {
> +    if (mReady) {
> +      mReady->MaybeResolve(this);
> +    }

As far as I can tell, this isn't necessary until we implement deferred pausing (bug 1109390) so we should just drop this for now.

::: dom/animation/AnimationPlayer.h
@@ +52,5 @@
>  
>  public:
>    explicit AnimationPlayer(AnimationTimeline* aTimeline)
>      : mTimeline(aTimeline)
> +    , mPlaybackRate(1)

This might be more clear as 1.0.

::: dom/webidl/AnimationPlayer.webidl
@@ +23,5 @@
>    attribute double? startTime;
>    [SetterThrows, BinaryName="currentTimeAsDouble"]
>    attribute double? currentTime;
>  
> +           attribute double             playbackRate;

This will need review from a DOM peer.
Attachment #8572669 - Flags: review?(bbirtles)
Attachment #8572669 - Attachment is obsolete: true
Attachment #8574985 - Flags: review?(bbirtles)
Comment on attachment 8574985 [details] [diff] [review]
part 1 - code changes

Also tag smaug for review of the minor DOM changes.
Attachment #8574985 - Flags: review?(bugs)
Attachment #8574985 - Flags: review?(bugs) → review+
Comment on attachment 8574985 [details] [diff] [review]
part 1 - code changes

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

r=me with comments addressed. Particularly, the one about setting mHoldTime to zero instead of null in DoPlay.

::: dom/animation/AnimationPlayer.cpp
@@ +53,5 @@
>  #endif
>    Nullable<TimeDuration> previousCurrentTime = GetCurrentTime();
>    mStartTime = aNewStartTime;
>    if (!aNewStartTime.IsNull()) {
> +    if (mPlaybackRate != 0) {

Here, and a half a dozen other places in this file I think it would be better to compare against 0.0 so it's clear we're doing floating-point comparison.

@@ +150,5 @@
>    // http://w3c.github.io/web-animations/#update-a-players-finished-state
>  }
>  
> +void
> +AnimationPlayer::SetPlaybackRate(double aPlaybackRate)

The duplication between SetPlaybackRate and SilentlySetPlaybackRate isn't great. With the simplifications to SetCurrentTime proposed in bug 1072037 comment 15 this will become a little more simple.

What do you think about adding an enum parameter to toggle between the two modes? I don't mind if you want to keep them separate however.

@@ +417,5 @@
>    // animation-play-state we *don't* trigger finishing behavior.
>  
>    Nullable<TimeDuration> currentTime = GetCurrentTime();
> +  if (mPlaybackRate > 0 &&
> +      (currentTime.IsNull())) {

Drop the extra parentheses here around currentTime.IsNull() and just put it all on one line? We can re-add them when we need to.

@@ +422,3 @@
>      mHoldTime.SetValue(TimeDuration(0));
> +  } else if (mPlaybackRate < 0 &&
> +             (currentTime.IsNull())) {

Likewise here.

@@ +423,5 @@
> +  } else if (mPlaybackRate < 0 &&
> +             (currentTime.IsNull())) {
> +    mHoldTime.SetValue(TimeDuration(SourceContentEnd()));
> +  } else if (mPlaybackRate == 0 && currentTime.IsNull()) {
> +    mHoldTime.SetNull();

The spec says to set the hold time to zero in this case. Is there a reason we make it null instead?

@@ +480,5 @@
>               "Expected to resume a pending player");
>    MOZ_ASSERT(!mHoldTime.IsNull(),
>               "A player in the pending state should have a resolved hold time");
>  
> +  mStartTime.SetValue(aResumeTime - (mHoldTime.Value() / mPlaybackRate));

I think we need to handle mPlaybackRate == 0.0 here. For this part, the spec says:

> If the player playback rate is zero, let new start time be simply ready time.
Attachment #8574985 - Flags: review?(bbirtles) → review+
Attached patch extra tweakSplinter Review
Attachment #8579965 - Flags: review?(bbirtles)
Attachment #8579965 - Flags: review?(bbirtles) → review+
As requested by Brian, pushed to unblock some of his work:

https://hg.mozilla.org/integration/mozilla-inbound/rev/78195e7115a2

I'll finish the tests and put them up for review soon.
Keywords: leave-open
Blocks: 1120339
Attached patch tests (obsolete) — Splinter Review
There are now several playbackRate tests in:

dom/animation/test/css-animations/test_animation-currenttime.html
dom/animation/test/css-animations/test_animation-finished.html

but we should still add at least these tests.
Attachment #8595708 - Flags: review?(bbirtles)
Comment on attachment 8595708 [details] [diff] [review]
tests

We really should go and add tests to every algorithm that factors in the playbackRate (e.g. when setting the startTime we need to check of playbackRate == 0).

I understand you might not have bandwidth for that now but could we at least add a test that checks that playbackRate actually affects the currentTime (the current test would pass even if we never implemented playbackRate). We could return the times of the document timeline between subsequent frames and then check that the corresponding difference in currentTime is proportional to the playbackRate?
Attachment #8595708 - Flags: review?(bbirtles)
timeline.currentTime isn't a sampled time that remains unchanged during a frame. It's live, so:

  assert_equals(timeline.currentTime, timeline.currentTime, 'blah');

may fail. To work around that there will need to be some sort of error tolerance, but I'm not sure that we can do that and guarantee avoiding orange. Maybe if I make the playbackRate something very large?
Attached patch tests (obsolete) — Splinter Review
Perhaps something like this?
Attachment #8595708 - Attachment is obsolete: true
Attachment #8595850 - Flags: review?(bbirtles)
(In reply to Jonathan Watt [:jwatt] from comment #11)
> timeline.currentTime isn't a sampled time that remains unchanged during a
> frame.

It should remain unchanged during a frame.

> It's live, so:
> 
>   assert_equals(timeline.currentTime, timeline.currentTime, 'blah');
> 
> may fail.

It shouldn't.
(In reply to Jonathan Watt [:jwatt] from comment #12)
> Created attachment 8595850 [details] [diff] [review]
> tests
> 
> Perhaps something like this?

It looks mostly fine but I'm not sure why you say animation.timeline.currentTime is not frozen within a frame?
Attachment #8595850 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #14)
> It looks mostly fine but I'm not sure why you say
> animation.timeline.currentTime is not frozen within a frame?

I'm not sure what I'm recalling, but at some point there was code that got a TimeStamp::Now() call rather than fetching the time from the refresh driver. Anyway, that seems not to be relevant here. That said, I do still seem to need around a 2% error tolerance when running this test locally. I'm not sure where that's coming from.
Attached patch tests (obsolete) — Splinter Review
Attachment #8595850 - Attachment is obsolete: true
Attachment #8598994 - Flags: review?(bbirtles)
Comment on attachment 8598994 [details] [diff] [review]
tests

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

I think this test is assuming that when the ready promise resolves the animation's currentTime is zero but the startTime of animations can actually fall between frames.

Let's assume the we create the animation 500ms after navigationStart and we get one animation every 16ms (i.e. 60fps).

t=500ms:

Animation is played
timeline.currentTime = 500;

t=510ms:

We finish painting the first frame of the animation and record the ready time as '510' (or actually the equivalent TimeStamp)
We store the pending ready time on the animation.

t=516ms: (the next frame)

We apply the ready time to the animation as its startTime:
animation.startTime = 510;
animation.currentTime = 6;

We resolve its ready promise:
initialTimelineTime = animation.timeline.currentTime - animation.currentTime;
i.e. initialTimelineTime = 516 - 6 = 510;

Then we set the playbackRate to 100.
The animation will automatically seek so that its currentTime remains 6. It does this by adjusting the startTime.
animation.startTime = timeline time - (seek time / playback rate) = 516 - (6 / 100) = 515.94

Wait 10 frames (16ms each):

t=676ms:

timelineTimeDelta = animation.timeline.currentTime - initialTimelineTime
                  = 676 - 510
                  = 166

timeAdvanceRatio = animation.currentTime / timelineTimeDelta
                 = [(timeline time - start time) * playback rate] / timelineTimeDelta
                 = [(676 - 515.94) * 100] / timelineTimeDelta
                 = 16006 / 166
                 = 96.422...

Hence why the comparison to 100 has a large margin of error.

A few suggestions:

* We should just record the timeline current times and animation current times between subsequent frames and compare the ratio. If we do that, we can set the playbackRate before waiting for the ready promise which simplifies the test somewhat.
* I think a playbackRate of 10 would be better since if the test machine is really lagged, waiting 10 frames with a playbackRate of 100 could put us near to the end of the active interval. In the example above where we run at an ideal 60fps, we still get 16s into the 1000s interval. If a machine is really lagged and takes 1s per frame we would end up outside the active interval and run into finishing behavior which would cause the test to fail.
* Also, I wonder if we need to wait 10 frames? If we do the measurement correctly there should be no error (except floating-point error) so 2 or 3 frames should be plenty.
Attachment #8598994 - Flags: review?(bbirtles)
Blocks: 1178660
jwatt, if you can not take time to finish the the test, may I do it?
Flags: needinfo?(jwatt)
Certainly, that would be very helpful. I think there are probably some bugs that will be found by the tests though.
Flags: needinfo?(jwatt)
Attached file tests v2 (obsolete) —
Updated jwatt's tests.

* Checking that animation.currentTime is surely advanced as expected in each frame.
* Checking that animation element position as well.
* Changing playbackRate to 10, 3, 21 in each frame.
* 100ms busy loop in a frame to simulate lag.

For reference here is a try results on android and b2g.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed526c31256

The patch in the try is slightly different from this patch:
 * does not check animation element position yet
 * does factor out currentTime check as new function

Though Android 4.3 debug tests are not finished yet now, other results are perfect.

One thing that is not clear to me is accuracy for checking element position. It's now 0.1 in this patch because it's pixel but I am not sure it's a proper value there.
Attachment #8598994 - Attachment is obsolete: true
Attachment #8629188 - Flags: feedback?(bbirtles)
Comment on attachment 8629188 [details]
tests v2

Looks good but I think it could be simplified further.

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+
>+  div.style.animation = 'anim 10s';

This fine but I think it could be a little neater to just change the stylesheet to:

  div {
    /* Make it easier to calculate expected values: */
    animation-timing-function: linear ! important;
  }

And then make this:

  var div = addDiv(t, { 'style' : 'animation: anim 10s' });

>+function assert_playbackrate(target,
>+                             previousAnimationCurrentTime,
>+                             previousTimelineCurrentTime,
>+                             description) {
>+  var accuracy = 0.001; /* accuracy of DOMHighResTimeStamp */
>+
>+  var animation = target.getAnimations()[0];

This should just take animation as a parameter (instead of target).

>+function assert_animation_position(target,
>+                                   previousAnimationCurrentTime,
>+                                   previousMarginLeft,
>+                                   description) {
>+  var accuracy = 0.1; /* what is a proper value here? */

I don't think we need this method. We should just test the result of playbackRate on the currentTime (not on margin-left). 

>+async_test(function(t) {
>+  var div = addDiv(t, {'class': 'animated-div'});
>+
>+  div.style.animation = 'anim 10s';
>+
>+  var animation = div.getAnimations()[0];
>+  animation.playbackRate = 10;
>+
>+  var previousTimelineCurrentTime;
>+  var previousAnimationCurrentTime;
>+  var previousMarginLeft;

I don't think we need previousMarginLeft.

>+  animation.ready.then(function() {
>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
>+    return waitForFrame();
>+  }).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 10 times faster than timeline.");

This, I think, is enough? I don't think we need the rest of the test?

>+    // Psuedo busyness
>+    var timeAtStart = window.performance.now();
>+    while (window.performance.now() - timeAtStart < 100) {
>+    }
>+    return waitForFrame();
>+  })).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");

Why would the result differ if the main thread was busy?

>+    assert_animation_position(div,
>+                              previousAnimationCurrentTime,
>+                              previousMarginLeft);
>+
>+    animation.playbackRate = 3;

Why would a playbackRate of 10 work, but a playbackRate of 3 fail?

If this is meant to test a dynamic update of playbackRate while moving, then it should be a separate test. In general we are trying to keep each test as small and isolated as possible.

>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
>+    return waitForFrame();
>+  })).then(t.step_func(function() {
>+    assert_playbackrate(div,
>+                        previousAnimationCurrentTime,
>+                        previousTimelineCurrentTime,
>+                        "animation.currentTime should be 3 times faster than timeline now.");
>+    assert_animation_position(div,
>+                              previousAnimationCurrentTime,
>+                              previousMarginLeft);
>+
>+    animation.playbackRate = 21;

Again, why would 21 be any different?
Attachment #8629188 - Flags: feedback?(bbirtles) → feedback+
Thanks quick reply!

(In reply to Brian Birtles (:birtles) from comment #21)
> >+async_test(function(t) {
> >+  var div = addDiv(t, {'class': 'animated-div'});
> >+
> >+  div.style.animation = 'anim 10s';
> 
> This fine but I think it could be a little neater to just change the
> stylesheet to:
> 
>   div {
>     /* Make it easier to calculate expected values: */
>     animation-timing-function: linear ! important;
>   }
> 
> And then make this:
> 
>   var div = addDiv(t, { 'style' : 'animation: anim 10s' });

Will do.

> >+function assert_playbackrate(target,
> >+                             previousAnimationCurrentTime,
> >+                             previousTimelineCurrentTime,
> >+                             description) {
> >+  var accuracy = 0.001; /* accuracy of DOMHighResTimeStamp */
> >+
> >+  var animation = target.getAnimations()[0];
> 
> This should just take animation as a parameter (instead of target).

Will do.
 
> >+function assert_animation_position(target,
> >+                                   previousAnimationCurrentTime,
> >+                                   previousMarginLeft,
> >+                                   description) {
> >+  var accuracy = 0.1; /* what is a proper value here? */
> 
> I don't think we need this method. We should just test the result of
> playbackRate on the currentTime (not on margin-left). 

I think this sort of tests are necessary to avoid bugs like bug 1175751.
Actually animation.currentTime is correctly advanced in case of CSS transform(the case of bug 1175751).

But I will remove this for now and reconsider it in bug 1175751.

> >+async_test(function(t) {
> >+  var div = addDiv(t, {'class': 'animated-div'});
> >+
> >+  div.style.animation = 'anim 10s';
> >+
> >+  var animation = div.getAnimations()[0];
> >+  animation.playbackRate = 10;
> >+
> >+  var previousTimelineCurrentTime;
> >+  var previousAnimationCurrentTime;
> >+  var previousMarginLeft;
> 
> I don't think we need previousMarginLeft.

Will do.

> >+  animation.ready.then(function() {
> >+    previousAnimationCurrentTime = animation.currentTime;
> >+    previousTimelineCurrentTime = animation.timeline.currentTime;
> >+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
> >+    return waitForFrame();
> >+  }).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 10 times faster than timeline.");
> 
> This, I think, is enough? I don't think we need the rest of the test?
> 
> >+    // Psuedo busyness
> >+    var timeAtStart = window.performance.now();
> >+    while (window.performance.now() - timeAtStart < 100) {
> >+    }
> >+    return waitForFrame();
> >+  })).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");
> 
> Why would the result differ if the main thread was busy?

Actually I do not know.
My intention here is that this is some kind of black-box test.
I imagined that animation.currentTime should be advanced correctly even if some frames are skipped because of heavy load in main thread. But OK I will remove this.

> >+    assert_animation_position(div,
> >+                              previousAnimationCurrentTime,
> >+                              previousMarginLeft);
> >+
> >+    animation.playbackRate = 3;
> 
> Why would a playbackRate of 10 work, but a playbackRate of 3 fail?
> 
> If this is meant to test a dynamic update of playbackRate while moving, then
> it should be a separate test. In general we are trying to keep each test as
> small and isolated as possible.

I will create the separate test.
 
> >+    previousAnimationCurrentTime = animation.currentTime;
> >+    previousTimelineCurrentTime = animation.timeline.currentTime;
> >+    previousMarginLeft = parseFloat(getComputedStyle(div).marginLeft);
> >+    return waitForFrame();
> >+  })).then(t.step_func(function() {
> >+    assert_playbackrate(div,
> >+                        previousAnimationCurrentTime,
> >+                        previousTimelineCurrentTime,
> >+                        "animation.currentTime should be 3 times faster than timeline now.");
> >+    assert_animation_position(div,
> >+                              previousAnimationCurrentTime,
> >+                              previousMarginLeft);
> >+
> >+    animation.playbackRate = 21;
> 
> Again, why would 21 be any different?

Will remove.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > >+function assert_animation_position(target,
> > >+                                   previousAnimationCurrentTime,
> > >+                                   previousMarginLeft,
> > >+                                   description) {
> > >+  var accuracy = 0.1; /* what is a proper value here? */
> > 
> > I don't think we need this method. We should just test the result of
> > playbackRate on the currentTime (not on margin-left). 
> 
> I think this sort of tests are necessary to avoid bugs like bug 1175751.
> Actually animation.currentTime is correctly advanced in case of CSS
> transform(the case of bug 1175751).
> 
> But I will remove this for now and reconsider it in bug 1175751.

Thanks. We should work out what the cause is for bug 1175751 and write a specific test for that. If it's something that could affect other implementations then we can add it here, otherwise we can put it in the 'mozilla' directory.

> > >+    // Psuedo busyness
> > >+    var timeAtStart = window.performance.now();
> > >+    while (window.performance.now() - timeAtStart < 100) {
> > >+    }
> > >+    return waitForFrame();
> > >+  })).then(t.step_func(function() {
> > >+    assert_playbackrate(div,
> > >+                        previousAnimationCurrentTime,
> > >+                        previousTimelineCurrentTime,
> > >+                        "animation.currentTime should be 10 times faster than timeline even if main thread is busy.");
> > 
> > Why would the result differ if the main thread was busy?
> 
> Actually I do not know.
> My intention here is that this is some kind of black-box test.
> I imagined that animation.currentTime should be advanced correctly even if
> some frames are skipped because of heavy load in main thread. But OK I will
> remove this.

I'm afraid I don't really understand how this might fail. If the main thread is busy the next frame will be delayed but not skipped. If somehow the currentTime and computed style are getting out of sync then it's probably not related to just playbackRate?
(In reply to Brian Birtles (:birtles) from comment #23)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > > >+function assert_animation_position(target,
> > > >+                                   previousAnimationCurrentTime,
> > > >+                                   previousMarginLeft,
> > > >+                                   description) {
> > > >+  var accuracy = 0.1; /* what is a proper value here? */
> > > 
> > > I don't think we need this method. We should just test the result of
> > > playbackRate on the currentTime (not on margin-left). 
> > 
> > I think this sort of tests are necessary to avoid bugs like bug 1175751.
> > Actually animation.currentTime is correctly advanced in case of CSS
> > transform(the case of bug 1175751).
> > 
> > But I will remove this for now and reconsider it in bug 1175751.
> 
> Thanks. We should work out what the cause is for bug 1175751 and write a
> specific test for that. If it's something that could affect other
> implementations then we can add it here, otherwise we can put it in the
> 'mozilla' directory.

Thanks for the explanation. I will do it next time when I face such situations.

> > > Why would the result differ if the main thread was busy?
> > 
> > Actually I do not know.
> > My intention here is that this is some kind of black-box test.
> > I imagined that animation.currentTime should be advanced correctly even if
> > some frames are skipped because of heavy load in main thread. But OK I will
> > remove this.
> 
> I'm afraid I don't really understand how this might fail. If the main thread
> is busy the next frame will be delayed but not skipped. If somehow the
> currentTime and computed style are getting out of sync then it's probably
> not related to just playbackRate?

Ah, right. The next frame is not skipped. I imagined the frame number I had been trying to implement in VSync drivers (bug 1167487).  In that case the frame is skipped but from the perspective of main thread it's just delayed. Sorry for the confusion.
Attached patch tests v3 (obsolete) — Splinter Review
Addressed all comments.

* some style fix (indentation, use single quote instead of double one).
* factor out create_animation function.
Attachment #8629188 - Attachment is obsolete: true
Attachment #8629242 - Flags: review?(bbirtles)
Attachment #8629242 - Attachment is patch: true
Comment on attachment 8629242 [details] [diff] [review]
tests v3

>+function create_animation(test) {
>+  var div = addDiv(test, {'style': 'animation: anim 10s'});
>+  var animation = div.getAnimations()[0];
>+
>+  animation.playbackRate = 10;
>+
>+  return animation;
>+}

I don't think we should factor this out as a separate function. I think it makes the tests harder to read (since you have to remember that 'create_animation' *also* updates the playbackRate to 10 and creates a new div).

If you want to keep this function then please remove the line where we set the playbackRate and rename 'createAnimation' (we've been using camelCase elsewhere in these tests--it's only the testharness.js methods that use underscores_to_separate_words).

>+async_test(function(t) {
>+  var animation = create_animation(t);
>+
>+  var previousTimelineCurrentTime;
>+  var previousAnimationCurrentTime;
>+
>+  animation.ready.then(function() {
>+    previousAnimationCurrentTime = animation.currentTime;
>+    previousTimelineCurrentTime = animation.timeline.currentTime;
>+
>+    animation.playbackRate = 1;
>+
>+    return waitForFrame();
>+  }).then(t.step_func_done(function() {
>+    assert_equals(animation.playbackRate, 1,
>+      'double check that animation.playbackRate should surely be 1.');

I'm not sure we need this but if we do, perhaps 'sanity check: animation.playbackRate is still 1' is a more clear description?

The rest looks great. Thanks for doing this.
Attachment #8629242 - Flags: review?(bbirtles) → review+
Attached file tests v4 (obsolete) —
Updated.

Pushed a try now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9b3fc5280f

I will add checkin-needed once the try results are good.

(In reply to Brian Birtles (:birtles) from comment #26)
> rename 'createAnimation' (we've been using camelCase
> elsewhere in these tests--it's only the testharness.js methods that use
> underscores_to_separate_words).

Wow! I did not notice it. I was wondering why there are camelCase and underscore_case.
Anyway the function was removed in this patch.

Thanks!
Attachment #8629242 - Attachment is obsolete: true
Attachment #8629251 - Flags: review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> Pushed a try now:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9b3fc5280f
> 
> I will add checkin-needed once the try results are good.

Thanks! You can drop the leave-open too when you do.
Attached patch Disable tests on android (obsolete) — Splinter Review
Unfortunately the tests failed on android emulator.

An example:

155 INFO TEST-UNEXPECTED-FAIL | dom/animation/test/css-animations/test_animation-playbackrate.html | Test the effect of setting playbackRate on currentTime - Test the effect of setting playbackRate on currentTime: assert_approx_equals: animation.currentTime should be 10 times faster than timeline. expected 8999.999639999996 +/- 0.001 but got 1786.0272100000002

I am suspecting this failure is specific to android. (Re-trying 10 times on b2g was no failures of this test, one failure of another test.) So I'd propose to disable this test on android for now. And I will investigate the failure reason in a new bug.
Attachment #8629716 - Flags: review?(bbirtles)
Comment on attachment 8629716 [details] [diff] [review]
Disable tests on android

>diff --git a/dom/animation/test/mochitest.ini b/dom/animation/test/mochitest.ini
>--- a/dom/animation/test/mochitest.ini
>+++ b/dom/animation/test/mochitest.ini
>@@ -14,16 +14,17 @@ support-files = css-animations/file_anim
> support-files = css-animations/file_animation-finished.html
> [css-animations/test_animation-pausing.html]
> support-files = css-animations/file_animation-pausing.html
> [css-animations/test_animation-play.html]
> support-files = css-animations/file_animation-play.html
> [css-animations/test_animation-playbackrate.html]
> support-files = css-animations/file_animation-playbackrate.html
> [css-animations/test_animation-playstate.html]
>+skip-if = os == 'android'
> support-files = css-animations/file_animation-playstate.html

I think you meant to put this under 'playbackrate', not 'playstate'.


(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
...
> 155 INFO TEST-UNEXPECTED-FAIL |
> dom/animation/test/css-animations/test_animation-playbackrate.html | Test
> the effect of setting playbackRate on currentTime - Test the effect of
> setting playbackRate on currentTime: assert_approx_equals:
> animation.currentTime should be 10 times faster than timeline. expected
> 8999.999639999996 +/- 0.001 but got 1786.0272100000002
> 
> I am suspecting this failure is specific to android. (Re-trying 10 times on
> b2g was no failures of this test, one failure of another test.) So I'd
> propose to disable this test on android for now. And I will investigate the
> failure reason in a new bug.

Seeing as this code has already landed without tests, I think it's ok to land these tests with Android turned off. However, it's a pretty significant failure so we should investigate it soon. Please leave a comment here with the bug number for investigating this.

r=me with the 'skip-if' in the right place

Thanks again for looking into this.
Attachment #8629716 - Flags: review?(bbirtles) → review+
Depends on: 1180559
Attached patch Disable tests on android (obsolete) — Splinter Review
Fix the horrible mistake.
Attachment #8629716 - Attachment is obsolete: true
Attachment #8629782 - Flags: review+
Attachment #8629251 - Flags: checkin+
Attachment #8574985 - Flags: checkin+
Attachment #8579965 - Flags: checkin+
Comment on attachment 8629251 [details]
tests v4

I am sorry for wrong flag here.
Attachment #8629251 - Flags: checkin+
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> ...
> > 155 INFO TEST-UNEXPECTED-FAIL |
> > dom/animation/test/css-animations/test_animation-playbackrate.html | Test
> > the effect of setting playbackRate on currentTime - Test the effect of
> > setting playbackRate on currentTime: assert_approx_equals:
> > animation.currentTime should be 10 times faster than timeline. expected
> > 8999.999639999996 +/- 0.001 but got 1786.0272100000002
> > 
> > I am suspecting this failure is specific to android. (Re-trying 10 times on
> > b2g was no failures of this test, one failure of another test.) So I'd
> > propose to disable this test on android for now. And I will investigate the
> > failure reason in a new bug.
> 
> Seeing as this code has already landed without tests, I think it's ok to
> land these tests with Android turned off. However, it's a pretty significant
> failure so we should investigate it soon. Please leave a comment here with
> the bug number for investigating this.

Filed bug 1180559.
Hi, this failed to apply:
patching file dom/animation/test/mochitest.ini
Hunk #1 FAILED at 11
1 out of 1 hunks FAILED -- saving rejects to file dom/animation/test/mochitest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh skip_playbackrate_on_android.patch

could you take a look ? thanks!
Flags: needinfo?(jwatt)
Keywords: checkin-needed
That's my fault. I meant both of files are needed to be checked-in.

Here is a unified patch. Please check this in. I am sorry for confusion.
Attachment #8629251 - Attachment is obsolete: true
Attachment #8629782 - Attachment is obsolete: true
Flags: needinfo?(jwatt)
Attachment #8629817 - Flags: review+
Attachment #8629817 - Attachment is patch: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> Created attachment 8629817 [details] [diff] [review]
> Tests v5 with disabling the tests on android
> 
> That's my fault. I meant both of files are needed to be checked-in.
> 
> Here is a unified patch. Please check this in. I am sorry for confusion.

np :)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11384529&repo=mozilla-inbound
Flags: needinfo?(hiikezoe)
(In reply to Carsten Book [:Tomcat] from comment #38)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=11384529&repo=mozilla-
> inbound

This failure is the same as the one on android emulator. I will investigate it.
Thanks!
Flags: needinfo?(hiikezoe)
Blocks: 1150808
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2891b5a0d90f

As I wrote in bug 1180559 comment #6, 100s and 2x playbackRate bring good results on the try.
Attachment #8629817 - Attachment is obsolete: true
Attachment #8630273 - Flags: review?(bbirtles)
Comment on attachment 8630273 [details] [diff] [review]
Test v6 longer duration and smaller plabackRate

I posted Old patch. sorry.
Attachment #8630273 - Flags: review?(bbirtles)
Attachment #8630273 - Attachment is patch: true
Comment on attachment 8630273 [details] [diff] [review]
Test v6 longer duration and smaller plabackRate

>diff --git a/dom/animation/test/mochitest.ini b/dom/animation/test/mochitest.ini
>--- a/dom/animation/test/mochitest.ini
>+++ b/dom/animation/test/mochitest.ini
>@@ -11,16 +11,19 @@ support-files = css-animations/file_anim
> [css-animations/test_animation-finish.html]
> support-files = css-animations/file_animation-finish.html
> [css-animations/test_animation-finished.html]
> support-files = css-animations/file_animation-finished.html
> [css-animations/test_animation-pausing.html]
> support-files = css-animations/file_animation-pausing.html
> [css-animations/test_animation-play.html]
> support-files = css-animations/file_animation-play.html
>+[css-animations/test_animation-playbackrate.html]
>+skip-if = os == 'android'
>+support-files = css-animations/file_animation-playbackrate.html

Can we drop the "skip-if = os == 'android'" part now?
Attachment #8630273 - Flags: review+
Attachment #8630320 - Attachment is patch: true
Attachment #8630320 - Flags: review?(bbirtles) → review+
https://hg.mozilla.org/mozilla-central/rev/e3dfa2011601
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Thanks, hiro!
Duplicate of this bug: 1180559
Depends on: 1182393
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.