647518 - Allow mozRequestAnimationFrame requests to be cancelable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Cameron McCormack (:heycam)]

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)]

(This allows animations implemented with setTimeout/clearTimeout to be more easily converted to requestAnimationFrame.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Taking, but if someone else wants this, feel free to steal.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Did we ever make progress on whether the handle should be an integer or not?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not to my knowledge.

I kind'a think it shouldn't be an integer.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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]]

(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 [:bzbarsky]]

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]]

(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 [:bzbarsky]]

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]

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 [:bzbarsky]]

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) No longer reading bugmail consistently]

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]]

(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]]

The flag will be gone once this bug is fixed.

Comment 15

[Dão Gottwald [:dao]]

I said "flag or handle"...

Comment 16

[Masatoshi Kimura [:emk]]

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]]

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]]

OK, filed bug 707425. Please do not advocate HERE.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make requestAnimationFrame return handles for animation frame requests.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

Attachment: part 3. Use cancelRequestAnimationCallback instead of tracking booleans in chrome code.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are we really using numerical handles here?

Comment 23

[Boris Zbarsky [:bzbarsky]]

See comment 3, comment 4, comment 5, comment 12, and the spec.

In short, for now yes.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/378505d09d17
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e232dd3af61

Comment 26

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/378505d09d17
https://hg.mozilla.org/mozilla-central/rev/3e232dd3af61

Comment 27

[Nickolay_Ponomarev]

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 [:bzbarsky]]

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 29

[Masatoshi Kimura [:emk]]

The spec has been changed.
http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/RequestAnimationFrame/Overview.html#cancelAnimationFrame

Comment 30

[Boris Zbarsky [:bzbarsky]]

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]]

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 [:bzbarsky]]

Given the instability of the spec, I'm not sure we can in fact add the unprefixed new name.

Comment 33

[Eric Shepherd [:sheppy]]

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.