Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Description • 14 years ago

See
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/527d0cedb9b0df7f/57625c94cdf493bf?pli=1

	Christopher Blizzard (:blizzard)
	Comment 1 • 14 years ago

See also:

http://www.mail-archive.com/public-webapps@w3.org/msg05877.html

	Christopher Blizzard (:blizzard)
	Comment 2 • 14 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 14 years ago

This is totally not ready; see the XXX comment about ownership model and such.  But it makes the square in that testcase animate!

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 5 • 14 years ago

Looks good!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 14 years ago

I made |time| 64-bit so that I can actually store an epoch time in ms there for beforepaint events.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 14 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 10 • 14 years ago

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!

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 11 • 14 years ago

We need to figure out how to evangelize this ... to get JQuery to support it, etc

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 12 • 14 years ago

We also need someone to write a more formal spec proposal.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 14 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 14 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 14 years ago

As far as jquery goes, ccing jresig.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 14 years ago

I looked over what the plug-in code does with .time, and there shouldn't be any problems.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 18 • 14 years ago

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.

	Joe Drew (not getting mail)
	Comment 19 • 14 years ago

Comment on attachment 464832 [details] [diff] [review]
Roll-up patch for approval

approved with at least some basic testing.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 14 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 14 years ago

And this got backed out.  NOTE: use the changesets from comment 20, NOT the attached patches, when relanding.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 22 • 14 years ago

Part 1: http://hg.mozilla.org/mozilla-central/rev/bf2386d0cc1b
Part 2: http://hg.mozilla.org/mozilla-central/rev/96de52e4a442

	Michael Wu [:mwu]
	Comment 23 • 14 years ago

Seems to have random orange. Bug 586900

	Eric Shepherd [:sheppy]
	Comment 24 • 14 years ago

Documented:

https://developer.mozilla.org/en/DOM/Animations_using_MozBeforePaint

https://developer.mozilla.org/en/DOM/window.mozRequestAnimationFrame
https://developer.mozilla.org/en/DOM/window.mozAnimationStartTime
https://developer.mozilla.org/en/DOM/window.onmozbeforepaint

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 25 • 14 years ago

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.

	Eric Shepherd [:sheppy]
	Comment 26 • 14 years ago

I've updated it; thanks!

	Avi Halachmi (:avih)
	Comment 27 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 11 years ago

We should file a bug to remove the prefixed version, imo, and remove it.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 29 • 11 years ago

I agree. We certainly shouldn't be wasting time fixing the prefixed version.

	Avi Halachmi (:avih)
	Comment 30 • 11 years ago

Filed bug 909154 to consider removing support for the prefixed mozRequestAnimationFrame.

	Avi Halachmi (:avih)
	Comment 31 • 11 years ago

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.