Last Comment Bug 647518 - Allow mozRequestAnimationFrame requests to be cancelable
: Allow mozRequestAnimationFrame requests to be cancelable
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal with 1 vote (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 704171 709384
Blocks: 708194 704063 708173
  Show dependency treegraph
 
Reported: 2011-04-02 23:58 PDT by Cameron McCormack (:heycam)
Modified: 2012-02-27 13:20 PST (History)
14 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Make requestAnimationFrame return handles for animation frame requests. (8.19 KB, patch)
2011-12-06 00:41 PST, Boris Zbarsky [:bz]
roc: review+
Details | Diff | Review
part 2. Allow canceling requestAnimationFrame requests. (6.45 KB, patch)
2011-12-06 00:42 PST, Boris Zbarsky [:bz]
roc: review+
Details | Diff | Review
part 3. Use cancelRequestAnimationCallback instead of tracking booleans in chrome code. (3.38 KB, patch)
2011-12-06 00:52 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Review

Description Cameron McCormack (:heycam) 2011-04-02 23:58:25 PDT
We should return an integer ID from mozRequestAnimationFrame, and add a mozCancelRequestAnimationFrame method to that can cancel ones of these requests by ID.
Comment 1 Cameron McCormack (:heycam) 2011-04-02 23:59:13 PDT
(This allows animations implemented with setTimeout/clearTimeout to be more easily converted to requestAnimationFrame.)
Comment 2 Boris Zbarsky [:bz] 2011-04-05 16:16:32 PDT
Taking, but if someone else wants this, feel free to steal.
Comment 3 Boris Zbarsky [:bz] 2011-11-21 10:33:32 PST
Did we ever make progress on whether the handle should be an integer or not?
Comment 4 Jonas Sicking (:sicking) 2011-11-21 10:46:57 PST
Not to my knowledge.

I kind'a think it shouldn't be an integer.
Comment 5 Boris Zbarsky [:bz] 2011-11-21 11:00:05 PST
OK.  So if it's not, we need to get the spec changed and figure out an implementation strategy for it....  I suppose we could just create nonce cycle-collected nsISupports objects on every requestAnimationFrame call, but that seems pretty heavyweight.  :(
Comment 6 Dão Gottwald [:dao] 2011-11-28 06:17:28 PST
(In reply to Cameron McCormack (:heycam) (away 21 - around 30 November) from comment #1)
> (This allows animations implemented with setTimeout/clearTimeout to be more
> easily converted to requestAnimationFrame.)

Having converted code myself, I never wanted to cancel a requested animation frame. Not sure what the goal is here.
Comment 7 Boris Zbarsky [:bz] 2011-11-28 10:26:33 PST
We have multiple instances of code in the tree that canceled animation frames; see the patch in bug 704171 which converted them to boolean flags for now.

As for the rest, the goal is to implement the current spec draft so we can move toward unprefixing this method.
Comment 8 Dão Gottwald [:dao] 2011-11-30 00:21:47 PST
(In reply to Boris Zbarsky (:bz) from comment #7)
> We have multiple instances of code in the tree that canceled animation
> frames; see the patch in bug 704171 which converted them to boolean flags
> for now.

That code used to just stop listening to MozBeforePaint, which is simpler than using a flag or keeping a handle around. See also bug 706323 comment 4 :/
Comment 9 Boris Zbarsky [:bz] 2011-11-30 08:56:07 PST
I don't see how it's simpler than keeping a flag, honestly.  Should be exactly identical to a flag...  Explain?
Comment 10 Jörn Berkefeld 2011-12-02 05:59:02 PST
Hi there,
I think I'm stating the obvious here but why don't you just copy the behavior of.. ohh, every other browser out there?!

There is a setInterval/Timeout function in all browsers just like there is a clearInterval/Timeout function.
Not providing a cancelRequestAnimationFrame is simply not consistent with the other functions and should therefore be changed immediately. As a fact most users currently fall back to using setInterval/Timeout in Firefox because that way they do not need extra code for this one browser (try googling it if you don't believe me)

Apart from that, your proposed solution of checking an on/off flag each time an animation frame is run wastes cpu-time - keeping a reference handle that can be used to stop the animation all together only wastes a tiny bit of memory.

my two cents...
Comment 11 Boris Zbarsky [:bz] 2011-12-02 10:37:57 PST
Uh.  I have no idea what comment 10 is about.  I'm working on fixing this bug.  Comment 6 through comment 9 have nothing to do with this bug.  Nor does comment 10.  That's half the comments on the bug being useless noise...
Comment 12 Jonas Sicking (:sicking) 2011-12-02 11:07:45 PST
FWIW, the more I think about it, the more I'm ok with returning an integer here. It does seem silly to waste performance on creating new objects, be they XPCOM or plain JS objects.
Comment 13 Dão Gottwald [:dao] 2011-12-03 03:00:05 PST
(In reply to Boris Zbarsky (:bz) from comment #9)
> I don't see how it's simpler than keeping a flag, honestly.  Should be
> exactly identical to a flag...  Explain?

It seems obvious to me that http://hg.mozilla.org/mozilla-central/diff/2db300a6241d/toolkit/content/widgets/scrollbox.xml is the opposite of a code simplification. The code needs to keep a flag or handle around, previously it didn't.
Comment 14 Masatoshi Kimura [:emk] 2011-12-03 03:09:42 PST
The flag will be gone once this bug is fixed.
Comment 15 Dão Gottwald [:dao] 2011-12-03 03:11:01 PST
I said "flag or handle"...
Comment 16 Masatoshi Kimura [:emk] 2011-12-03 03:19:08 PST
Then it is completely unrelated to this bug. We will not bring back mozBeforePaint event unless it is spec'ed. Please stop advocating here.
Comment 17 Dão Gottwald [:dao] 2011-12-03 03:44:25 PST
I'm not going to let you tell me to shut up, sorry. What I'm saying is directly related to this bug, since this bug is going to add the handle and motivated the mozBeforePaint removal. Whether it's our fault or the spec's I don't care. If my comments imply that the spec is suboptimal, then I guess that's the gist of my comments. However, as I understand it, the spec doesn't actually mandate the mozBeforePaint removal.
Comment 18 Masatoshi Kimura [:emk] 2011-12-03 03:50:26 PST
OK, filed bug 707425. Please do not advocate HERE.
Comment 19 Boris Zbarsky [:bz] 2011-12-06 00:41:27 PST
Created attachment 579255 [details] [diff] [review]
part 1.  Make requestAnimationFrame return handles for animation frame requests.
Comment 20 Boris Zbarsky [:bz] 2011-12-06 00:42:45 PST
Created attachment 579256 [details] [diff] [review]
part 2.  Allow canceling requestAnimationFrame requests.

I believe just removing from the list on cancel is equivalent to what the spec calls for, but please double-check me on that, ok?
Comment 21 Boris Zbarsky [:bz] 2011-12-06 00:52:24 PST
Created attachment 579259 [details] [diff] [review]
part 3.  Use cancelRequestAnimationCallback instead of tracking booleans in chrome code.
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-12-06 04:13:24 PST
Are we really using numerical handles here?
Comment 23 Boris Zbarsky [:bz] 2011-12-06 11:01:51 PST
See comment 3, comment 4, comment 5, comment 12, and the spec.

In short, for now yes.
Comment 24 Boris Zbarsky [:bz] 2011-12-06 20:03:43 PST
Comment on attachment 579255 [details] [diff] [review]
part 1.  Make requestAnimationFrame return handles for animation frame requests.

> +  if (newHandle == 0) {

Actually, this is bogus.  First of all, this is a signed int, so we'll wrap to negatives before we hit 0.  Second of all, we already incremented, so we'll just throw once, then happily proceed to keep scheduling.  I'm going to replace this with a test against PR_INT32_MAX before incrementing.

Also, going to push parts 1 and 2 and spin part 3 off into a separate bug.
Comment 27 Nickolay_Ponomarev 2011-12-10 13:13:59 PST
dev-doc-needed: https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame + a new page for window.mozCancelRequestAnimationFrame(). As far as I can tell from the patches, comment 0 describes the changes made here accurately.
Comment 28 Boris Zbarsky [:bz] 2011-12-10 15:16:01 PST
Note that the spec here is changing, so we will be renaming this method shortly (no bug on this yet).  So when documenting, it might be better to just document the new name.
Comment 30 Boris Zbarsky [:bz] 2011-12-13 18:45:45 PST
Yeah, I know.  Thanks for the heads-up, though!

We may need to still ship mozCancelRequestAnimationFrame (in addition to the new name, which I will file a bug on in the next day or two) unless the sites that depend on it get fixed to use the new name.  :(
Comment 31 Masatoshi Kimura [:emk] 2011-12-14 18:17:55 PST
We can add the unprefixed new name (that is, fix bug 704063) and leave the prefixed name as-is rather than renaming twice.
Comment 32 Boris Zbarsky [:bz] 2011-12-14 20:04:44 PST
Given the instability of the spec, I'm not sure we can in fact add the unprefixed new name.
Comment 33 Eric Shepherd [:sheppy] 2012-02-27 13:20:11 PST
Docs updated:

https://developer.mozilla.org/en/DOM/window.requestAnimationFrame

New article:

https://developer.mozilla.org/en/DOM/window.cancelAnimationFrame

Also updated Firefox 11 for developers.

Note You need to log in before you can comment on or make changes to this bug.