Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Allow mozRequestAnimationFrame requests to be cancelable

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla11
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
We should return an integer ID from mozRequestAnimationFrame, and add a mozCancelRequestAnimationFrame method to that can cancel ones of these requests by ID.
(Reporter)

Comment 1

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

Comment 2

6 years ago
Taking, but if someone else wants this, feel free to steal.
Assignee: nobody → bzbarsky
Priority: -- → P1
Blocks: 704063
(Assignee)

Updated

6 years ago
Depends on: 704171
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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 :/
(Assignee)

Comment 9

6 years ago
I don't see how it's simpler than keeping a flag, honestly.  Should be exactly identical to a flag...  Explain?

Comment 10

6 years ago
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...
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 19

6 years ago
Created attachment 579255 [details] [diff] [review]
part 1.  Make requestAnimationFrame return handles for animation frame requests.
Attachment #579255 - Flags: review?(roc)
(Assignee)

Comment 20

6 years ago
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?
Attachment #579256 - Flags: review?(roc)
(Assignee)

Comment 21

6 years ago
Created attachment 579259 [details] [diff] [review]
part 3.  Use cancelRequestAnimationCallback instead of tracking booleans in chrome code.
Attachment #579259 - Flags: review?(gavin.sharp)
(Assignee)

Updated

6 years ago
Whiteboard: [need review]
Attachment #579255 - Flags: review?(roc) → review+
Attachment #579256 - Flags: review?(roc) → review+
Are we really using numerical handles here?
(Assignee)

Comment 23

6 years ago
See comment 3, comment 4, comment 5, comment 12, and the spec.

In short, for now yes.
(Assignee)

Comment 24

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 708173
(Assignee)

Updated

6 years ago
Attachment #579259 - Attachment is obsolete: true
Attachment #579259 - Flags: review?(gavin.sharp)
(Assignee)

Comment 25

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/378505d09d17
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e232dd3af61
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Blocks: 708194
https://hg.mozilla.org/mozilla-central/rev/378505d09d17
https://hg.mozilla.org/mozilla-central/rev/3e232dd3af61
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 27

6 years ago
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
(Assignee)

Comment 28

6 years ago
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.
The spec has been changed.
http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/RequestAnimationFrame/Overview.html#cancelAnimationFrame
(Assignee)

Comment 30

6 years ago
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.
(Assignee)

Comment 32

6 years ago
Given the instability of the spec, I'm not sure we can in fact add the unprefixed new name.
(Assignee)

Updated

6 years ago
Depends on: 709384
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.