1361260 - When calculating the start time for a pending compositor animation, the playback rate should be included

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

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

Basically, we should be doing the same calculation as Animation::StartTimeFromReadyTime.

Comment 1

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

Attachment: Bug 1361260 - Incorporate playbackRate when calculating the start time of a pending compositor animation;

Review commit: https://reviewboard.mozilla.org/r/135412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135412/

Comment 2

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

Comment on attachment 8863623 [details]
Bug 1361260 - Incorporate playbackRate when calculating the start time of a pending compositor animation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135412/diff/1-2/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863623 [details]
Bug 1361260 - Incorporate playbackRate when calculating the start time of a pending compositor animation;

https://reviewboard.mozilla.org/r/135412/#review138378

::: dom/animation/test/mozilla/file_deferred_start.html:132
(Diff revision 2)
> +// NOTE: As with the previous test, it is important that we DON'T use
> +// SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh here since that takes
> +// us through a different code path.
> +promise_test(function(t) {

It would be nice to have an assertion to check DOMWindowUtils.isTestControllingRefreshes?

::: dom/animation/test/mozilla/file_deferred_start.html:141
(Diff revision 2)
> +  const div = addDiv(t);
> +  div.classList.add('target');

Nit: addDiv(t, { class: 'target' });

::: dom/animation/test/mozilla/file_deferred_start.html:155
(Diff revision 2)
> +    var transformStr =
> +      SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
> +
> +    var matrixComponents =
> +      transformStr.startsWith('matrix(')
> +      ? transformStr.substring('matrix('.length, transformStr.length-1)
> +                    .split(',')
> +                    .map(component => Number(component))
> +      : [];
> +    assert_equals(matrixComponents.length, 6,
> +                  'Got a valid transform matrix on the compositor'
> +                  + ' (got: "' + transformStr + '")');

This should be factored out as a function and reuse it in above test as well?

::: gfx/layers/Layers.cpp:295
(Diff revision 2)
> +          // This mirrors the calculation in Animation::StartTimeFromReadyTime.
>            if (anim.startTime().IsNull() && !anim.isNotPlaying()) {
> -            anim.startTime() = aReadyTime - anim.holdTime();
> +            anim.startTime() =
> +              anim.playbackRate() == 0
> +              ? aReadyTime
> +              : aReadyTime - anim.holdTime().MultDouble(1.0 /
> +                                                        anim.playbackRate());

This change is really great!

Comment 4

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

Comment on attachment 8863623 [details]
Bug 1361260 - Incorporate playbackRate when calculating the start time of a pending compositor animation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135412/diff/2-3/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc0edea49a70
Incorporate playbackRate when calculating the start time of a pending compositor animation; r=hiro

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc0edea49a70