1181966 - Remove uses of mozRequestAnimationFrame from browser code

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove uses of mozRequestAnimationFrame from browser code

Subject to the same caveats as bug 1181762 comment 3

Comment 2

[:Gijs (he/him)]

Comment on attachment 8631876 [details] [diff] [review]
Remove uses of mozRequestAnimationFrame from browser code

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

Please revert the jquery change unless there's a really good reason we need to change that library as it is in our tree. :-)

Besides the issues below, LGTM, but maybe get a check-in from :paul about tilt.

::: browser/devtools/tilt/tilt-visualizer.js
@@ +459,5 @@
>        return;
>      }
>  
>      // prepare for the next frame of the animation loop
> +    this.chromeWindow.requestAnimationFrame(this._loop.bind(this));

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/tilt/tilt-utils.js#511 does this so I don't think we need to do it here.

@@ +482,5 @@
>     */
>    _handleFrameDelta: function TVP__handleFrameDelta()
>    {
>      this._prevFrameTime = this._currFrameTime;
> +    this._currFrameTime = this.chromeWindow.performance.now();

So... I am not an expert on this code. I'd recommend asking Paul Rouget. But if I understand this correctly, based on your explanation in the toolkit/ bug, this is called from _loop, but it is called after a redraw, if we did one. If we do one, I could imagine that taking substantial time. Unfortunately, that means that now() and mozAnimationStartTime will have diverged, since we might be, say, 5 or 10ms out from the start _loop (being called from requestAnimationFrame). That's going to mess with _currFrameTime and _delta. Does that analysis sound sensible?

If so, I imagine we should pass in _currFrameTime as an argument (or set it earlier or something) and call window.performance.now early in _loop instead.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Please revert the jquery change unless there's a really good reason we need to change
> that library as it is in our tree. :-)

There's a good reason.  This is an antediluvian version of the library that has no concept of unprefixed requestAnimationFrame.  The part I'm changing is this bit:

  cp=a.webkitRequestAnimationFrame||a.mozRequestAnimationFrame||a.oRequestAnimationFrame;

and I'm just removing the prefix from the second bit.  If I don't do that, then any consumers of this code will end up using setInterval instead when I remove mozRequestAnimationFrame, which seems pretty suboptimal.

I can undo this change and let things fall through to setInterval if you think that's the right thing... please let me know.

> I don't think we need to do it here.

Good catch.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Does that analysis sound sensible?

Yes, it does.  Paul, does just passing the time grabbed at the top of _loop to handleFrameDelta make sense to you?

Comment 5

[Paul Rouget [:paul]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > Does that analysis sound sensible?
> 
> Yes, it does.  Paul, does just passing the time grabbed at the top of _loop
> to handleFrameDelta make sense to you?

Yes.

Comment 6

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > Please revert the jquery change unless there's a really good reason we need to change
> > that library as it is in our tree. :-)
> 
> There's a good reason.  This is an antediluvian version of the library that
> has no concept of unprefixed requestAnimationFrame.  The part I'm changing
> is this bit:
> 
>  
> cp=a.webkitRequestAnimationFrame||a.mozRequestAnimationFrame||a.
> oRequestAnimationFrame;
> 
> and I'm just removing the prefix from the second bit.  If I don't do that,
> then any consumers of this code will end up using setInterval instead when I
> remove mozRequestAnimationFrame, which seems pretty suboptimal.

This is only used in test code for event handling detection in the markup view of the devtools. From a quick glance at the test ( http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_markupview_events_jquery_1.6.js, see also http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/helper_events_test_runner.js#9 , I don't think it matters either way, so rs=me for either fixing it or leaving it alone.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove uses of mozRequestAnimationFrame from browser code

Comment 8

[:Gijs (he/him)]

Comment on attachment 8634204 [details] [diff] [review]
Remove uses of mozRequestAnimationFrame from browser code

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

Nice!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4db7a8797615

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4db7a8797615