Closed Bug 647518 Opened 13 years ago Closed 13 years ago

Allow mozRequestAnimationFrame requests to be cancelable

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: heycam, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

We should return an integer ID from mozRequestAnimationFrame, and add a mozCancelRequestAnimationFrame method to that can cancel ones of these requests by ID.
(This allows animations implemented with setTimeout/clearTimeout to be more easily converted to requestAnimationFrame.)
Taking, but if someone else wants this, feel free to steal.
Assignee: nobody → bzbarsky
Priority: -- → P1
Blocks: 704063
Depends on: 704171
Did we ever make progress on whether the handle should be an integer or not?
Not to my knowledge.

I kind'a think it shouldn't be an integer.
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.  :(
(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.
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.
(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 :/
I don't see how it's simpler than keeping a flag, honestly.  Should be exactly identical to a flag...  Explain?
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...
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...
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.
(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.
The flag will be gone once this bug is fixed.
I said "flag or handle"...
Then it is completely unrelated to this bug. We will not bring back mozBeforePaint event unless it is spec'ed. Please stop advocating here.
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.
OK, filed bug 707425. Please do not advocate HERE.
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?
Attachment #579256 - Flags: review?(roc)
Whiteboard: [need review]
Are we really using numerical handles here?
See comment 3, comment 4, comment 5, comment 12, and the spec.

In short, for now yes.
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.
Blocks: 708173
Attachment #579259 - Attachment is obsolete: true
Attachment #579259 - Flags: review?(gavin.sharp)
Blocks: 708194
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.
Keywords: dev-doc-needed
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.
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.  :(
We can add the unprefixed new name (that is, fix bug 704063) and leave the prefixed name as-is rather than renaming twice.
Given the instability of the spec, I'm not sure we can in fact add the unprefixed new name.
Depends on: 709384
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.