Implement Animation.updatePlaybackRate()

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(14 attachments)

59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
hiro
: review+
Details
59 bytes, text/x-review-board-request
daisuke
: review+
Details
As part of getting the ready/finished Promises to ship we need to resolve the fact that the setPlaybackRate in Firefox is synchronous but async in Chrome. For this the spec has been updated to add a separate async method: updatePlaybackRate().

Spec changes:

  1. https://github.com/w3c/csswg-drafts/commit/5af5e276badf4df0271bcfa0b8e7837fff24133a
  2. https://github.com/w3c/csswg-drafts/commit/673f6fc1269829743c707c53dcb04092f958de35

(2) partially undoes (1) so it's probably easier to look at the combined diff:

  https://github.com/w3c/csswg-drafts/compare/653dd600245d2ff3f5f4b91a00194f664a451eda...673f6fc1269829743c707c53dcb04092f958de35#diff-4c9f5c055fb219a7fcad23a9a7a80b64

Or, better still this diff I have prepared here:

  https://gist.github.com/birtles/d147eb2e0e2d4d37fadf217abd709411
I have a rough implementation of this at: https://hg.mozilla.org/try/rev/16c1ce8d6446f6601a840c3fef65e1ee0ae6a600

Which I have nearly finished splitting up here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37eaa15a99dac0ebf79a6c59eda9420e3c2f36e7

