1181762 - Remove uses of mozRequestAnimationFrame from toolkit code

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove uses of mozRequestAnimationFrame from toolkit code

Comment 2

[:Gijs (he/him)]

Comment on attachment 8631241 [details] [diff] [review]
Remove uses of mozRequestAnimationFrame from toolkit code

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

rs=me, though I don't profess to understand the mozAnimationStartTime vs. performance.now() distinction, and I trust that you're sure that the difference is unimportant.

::: toolkit/content/browser-content.js
@@ +142,5 @@
>      this._screenX = event.screenX;
>      this._screenY = event.screenY;
>      this._scrollErrorX = 0;
>      this._scrollErrorY = 0;
> +    this._lastFrame = content.performance.now();

Based on MDN, this is not exactly identical to mozAnimationStartTime. Is MDN wrong? If not, does the difference matter here?

::: toolkit/content/widgets/progressmeter.xml
@@ +73,5 @@
>            var spacer =
>              document.getAnonymousElementByAttribute(this, "anonid", "spacer");
>            var isLTR =
>             document.defaultView.getComputedStyle(this, null).direction == "ltr";
> +          var startTime = performance.now();

Ditto.

::: toolkit/content/widgets/scrollbox.xml
@@ +291,5 @@
>          start: function scrollAnim_start(distance) {
>            this.distance = distance;
>            this.startPos = this.scrollbox.scrollPosition;
>            this.duration = Math.min(1000, Math.round(50 * Math.sqrt(Math.abs(distance))));
> +          this.startTime = window.performance.now();

And here.

@@ +676,5 @@
>        <field name="_arrowScrollAnim"><![CDATA[({
>          scrollbox: this,
>          requestHandle: 0, /* 0 indicates there is no pending request */
>          start: function arrowSmoothScroll_start() {
> +          this.lastFrameTime = window.performance.now();

and here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> and I trust that you're sure that the difference is unimportant.

I'm not.  I was hoping you knew enough about the code I'm changing to judge that.  How about I explain the difference?

performance.now() is the time right now, with the same zero time as the argument to the callback passed to requestAnimationFrame.

mozAnimationStartTime is the time when the refresh driver last ticked, with the same zero time as the argument to the callback passed to mozRequestAnimationFrame.  This is NOT the same zero time as the zero time for performance.now(): performance.now()/requestAnimationFrame has navigation start as 0 time, while mozAnimationStartTime/mozRequestAnimationFrame has the Unix epoch as 0 time.

So in there is a bit of an offset between performance.now() and mozAnimationStartTime, even ignoring the different zero times; up to a single refresh tick (so figure 16ms).

The basic difference in practice is whether you want your animation to look like it started at the previous refresh tick or whether you want it to look like it started at the point when performance.now() is called.  But it can depend on the details of how this time is used by the animation code, of course.

I _could_ probably preserve the behavior we have right now by doing mozAnimationStartTime - navigationStart to get it into the right timebase.  However, mozAnimationStartTime never ended up standardized, and it would be kinda nice to work on removing it... but I can do that in a separate bug if you prefer.

Comment 4

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> So in there is a bit of an offset between performance.now() and
> mozAnimationStartTime, even ignoring the different zero times; up to a
> single refresh tick (so figure 16ms).
> 
> The basic difference in practice is whether you want your animation to look
> like it started at the previous refresh tick or whether you want it to look
> like it started at the point when performance.now() is called.  But it can
> depend on the details of how this time is used by the animation code, of
> course.

Right, so I don't know all of this code as a matter of course, but I think we can work it out.

In browser-content.js, we set the initial _lastFrame value, which gets used like:

183     // avoid long jumps when the browser hangs for more than
184     // |maxTimeDelta| ms
185     const maxTimeDelta = 100;
186     var timeDelta = Math.min(maxTimeDelta, timestamp - this._lastFrame);
187     // we used to scroll |accelerate()| pixels every 20ms (50fps)
188     var timeCompensation = timeDelta / 20;
189     this._lastFrame = timestamp;

We then use timeCompensation in determining scroll acceleration. So this ought to be alright; it's probably more correct to accelerate based on the initial start of autoscroll rather than the last refresh tick.


In progressmeter.xml, we calculate the elapsed time for an undetermined progressmeter so it animates smoothly. The difference will only influence the starting position/offset, so as best I can tell that ought to be fine, too.

Finally, in scrollbox.xml we're doing autoscrolling, and again, this might slightly alter scrolling speed but it seems like "current time" rather than "previous refresh tick time" would be more correct anyway.

So... seems fine to me? If in doubt, looking at blame might suggest other people who know this code better...

Comment 5

[Boris Zbarsky [:bzbarsky]]

> So... seems fine to me? 

Yep.  Works for me.  Thanks!

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19640c24a48b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19640c24a48b