Closed Bug 569520 Opened 10 years ago Closed 10 years ago
Implement JS animation-scheduling API
684 bytes, text/html
7.11 KB, patch
|Details | Diff | Splinter Review|
13.87 KB, patch
|Details | Diff | Splinter Review|
20.75 KB, patch
|Details | Diff | Splinter Review|
Just trying to get a summary of goals down: 1. To avoid running the CPU more often than required by giving access to the refresh rate of a browser page. 2. To give us the ability to turn down, or turn off animations when a canvas element isn't visible (i.e. it's scrolled off page or in a background tab.) 3. To make it very easy for people to take advantage of this API.
This is totally not ready; see the XXX comment about ownership model and such. But it makes the square in that testcase animate!
I made |time| 64-bit so that I can actually store an epoch time in ms there for beforepaint events.
I tested this on the attached testcase with the refresh driver throttled to 10Hz. Works great. It feels silly to keep two separate timestamps in the refresh driver, but I couldn't see an obvious better way to do it. On the bright side, this _is_ using the same logic as transitions, etc. Please double-check the logic surrounding presshells going away, not being there, coming back, etc, carefully? Flagging jst for the DOM-facing API sr.
Whiteboard: [evang-wanted] → [need review][evang-wanted]
Comment on attachment 464319 [details] [diff] [review] Main part of the fix + // We don't want to use GetPresShell() here, because we want to You mean GetShell + FORWARD_TO_OUTER(MozRequestAnimationFrame, (), NS_ERROR_NOT_INITIALIZED); I'm not very familiar with the inner/outer logic, but why are we forwarding to the outer instead of the inner here? mozilla::TimeStamp MostRecentRefresh() const; + PRInt64 MostRecentRefreshEpochTime() const; Need to say what the units are ... microseconds since the epoch Very nice!
Attachment #464319 - Flags: review?(roc) → review+
We need to figure out how to evangelize this ... to get JQuery to support it, etc
We also need someone to write a more formal spec proposal.
Comment on attachment 464318 [details] [diff] [review] Part 1 actually compiling >@@ -529,17 +530,17 @@ public: > PRUint8 eventStructType; > // See GUI MESSAGES, > PRUint32 message; > // Relative to the widget of the event, or if there is no widget then it is > // in screen coordinates. Not modified by layout code. > nsIntPoint refPoint; > // Elapsed time, in milliseconds, from a platform-specific zero time > // to the time the message was created >- PRUint32 time; >+ PRUint64 time; Does this work with non-IPC plugins on Linux?
> I'm not very familiar with the inner/outer logic, but why are we forwarding to > the outer instead of the inner here? Hmm. We forward to the outer for the other method because that's where the docshell lives. Perhaps I should simply forward to the inner for both methods, and get the presshell off the document. That would make more sense anyway; I'll do it. This was one of the things I wanted jst to look over, fwiw. > Does this work with non-IPC plugins on Linux? I have no idea. What issues would it cause? I can avoid it if I create a new event class, but its a bunch more work to do that, and it might be more cognitive overhead for authors...
As far as jquery goes, ccing jresig.
I looked over what the plug-in code does with .time, and there shouldn't be any problems.
Attachment #464609 - Flags: superreview?(jst) → superreview+
Attachment #464318 - Flags: review?(Olli.Pettay) → review+
This adds a new web-facing way to do animations that plays much nicer with our attempts to do less work in background tabs (e.g. we can either stop or significantly throttle the animations in background tabs if they use this API). For mobile it has the additional benefit of reducing the number of in-flight timers.
Attachment #464832 - Flags: approval2.0?
Comment on attachment 464832 [details] [diff] [review] Roll-up patch for approval approved with at least some basic testing.
Attachment #464832 - Flags: approval2.0? → approval2.0+
Whiteboard: [need review][evang-wanted] → [need landing][evang-wanted]
Pushed with a test: http://hg.mozilla.org/mozilla-central/rev/464f978cfca5 http://hg.mozilla.org/mozilla-central/rev/66c25030a8d4 Summary of how this works: 1) A new API on the Window object, window.mozAnimationStartTime, returns the time, in ms since the epoch, when animations started "now" should be assumed to start. This may be slightly off from Date.now when multiple animations are starting at once. 2) A new API on the Window object, window.mozRequestAnimationFrame, ensures that a MozBeforePaint event will be fired. 3) A MozBeforePaint event is fired, if requested, at the Document, bubbling to the window. The .timeStamp of this event is the time, in ms since the epoch, when the paint should be assumed to happen. This may be slightly off from Date.now when a number of animations are in progress at once. A typical API consumer will save the current mozAnimationStartTime, request an animation frame, and add a MozBeforePaint event listener. Then in the event listener subtract the event's timeStamp from the saved start time to determine how long the animation has been in progress as of the paint, and update the state of the DOM and CSSOM accordingly.
And this got backed out. NOTE: use the changesets from comment 20, NOT the attached patches, when relanding.
Part 1: http://hg.mozilla.org/mozilla-central/rev/bf2386d0cc1b Part 2: http://hg.mozilla.org/mozilla-central/rev/96de52e4a442
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [need landing][evang-wanted] → [evang-wanted]
Target Milestone: --- → mozilla2.0b4
Seems to have random orange. Bug 586900
Thanks!!! I made some edits, particularly to the first page, to note about the fact that we limit the frame rate intentionally. The sample code should have a removeEventListener added. I updated the inline example, but I can't figure out how to update https://developer.mozilla.org/samples/js-animation/mozbeforepaint.html.
I've updated it; thanks!
The test for this bug has been failing intermittently (few times a week) for few months now - Bug 858737. The part which fails is that sometimes the argument values from 2 consecutive mozRequestAnimationFrame callbacks are non monotonic, but the test expects them to be. bz suggested to use the non prefixed version instead since its callback argument isn't based on wall clock and it should be monotonic. This would also require to replace mozAnimationStartTime with performance.now() within that test. However, doing so would leave us, as far as I can tell, without a test for the prefixed version (if ignoring other code/tests which might still use the prefixed version). We do have a test which checks that performance.now() is monotonic, but it uses setTimeout and doesn't test rAF's callback argument or rAF itself. See bug 749894 which touched this test. Also, since the un/prefixed versions behave differently (perf.now arg + monotinic vs wall clock arg + not necessarily monotonic), we should probably update the docs at https://developer.mozilla.org/en-US/docs/Web/API/window.requestAnimationFrame to reflect this info. So in order to cover both the prefixed and unprefixed versions, I was thinking of adding to test_bug569520.html code which tests the unprefixed version as well, and in order to also test the prefixed version, give it some slack in what we actually test. Problem is, I don't know what form should this slack take. Any help here? or concerns with my approach in general? Bug 753453 discussed the monotonic arg to the unprefixed rAF and also concluded that the prefixed version should keep the old behavior.
We should file a bug to remove the prefixed version, imo, and remove it.
I agree. We certainly shouldn't be wasting time fixing the prefixed version.
Filed bug 909154 to consider removing support for the prefixed mozRequestAnimationFrame.
I've updates the test for this bug in bug 858737. The new test checks for monotonic args with the unprefixed rAF instead of the prefixed one, but still keeps the other 2 prefixed rAF tests, since we still have unprefixed versions at the code. Of the two prefixed tests, I've also given a bit more slack to one.
You need to log in before you can comment on or make changes to this bug.