(Just in case I don't get back to this soon.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8950485 [details]
Bug 1436659 - Add Animation.updatePlaybackRate WebIDL definition;

https://reviewboard.mozilla.org/r/219758/#review225460

r=me, assuming the spec defines clearly how this interacts with the playbackRate attribute.
Attachment #8950485 - Flags: review?(bzbarsky) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8950490 [details]
Bug 1436659 - Use updatePlaybackRate in DevTools;

https://reviewboard.mozilla.org/r/219768/#review225464

Thank you very much!

::: devtools/server/actors/animation.js:877
(Diff revision 1)
> +    let readyPromises = [];
>      for (let player of players) {
> -      player.setPlaybackRate(rate);
> +      readyPromises.push(player.setPlaybackRate(rate));
>      }
> +    return promise.all(readyPromises);

We can use map in here.

e.g.
`return promise.all(players.map(player => player.setPlaybackRate(rate)));`
Attachment #8950490 - Flags: review?(dakatsuka) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> Comment on attachment 8950485 [details]
> Bug 1436659 - Add Animation.updatePlaybackRate WebIDL definition;
> 
> https://reviewboard.mozilla.org/r/219758/#review225460
> 
> r=me, assuming the spec defines clearly how this interacts with the
> playbackRate attribute.

Yes, it does. The tests for which are in this patch series (specifically the "Add tests for Animation.updatePlaybackRate" patch).

Comment 20

a year ago
mozreview-review
Comment on attachment 8950477 [details]
Bug 1436659 - Rename tests in timing-model/animations to match spec section titles;

https://reviewboard.mozilla.org/r/219742/#review225476
Attachment #8950477 - Flags: review?(hikezoe) → review+

Comment 21

a year ago
mozreview-review
Comment on attachment 8950478 [details]
Bug 1436659 - Use async/await in timing-model/animations tests;

https://reviewboard.mozilla.org/r/219744/#review225484

::: testing/web-platform/tests/web-animations/timing-model/animations/canceling-an-animation.html:49
(Diff revision 1)
>     + ' canceled');
>  
> -promise_test(t => {
> +promise_test(async t => {
>    const animation = createDiv(t).animate(null);
>    animation.cancel();
> -  return animation.ready.then(p => {
> +  const p = await animation.ready;

I'd like to avoid single charactor variable name as possible.  In this case, the returned value is not a Promise?  It's an animation object which got involved with the Promise?

I don't know what the value get involved with Promise is called, but it should be animationInResolvedPromise or something.  Though I am fine with the change in a subsequent patch.

::: testing/web-platform/tests/web-animations/timing-model/animations/finishing-an-animation.html:22
(Diff revision 1)
>    let readyResolved = false;
>  
>    animation.finish();
>    animation.ready.then(() => { readyResolved = true; });
>  
> -  return animation.finished.then(p => {
> +  const p = await animation.finished;

Likewise here.

::: testing/web-platform/tests/web-animations/timing-model/animations/pausing-an-animation.html:19
(Diff revision 1)
> -promise_test(t => {
> +promise_test(async t => {
>    const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
>    const promise = animation.ready;
>    animation.pause();
> -  return promise.then(p => {
> +
> +  const p = await promise;

Likewise.

::: testing/web-platform/tests/web-animations/timing-model/animations/playing-an-animation.html:51
(Diff revision 1)
>     + ' pending');
>  
> -promise_test(t => {
> +promise_test(async t => {
>    const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
>    const promise = animation.ready;
> -  return promise.then(p => {
> +  const p = await promise;

Likewise.
Attachment #8950478 - Flags: review?(hikezoe) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8950479 [details]
Bug 1436659 - Move playback rate test to timing-model;

https://reviewboard.mozilla.org/r/219746/#review225496
Attachment #8950479 - Flags: review?(hikezoe) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8950481 [details]
Bug 1436659 - Move finishing tests to timing-model;

https://reviewboard.mozilla.org/r/219750/#review225516
Attachment #8950481 - Flags: review?(hikezoe) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8950482 [details]
Bug 1436659 - Tidy up tests for "finishing an animation";

https://reviewboard.mozilla.org/r/219752/#review225524

r=me if my concern about the description is irrelevant.

::: testing/web-platform/tests/web-animations/timing-model/animations/finishing-an-animation.html:22
(Diff revision 1)
>    animation.playbackRate = 0;
>  
>    assert_throws({name: 'InvalidStateError'}, () => {
>      animation.finish();
>    });
> -}, 'Test exceptions when finishing non-running animation');
> +}, 'Finishing an animation with a zero playback rate should throw');

I thought we've decided to use BDD style description in tests somewhere.  Though I couldn't find it now.
If so, we should avoid using 'should' here, I think.
Attachment #8950482 - Flags: review?(hikezoe) → review+

Comment 25

a year ago
mozreview-review
Comment on attachment 8950483 [details]
Bug 1436659 - Further divide up finishing tests;

https://reviewboard.mozilla.org/r/219754/#review225536

::: testing/web-platform/tests/web-animations/animation-model/combining-effects/applying-the-composited-result.html:26
(Diff revision 1)
> +
> +  animation.finish();
> +
> +  const marginLeft = parseFloat(getComputedStyle(div).marginLeft);
> +  assert_equals(marginLeft, 10, 'The computed style should be reset');
> +}, 'Finishing an animation that does not fill forwards should cause'

I am also concerned 'should' in this description.
Attachment #8950483 - Flags: review?(hikezoe) → review+

Comment 26

a year ago
mozreview-review
Comment on attachment 8950487 [details]
Bug 1436659 - Add Animation::GetCurrentTimeForHoldTime helper;

https://reviewboard.mozilla.org/r/219762/#review225542
Attachment #8950487 - Flags: review?(hikezoe) → review+

Comment 27

a year ago
mozreview-review
Comment on attachment 8950486 [details]
Bug 1436659 - Factor out static time calculation methods on Animation;

https://reviewboard.mozilla.org/r/219760/#review225546

Someday we might want to separate these two functions into a new header file to avoid compiling AnimationHelper.cpp on each time we touch Animation.h.
Attachment #8950486 - Flags: review?(hikezoe) → review+
I couldn't finish all reviewing (I haven't yet read the new spec).  I will go on reviewing tomorrow morning.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> Comment on attachment 8950478 [details]
> Bug 1436659 - Use async/await in timing-model/animations tests;
> 
> https://reviewboard.mozilla.org/r/219744/#review225484
> 
> :::
> testing/web-platform/tests/web-animations/timing-model/animations/canceling-
> an-animation.html:49
> (Diff revision 1)
> >     + ' canceled');
> >  
> > -promise_test(t => {
> > +promise_test(async t => {
> >    const animation = createDiv(t).animate(null);
> >    animation.cancel();
> > -  return animation.ready.then(p => {
> > +  const p = await animation.ready;
> 
> I'd like to avoid single charactor variable name as possible.  In this case,
> the returned value is not a Promise?  It's an animation object which got
> involved with the Promise?

Agreed, but this patch is--as far as possible--simply a mechanical translation from one format to another so I didn't think it was appropriate to rename variables at the same time.

Comment 30

a year ago
mozreview-review
Comment on attachment 8950488 [details]
Bug 1436659 - Implement pending playback rate mechanism;

https://reviewboard.mozilla.org/r/219764/#review225814

I haven't finished reviewing yet (I still need to review Animation::GetCurrentOrPendingStartTime), but I post some comments so far.

::: dom/animation/Animation.cpp:276
(Diff revision 1)
>    if (timelineTime.IsNull() && !aNewStartTime.IsNull()) {
>      mHoldTime.SetNull();
>    }
>  
>    Nullable<TimeDuration> previousCurrentTime = GetCurrentTime();
> +  ApplyPendingPlaybackRate();

Can we move this 1-line below?  I.e. after 'mStartTime = aNewStartTime', it matches the spec.

::: dom/animation/Animation.cpp:342
(Diff revision 1)
>      mStartTime.SetNull();
>  
> +    ApplyPendingPlaybackRate();

I did suggest in the above comment, but after more read the code, we should reverse the order here.

::: dom/animation/Animation.cpp:620
(Diff revision 1)
>    if (!mTimeline || mTimeline->GetCurrentTime().IsNull()) {
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return;
>    }
>  
> -  if (mPlaybackRate == 0.0) {
> +  double effectivePlaybackRate = mPendingPlaybackRate.valueOr(mPlaybackRate);

CurrentOrPendingPlaybackRate() instead?

::: dom/animation/Animation.cpp:1292
(Diff revision 1)
> -  // we should use. In all other cases, we resolve it from the ready time.
> -  if (mStartTime.IsNull()) {
> +  bool hadPendingPlaybackRate = mPendingPlaybackRate.isSome();
> +
> +  if (!mHoldTime.IsNull()) {
> +    // The hold time is set, so we don't need any special handling to preserve
> +    // the current time.
> +    ApplyPendingPlaybackRate();

Oh, now I realized that we need to apply pending playback rate prior to calculating starttime. So, we should align applying pending playback rate prior to starttime all over the places?

::: dom/animation/Animation.cpp:1335
(Diff revision 1)
> +  ApplyPendingPlaybackRate();
>    mStartTime.SetNull();

Yeah, I think this order makes much sense.

::: dom/animation/Animation.cpp:1482
(Diff revision 1)
>  {
>    if (mPendingState == PendingState::NotPending) {
>      return;
>    }
>  
> +  ApplyPendingPlaybackRate();

I'd like to match the process order to the spec as well.  CancelPendingTasks() and then ApplyPendingPlaybackRate().

Comment 31

a year ago
mozreview-review
Comment on attachment 8950488 [details]
Bug 1436659 - Implement pending playback rate mechanism;

https://reviewboard.mozilla.org/r/219764/#review225852

The change for GetCurrentOrPendingStartTime looks reasonable to me.
I am wondering whether we have already automation test cases that fail without this change, or do you have an example that results choppy animation?  I am curious how it looks like, I guess it's changing playback rate against an animation on the compositor in an rAF callback?  It is closely related to the next patch?
Attachment #8950488 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> Comment on attachment 8950488 [details]
> Bug 1436659 - Implement pending playback rate mechanism;
> 
> https://reviewboard.mozilla.org/r/219764/#review225852
> 
> The change for GetCurrentOrPendingStartTime looks reasonable to me.
> I am wondering whether we have already automation test cases that fail
> without this change, or do you have an example that results choppy
> animation?  I am curious how it looks like, I guess it's changing playback
> rate against an animation on the compositor in an rAF callback?  It is
> closely related to the next patch?

Yes, this was just from manual inspection. It would flicker without this change. I'm not sure if it's possible to capture that in an automated test. I'll give it a try, but I'm not sure it's worth more than a few hours trying.

Comment 33

a year ago
mozreview-review
Comment on attachment 8950480 [details]
Bug 1436659 - Simplify playback rate setting test;

https://reviewboard.mozilla.org/r/219748/#review225860

Looks fine but still it seems to me that the first test case is not suitable for 'setting playback rate' tests.  It seems for the section of 'https://drafts.csswg.org/web-animations-1/#the-current-time-of-an-animation'.

Also I am still concerned about the description message.
Attachment #8950480 - Flags: review?(hikezoe) → review+
(In reply to Brian Birtles (:birtles) from comment #32)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> > Comment on attachment 8950488 [details]
> > Bug 1436659 - Implement pending playback rate mechanism;
> > 
> > https://reviewboard.mozilla.org/r/219764/#review225852
> > 
> > The change for GetCurrentOrPendingStartTime looks reasonable to me.
> > I am wondering whether we have already automation test cases that fail
> > without this change, or do you have an example that results choppy
> > animation?  I am curious how it looks like, I guess it's changing playback
> > rate against an animation on the compositor in an rAF callback?  It is
> > closely related to the next patch?
> 
> Yes, this was just from manual inspection. It would flicker without this
> change. I'm not sure if it's possible to capture that in an automated test.
> I'll give it a try, but I'm not sure it's worth more than a few hours trying.

No problem,  I just wanted to see it if you already have/know.

Comment 35

a year ago
mozreview-review
Comment on attachment 8950484 [details]
Bug 1436659 - Add tests for Animation.updatePlaybackRate;

https://reviewboard.mozilla.org/r/219756/#review225804

I am still trying to understand the test case in playing-an-animation.html.  But other parts look fine.

::: testing/web-platform/tests/web-animations/timing-model/animations/finishing-an-animation.html:233
(Diff revision 1)
>  }, 'A pending ready promise should be resolved and not replaced when the'
>     + ' animation is finished');
>  
> +promise_test(async t => {
> +  const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
> +  animation.currentTime = 50 * MS_PER_SEC;

I am wondering why we need to set current time here (and other test cases in this file and setting-the-playback-rate-of-an-animation.html).  It is for making sure there is no pending playback rate before proceeding test?

::: testing/web-platform/tests/web-animations/timing-model/animations/pausing-an-animation.html:27
(Diff revision 1)
> +promise_test(async t => {
> +  const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
> +  // Let animation start roughly half-way through
> +  animation.currentTime = 50 * MS_PER_SEC;
> +  await animation.ready;
> +
> +  // Go pause-pending and also set a pending playback rate
> +  animation.pause();
> +  animation.updatePlaybackRate(0.5);
> +
> +  await animation.ready;
> +  // If the current time was updated using the new playback rate it will jump
> +  // back to 25s but if we correctly used the old playback rate the current time
> +  // will be >50s.
> +  assert_greater_than(animation.currentTime, 50 * MS_PER_SEC);
> +}, 'A pause-pending animation maintains the current time when applying a'
> +   + ' pending playback rate');

This test looks the same as the one in seamlessly-updating-the-playback-rate-of-an-animation.html.  Do we need a test case that pause() applies pending palyback rate instead?

::: testing/web-platform/tests/web-animations/timing-model/animations/seamlessly-updating-the-playback-rate-of-an-animation.html:28
(Diff revision 1)
> +  // motion!) we can't assert that the current time == 50s but we can check
> +  // that the current time is NOT re-calculated by simply substituting in the
> +  // new playback rate (i.e. without adjusting the start time). If that were
> +  // the case the currentTime would jump to 25s. So we just test the currentTime
> +  // hasn't gone backwards.
> +  assert_true(animation.currentTime >= 50 * MS_PER_SEC,

We usually use assert_greater_than_equal() for this kind of checks, but it seem OK for this particular case.

::: testing/web-platform/tests/web-animations/timing-model/animations/seamlessly-updating-the-playback-rate-of-an-animation.html:37
(Diff revision 1)
> +  animation.updatePlaybackRate(2);
> +  await animation.ready;
> +  // Likewise, we test here that the current time does not jump to 100s as it
> +  // would if we naively applied a playbackRate of 2 without adjusting the
> +  // startTime.
> +  assert_true(animation.currentTime < 100 * MS_PER_SEC,

Initially I worried about the timer precision issue here, but the issue should never happen here.

::: testing/web-platform/tests/web-animations/timing-model/animations/seamlessly-updating-the-playback-rate-of-an-animation.html:72
(Diff revision 1)
> +promise_test(async t => {
> +  const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
> +  animation.currentTime = 50 * MS_PER_SEC;
> +  await animation.ready;
> +
> +  animation.pause();
> +  animation.updatePlaybackRate(0.5);
> +
> +  assert_true(animation.currentTime >= 50 * MS_PER_SEC);
> +}, 'Updating the playback rate on a pause-pending animation should maintain'
> +   + ' the current time');

This is the one what I menioned for the test case in pausing-an-animation.html.

::: testing/web-platform/tests/web-animations/timing-model/animations/setting-the-target-effect-of-an-animation.html:103
(Diff revision 1)
> +  anim.updatePlaybackRate(2);
> +  assert_equals(anim.playbackRate, 1);
> +
> +  anim.effect = null;
> +  assert_equals(anim.playbackRate, 2);
> +}, 'Setting the target effect causes a pending playback rate to be applied');

As per the spec, applying pending playback rate happens only when setting null effect, right?  We should mention it in this description.

Comment 36

a year ago
mozreview-review
Comment on attachment 8950489 [details]
Bug 1436659 - Support pending playback rates on compositor animations;

https://reviewboard.mozilla.org/r/219766/#review225890

Nulling out start time looks a bit tricky to me, but after discussion on IRC, I have no great idea to make pending playback rate case less tricker.
Attachment #8950489 - Flags: review?(hikezoe) → review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8950484 [details]
Bug 1436659 - Add tests for Animation.updatePlaybackRate;

https://reviewboard.mozilla.org/r/219756/#review225892

I think now I understand the test case in playing-an-animation.html. (I found a similar test case for setting playbackRate in the same test file). :)

Is it worth testing for zero pending playback rate or positive one?
Attachment #8950484 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #35)
> testing/web-platform/tests/web-animations/timing-model/animations/finishing-
> an-animation.html:233
> (Diff revision 1)
> >  }, 'A pending ready promise should be resolved and not replaced when the'
> >     + ' animation is finished');
> >  
> > +promise_test(async t => {
> > +  const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
> > +  animation.currentTime = 50 * MS_PER_SEC;
> 
> I am wondering why we need to set current time here (and other test cases in
> this file and setting-the-playback-rate-of-an-animation.html).  It is for
> making sure there is no pending playback rate before proceeding test?

Yes, you're right. I'm not sure why it's in these tests. I probably just copy and pasted it from somewhere. I've dropped it now.

> testing/web-platform/tests/web-animations/timing-model/animations/pausing-an-
> animation.html:27
> (Diff revision 1)
> > +promise_test(async t => {
> > +  const animation = createDiv(t).animate(null, 100 * MS_PER_SEC);
> > +  // Let animation start roughly half-way through
> > +  animation.currentTime = 50 * MS_PER_SEC;
> > +  await animation.ready;
> > +
> > +  // Go pause-pending and also set a pending playback rate
> > +  animation.pause();
> > +  animation.updatePlaybackRate(0.5);
> > +
> > +  await animation.ready;
> > +  // If the current time was updated using the new playback rate it will jump
> > +  // back to 25s but if we correctly used the old playback rate the current time
> > +  // will be >50s.
> > +  assert_greater_than(animation.currentTime, 50 * MS_PER_SEC);
> > +}, 'A pause-pending animation maintains the current time when applying a'
> > +   + ' pending playback rate');
> 
> This test looks the same as the one in
> seamlessly-updating-the-playback-rate-of-an-animation.html.  Do we need a
> test case that pause() applies pending palyback rate instead?

They're slightly different. This one, in pausing-an-animation, tests the behavior of the task that runs when a pause-pending animation completes (i.e. the bit under the "pause an animation" procedure) where as the one in seamlessly-updating-the-playback-rate-of-an-animation tests the behavior before that task runs (i.e. the logic under the "seamlessly update the playback rate" procedure). They are very similar, however.

> testing/web-platform/tests/web-animations/timing-model/animations/seamlessly-
> updating-the-playback-rate-of-an-animation.html:28
> (Diff revision 1)
> > +  // motion!) we can't assert that the current time == 50s but we can check
> > +  // that the current time is NOT re-calculated by simply substituting in the
> > +  // new playback rate (i.e. without adjusting the start time). If that were
> > +  // the case the currentTime would jump to 25s. So we just test the currentTime
> > +  // hasn't gone backwards.
> > +  assert_true(animation.currentTime >= 50 * MS_PER_SEC,
> 
> We usually use assert_greater_than_equal() for this kind of checks, but it
> seem OK for this particular case.

Fixed.

> :::
> testing/web-platform/tests/web-animations/timing-model/animations/setting-
> the-target-effect-of-an-animation.html:103
> (Diff revision 1)
> > +  anim.updatePlaybackRate(2);
> > +  assert_equals(anim.playbackRate, 1);
> > +
> > +  anim.effect = null;
> > +  assert_equals(anim.playbackRate, 2);
> > +}, 'Setting the target effect causes a pending playback rate to be applied');
> 
> As per the spec, applying pending playback rate happens only when setting
> null effect, right?  We should mention it in this description.

Yes, good point.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
> Comment on attachment 8950484 [details]
> Bug 1436659 - Add tests for Animation.updatePlaybackRate;
> 
> https://reviewboard.mozilla.org/r/219756/#review225892
> 
> I think now I understand the test case in playing-an-animation.html. (I
> found a similar test case for setting playbackRate in the same test file). :)
> 
> Is it worth testing for zero pending playback rate or positive one?

The purpose of that test is just to see that we use the *pending* playback rate, not to test the calculation of endpoints used by auto-rewind. For general auto-rewind behavior regardless of pending playback rates we have a test for a positive playback rate in this file but not a zero one it seems. We should add one but that sounds like a different bug to me.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 54

a year ago
mozreview-review-reply
Comment on attachment 8950488 [details]
Bug 1436659 - Implement pending playback rate mechanism;

https://reviewboard.mozilla.org/r/219764/#review225814

> I did suggest in the above comment, but after more read the code, we should reverse the order here.

I'm not sure what the above comment refers to here but I'd rather match the spec here.

> Oh, now I realized that we need to apply pending playback rate prior to calculating starttime. So, we should align applying pending playback rate prior to starttime all over the places?

Again, I'd rather match the spec here.
(In reply to Brian Birtles (:birtles) from comment #54)
> Comment on attachment 8950488 [details]
> Bug 1436659 - Implement pending playback rate mechanism;
> 
> https://reviewboard.mozilla.org/r/219764/#review225814
> 
> > I did suggest in the above comment, but after more read the code, we should reverse the order here.
> 
> I'm not sure what the above comment refers to here but I'd rather match the
> spec here.
> 
> > Oh, now I realized that we need to apply pending playback rate prior to calculating starttime. So, we should align applying pending playback rate prior to starttime all over the places?
> 
> Again, I'd rather match the spec here.

What I wanted to suggest is to apply pending playback rate first both in the spec and this implementation. Say,

in the spec in 'Pausing an animation' <https://drafts.csswg.org/web-animations-1/#pausing-an-animation-section>

 6. Apply any pending playback rate on animation.

 7. Make animation’s start time unresolved.
 
Whereas in the spec 'Setting the current time of an animation' in <https://drafts.csswg.org/web-animations-1/#setting-the-current-time-of-an-animation>

 2. Make animation’s start time unresolved.

 3. Apply any pending playback rate to animation.

It's bit confusing.
Comment hidden (mozreview-request)

Comment 57

a year ago
mozreview-review
Comment on attachment 8950490 [details]
Bug 1436659 - Use updatePlaybackRate in DevTools;

https://reviewboard.mozilla.org/r/219768/#review225918


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/animation.js:879
(Diff revision 2)
>     * @param {Number} rate The new rate.
>     */
>    setPlaybackRates: function (players, rate) {
> -    for (let player of players) {
> -      player.setPlaybackRate(rate);
> -    }
> +    return promise.all(
> +      players.map(player => player.setPlaybackRate(rate)
> +    );

Error: Parsing error: unexpected token ; [eslint: None]
(Hmm, it's already fixed in the subsequent review update--I guess the review bot doesn't run too frequently.)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> (In reply to Brian Birtles (:birtles) from comment #54)
> > Comment on attachment 8950488 [details]
> > Bug 1436659 - Implement pending playback rate mechanism;
> > 
> > https://reviewboard.mozilla.org/r/219764/#review225814
> > 
> > > I did suggest in the above comment, but after more read the code, we should reverse the order here.
> > 
> > I'm not sure what the above comment refers to here but I'd rather match the
> > spec here.
> > 
> > > Oh, now I realized that we need to apply pending playback rate prior to calculating starttime. So, we should align applying pending playback rate prior to starttime all over the places?
> > 
> > Again, I'd rather match the spec here.
> 
> What I wanted to suggest is to apply pending playback rate first both in the
> spec and this implementation. Say,
> 
> in the spec in 'Pausing an animation'
> <https://drafts.csswg.org/web-animations-1/#pausing-an-animation-section>
> 
>  6. Apply any pending playback rate on animation.
> 
>  7. Make animation’s start time unresolved.
>  
> Whereas in the spec 'Setting the current time of an animation' in
> <https://drafts.csswg.org/web-animations-1/#setting-the-current-time-of-an-
> animation>
> 
>  2. Make animation’s start time unresolved.
> 
>  3. Apply any pending playback rate to animation.
> 
> It's bit confusing.

Oh, I see. I looked into this but I'm afraid we can't have a completely consistent order. In UpdatePlaybackRate we need to update the start time before applying the pending playback rate since we need to use the old playback rate when calculating the unconstrained current time.

I can switch the order in the "set start time" and "set current time" procedures and in the spec though.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Thanks!

Comment 66

a year ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9777027b7ad
Rename tests in timing-model/animations to match spec section titles; r=hiro
https://hg.mozilla.org/integration/autoland/rev/c2932cd4839f
Use async/await in timing-model/animations tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/071666b6c7e9
Move playback rate test to timing-model; r=hiro
https://hg.mozilla.org/integration/autoland/rev/737ff1676ff0
Simplify playback rate setting test; r=hiro
https://hg.mozilla.org/integration/autoland/rev/467dd218d3d3
Move finishing tests to timing-model; r=hiro
https://hg.mozilla.org/integration/autoland/rev/bfa0d1a4bf1c
Tidy up tests for "finishing an animation"; r=hiro
https://hg.mozilla.org/integration/autoland/rev/920aa51ae3a2
Further divide up finishing tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/8f61bf3de90a
Add tests for Animation.updatePlaybackRate; r=hiro
https://hg.mozilla.org/integration/autoland/rev/607dccfa8387
Add Animation.updatePlaybackRate WebIDL definition; r=bz
https://hg.mozilla.org/integration/autoland/rev/7301bfeeb65c
Factor out static time calculation methods on Animation; r=hiro
https://hg.mozilla.org/integration/autoland/rev/e9381081ab6a
Add Animation::GetCurrentTimeForHoldTime helper; r=hiro
https://hg.mozilla.org/integration/autoland/rev/4ae911f19aee
Implement pending playback rate mechanism; r=hiro
https://hg.mozilla.org/integration/autoland/rev/c653d7a1b3ef
Support pending playback rates on compositor animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a2890507d13a
Use updatePlaybackRate in DevTools; r=daisuke
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9522 for changes under testing/web-platform/tests
Upstream web-platform-tests status error for continuous-integration/travis-ci/pr. This will block the upstream PR from merging. See https://travis-ci.org/w3c/web-platform-tests/builds/341602921?utm_source=github_status&utm_medium=notification for more information
Upstream PR was closed without merging
It looks like when autoland was closed bug 1436978 was also in the queue and it landed first and bitrotted this.
Flags: needinfo?(bbirtles)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 86

a year ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0634cd8f08eb
Rename tests in timing-model/animations to match spec section titles; r=hiro
https://hg.mozilla.org/integration/autoland/rev/c8ee4a012dae
Use async/await in timing-model/animations tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/c2c352456a4c
Move playback rate test to timing-model; r=hiro
https://hg.mozilla.org/integration/autoland/rev/b93a8879555d
Simplify playback rate setting test; r=hiro
https://hg.mozilla.org/integration/autoland/rev/b86e1331cb36
Move finishing tests to timing-model; r=hiro
https://hg.mozilla.org/integration/autoland/rev/44ddf14fd334
Tidy up tests for "finishing an animation"; r=hiro
https://hg.mozilla.org/integration/autoland/rev/a1a5840a6bb5
Further divide up finishing tests; r=hiro
https://hg.mozilla.org/integration/autoland/rev/7465cb110ae5
Add tests for Animation.updatePlaybackRate; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5ec4adb145cf
Add Animation.updatePlaybackRate WebIDL definition; r=bz
https://hg.mozilla.org/integration/autoland/rev/e11bc080108c
Factor out static time calculation methods on Animation; r=hiro
https://hg.mozilla.org/integration/autoland/rev/2be0189e2e8e
Add Animation::GetCurrentTimeForHoldTime helper; r=hiro
https://hg.mozilla.org/integration/autoland/rev/3fc608432c52
Implement pending playback rate mechanism; r=hiro
https://hg.mozilla.org/integration/autoland/rev/0b4efe60e6ec
Support pending playback rates on compositor animations; r=hiro
https://hg.mozilla.org/integration/autoland/rev/1082bb7dbc52
Use updatePlaybackRate in DevTools; r=daisuke
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9531 for changes under testing/web-platform/tests
Upstream web-platform-tests status error for continuous-integration/travis-ci/pr. This will block the upstream PR from merging. See https://travis-ci.org/w3c/web-platform-tests/builds/341686519?utm_source=github_status&utm_medium=notification for more information
(In reply to Web Platform Test Sync Bot from comment #88)
> Upstream web-platform-tests status error for
> continuous-integration/travis-ci/pr. This will block the upstream PR from
> merging. See
> https://travis-ci.org/w3c/web-platform-tests/builds/
> 341686519?utm_source=github_status&utm_medium=notification for more
> information

As far as I can tell, this situation can only possibly arise if the stability check against "firefox:nightly" is actually being run against a version of Firefox that does not include bug 1422248 (i.e. Firefox 59 or earlier).
Upstream web-platform-tests status error for continuous-integration/travis-ci/pr. This will block the upstream PR from merging. See https://travis-ci.org/w3c/web-platform-tests/builds/341811882?utm_source=github_status&utm_medium=notification for more information
Can't merge web-platform-tests PR due to failing upstream tests
Upstream PR merged
(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #94)
> Added MDN:
> https://developer.mozilla.org/en-US/docs/Web/API/Animation/updatePlaybackRate
> 
> Browser compat PR:
> https://github.com/mdn/browser-compat-data/pull/1106
> 
> Chris, can you please review the added page?

Thanks Brian! I've copy edited the new page, and updated its parent page so that the new method is listed.

I've also updated the Fx60 rel notes to mention it:
https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM
Flags: needinfo?(cmills)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #95)
> Thanks Brian! I've copy edited the new page, and updated its parent page so
> that the new method is listed.
> 
> I've also updated the Fx60 rel notes to mention it:
> https://developer.mozilla.org/en-US/Firefox/Releases/60#DOM

Thanks Chris!
I'm curious why the browser compatability data doesn't show up, however. The PR was merged about a week ago.
(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #97)
> I'm curious why the browser compatability data doesn't show up, however. The
> PR was merged about a week ago.

There is some kind of build step that has to happen before the data is actually available, and a new version of the NPM package is available, etc.
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.