Last Comment Bug 753453 - requestAnimationFrame callback should return DOMHighResTimeStamp
: requestAnimationFrame callback should return DOMHighResTimeStamp
Status: RESOLVED FIXED
[games:p1][See comment 20 for dev-doc...
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 862629 866402 866545
Blocks: gecko-games 704063
  Show dependency treegraph
 
Reported: 2012-05-09 11:38 PDT by Martin Best (:mbest)
Modified: 2013-08-29 03:51 PDT (History)
17 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Add a way for us to pass a high-res timestamp to requestAnimationFrame callbacks. (12.43 KB, patch)
2013-04-18 01:04 PDT, Boris Zbarsky [:bz]
roc: review+
Details | Diff | Splinter Review

Description Martin Best (:mbest) 2012-05-09 11:38:51 PDT
Do you know if we plan to update the requestAnimationFrame callback to
return a DOMHighResTimeStamp (http://www.w3.org/TR/hr-time/) instead of
a DOMTimeStamp? The spec says it should be a DOMTimeStamp but it makes
sense to switch to DOMHighResTimeStamp while it's still prefixed.

Chrome Canary just updated with DOMHighResTimeStamp being returned in
the requestAnimationFrame callback. I imagine this will screw up some
existing behaviour but it will surely be less painful to switch now than
it would be to do it later.
Comment 1 Boris Zbarsky [:bz] 2012-05-09 12:00:26 PDT
This would be pretty straightforward, right?

The only questions are:

1) Whether it would make sense to do this independently of switching to a monotonic clock.
2) Whether JS_Now ever in fact returns non-integer numbers of ms.

This last should be easy to test, of course.  Especially if someone writes a patch.  I'm happy to review.  ;)
Comment 2 Dietrich Ayala (:dietrich) 2012-06-05 13:31:43 PDT
This doesn't block shipping of B2G V1, so minusing. Re-nom for K9o if you think it's an overall blocker for that.
Comment 3 Martin Best (:mbest) 2012-06-05 13:57:14 PDT
The reason that I put this up is that micro second timers have landed and we haven't updated the requestAnimationFrame return value use the new resolution.  Chrome has done so already.  This is a relatively minor change and we should do this fix before this feature is widely used.

Change your mind on the flag?
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-05 14:06:26 PDT
What do you mean by microsecond timers have landed?  Last I checked we were still at multiple-ms timer latency on Windows.
Comment 5 Boris Zbarsky [:bz] 2012-06-05 14:27:06 PDT
> This is a relatively minor change

It's not quite minor, actually.  For one thing, the 0 values are different, so the change is a compat-breaking change.

What's needed to fix this bug is:

1)  Fixing mozAnimationStartTime to return a DOMHighResTimeStamp
2)  Changing the argument passed to the callback to a DOMHighResTimeStamp
3)  Auditing all in-tree callers of mozRequestAnimationFrame to make sure they're not
    comparing the values to Date.now.
4)  Reorganizing the entire setup for how we call requestAnimationFrame callbacks to make
    all this work, because different callbacks will need to be passed different time
    values (since the zero point of DOMHighResTimeStamp depends on the document, and we
    share refresh drivers across multiple documents).
5)  As #3, but for the web and extensions.  The IE folks say the web is not a big problem
    here, which is good if true.

Depending on the status of extensions, it might make the most sense to do this change as part of unprefixing the API (and leave the prefixed version around with the old behavior for a bit, possibly).
Comment 6 Martin Best (:mbest) 2012-06-05 14:51:53 PDT
Here is the bug I have been tracking for Timer status: 673105

I see your point about it not being as straight forward as it seems on the surface.  Currently my understanding it that we are still only using mozRequestAnimationFrame rather than requestAnimationFrame so if we are going to make this change, we should do it before the unprefixed version lands as you suggest.  The idea of leaving the prefixed version using the old timers is fine with me if that helps to not break the web.  

Starting to feel like we are getting a clearer picture.  To the original question, does the prefixing need to happen for basecamp or can it wait?
Comment 7 :Ehsan Akhgari (away Aug 1-5) 2012-08-15 10:30:40 PDT
What's the status of this bug?
Comment 8 Boris Zbarsky [:bz] 2012-08-15 23:02:45 PDT
Status is someone needs to do what comment 5 says...
Comment 9 Boris Zbarsky [:bz] 2013-01-11 22:36:58 PST
> so the change is a compat-breaking change.

As a data point, this exact change broke sites in Chrome.  See http://code.google.com/p/chromium/issues/detail?id=158910
Comment 10 Boris Zbarsky [:bz] 2013-01-11 22:41:30 PST
And given the goings-on over there, I'm _really_ glad we didn't do it in May.  Back then there weren't even patches to GWT that people could have deployed...

An open question is whether we should only update the unprefixed version when we do update, by the way.
Comment 11 Thomas Broyer 2013-01-20 14:05:29 PST
Back in May, there was no *patch* to GWT that people could have deployed but there was (and still is) a way to disable the use of requestAnimationFrame and only use setTimeout for animations.

And for those running off trunk, the patch came at the end of May: https://code.google.com/p/google-web-toolkit/source/detail?r=10989 It shipped in a RC mid-july and in GA in October: 3 months before the change reaches Chrome's Stable channel.

