Remove uses of mozRequestAnimationFrame from browser code

RESOLVED FIXED in Firefox 42

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
Firefox 42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8631876 [details] [diff] [review]
Remove uses of mozRequestAnimationFrame from browser code

Subject to the same caveats as bug 1181762 comment 3
Attachment #8631876 - Flags: review?(gijskruitbosch+bugs)

Comment 2

3 years ago
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.
Attachment #8631876 - Flags: review?(gijskruitbosch+bugs) → feedback+
> 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.
Flags: needinfo?(gijskruitbosch+bugs)
> 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?
Flags: needinfo?(paul)

Comment 5

3 years ago
(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.
Flags: needinfo?(paul)

Comment 6

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Created attachment 8634204 [details] [diff] [review]
Remove uses of mozRequestAnimationFrame from browser code
Attachment #8634204 - Flags: review?(paul)
Attachment #8634204 - Flags: review?(gijskruitbosch+bugs)
Attachment #8631876 - Attachment is obsolete: true

Updated

3 years ago
Attachment #8634204 - Flags: review?(paul) → review+

Comment 8

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

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

Nice!
Attachment #8634204 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4db7a8797615
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.