The problems are:
 - we (GWT) should probably haven't used prefixed APIs, at least without an easy (I mean, easier than it is already) and well-documented switch to turn it off (or possibly turned off by default); we're going to fix that (add a well-documented global switch, possibly turned off by default)
 - people don't update their apps to the latest GWT version fast enough and/or don't test with non-stable browsers (they'd have caught it 6 weeks earlier, before it impacted their users); this is unfortunately not going to change: most GWT apps are "Enterprisey", with lots of bureaucracy and "if it ain't broke don't fix it" culture (no preventive maintenance, continuous testing of apps with beta versions of browsers)
Comment 12 Boris Zbarsky [:bz] 2013-01-20 15:26:12 PST
Thomas, just to make sure I understand the current GWT status...

We have two options for this bug:

1)  Make requestAnimationFrame pass in a DOMHighResTimeStamp while mozRequestAnimationFrame continues to have the behavior it has now.

2)  Make both pass in a DOMHighResTimeStamp.

Which one is more likely to work correctly with GWT?  Or does it not matter?
Comment 13 Thomas Broyer 2013-01-20 15:46:00 PST
Changing mozRequestAnimationFrame's behavior will have the same effect as for Chrome: people using GWT 2.5 (or trunk after r10989) won't see a difference, this is because we don't use the callback's argument, we always '(new Date()).getTime()' to compute the animations' progress. Previously (in GWT 2.4), we used the callback's argument as a DOMTimeStamp for both webkitRAF and mozRAF.

GWT doesn't yet use the unprefixed rAF but the patch is in review. It uses the same approach as for mozRAF: don't rely on the return value of rAF –i.e. don't use cancelAnimationFrame, set a flag that's checked in the callback instead– and don't use the callback's argument.

You can thus safely change rAF: not yet in GWT, will be immune to the change anyway when added.
Changing mozRAF will break everyone not using GWT 2.5+, just like for Chrome. The workaround is similar (disable the use of mozRAF, use setTimeout instead). I therefore don't think it's risky: because Chrome crossed the line before Firefox, people already know how to deal with it (if they only applied the workaround) or have already upgraded to GWT 2.5 and thus won't notice the change.
Comment 14 Boris Zbarsky [:bz] 2013-01-22 12:27:09 PST
Thomas, thanks for the data!
Comment 15 Brian Slesinsky 2013-01-22 12:30:33 PST
By the way, *removing* mozRequestAnimationFrame would not break GWT 2.4 (much) since it will just fall back on a timer-based implementation. I suspect that's true in general since most JavaScript code has a fallback for a missing vendor-prefixed symbol, but can't deal with arbitrary changes in behavior since those can't be predicted or tested in advance.

So for completeness, a third possibility if you're not ready to unprefix it would be to provide the new behavior in mozRequestAnimationFrame2 and (optionally) delete the old name. For GWT we'd rather this didn't happen since it will cause GWT 2.5 and trunk to fall back to timers as well, and I'd rather have something else in place first.

Whatever you decide, I'd like to know what to put in the patch so that GWT 2.5.1 and trunk will be able to use requestAnimationFrame on Firefox. The (incomplete) patch is here:
https://gwt-review.googlesource.com/#/c/1780/
Comment 16 Boris Zbarsky [:bz] 2013-01-23 06:38:47 PST
Brian,

I don't think we have any plans to do mozRequestAnimationFrame2.  

We also have no plans to do an unprefixed requestAnimationFrame without fixing this bug first.

The plan is to fix this bug and unprefix at the same time; the only thing I'm still undecided on is what to do with mozRequestAnimationFrame when I do that.

Does that help?  Your GWT patch to use unprefixed requestAnimationFrame else fall back to timers looks reasonable to me, for what it's worth...
Comment 17 Thomas Broyer 2013-01-27 07:47:48 PST
FWIW, Google reverted their change to the prefixed version: http://code.google.com/p/chromium/issues/detail?id=158910
Comment 18 Boris Zbarsky [:bz] 2013-04-18 01:04:09 PDT
Created attachment 738917 [details] [diff] [review]
Add a way for us to pass a high-res timestamp to requestAnimationFrame callbacks.
Comment 19 Boris Zbarsky [:bz] 2013-04-18 01:07:00 PDT
So I'm going to take the same approach Chrome took.  In this bug I'll implement the infrastructure for passing a DOMHighResTimeStamp to requestAnimationFrame callbacks.  In bug 704063 I'll add a window.requestAnimationFrame that will make use of that.  The behavior of window.mozRequestAnimationFrame and mozAnimationStartTime is not going to change.
Comment 20 Boris Zbarsky [:bz] 2013-04-25 08:18:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/bff130404258

So to recap the behavior, for dev-doc purposes:

* mozRequestAnimationFrame passes an epoch-based time to callbacks.  The passed-in value
  has millisecond precision and can be compared to mozAnimationStartTime.
* requestAnimationFrame passes a DOMHighResTimeStamp to callbacks.  It has microsecond
  precision and can be compared to performance.now(), kinda.
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-04-25 18:57:40 PDT
https://hg.mozilla.org/mozilla-central/rev/bff130404258
Comment 22 Kohei Yoshino [:kohei] 2013-05-17 13:11:45 PDT
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23
Comment 23 Boris Zbarsky [:bz] 2013-05-17 13:51:56 PDT
That looks great; thank you!

Note You need to log in before you can comment on or make changes to this bug.