Closed
Bug 909154
Opened 11 years ago
Closed 10 years ago
Consider removing support for the prefixed mozRequestAnimationFrame
Categories
(Core :: Layout, defect, P1)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: avih, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [evang-wanted])
Attachments
(1 file, 1 obsolete file)
17.45 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
Following recent failures of the mozRequestAnimationFrame test (bug 858737), bz and roc suggested to remove support for the prefixed version.
The prefixed and unprefixed versions behave slightly differently, especially (only?) in their callback argument. See bug 569520 comment 27 onwards.
The factors to consider here are probably modifying all our code which uses the prefixed version (76 instances on m-c, not including other trees) and make sure the change in behavior is handled, if applicable.
Other than this, there's the issue of pages which only use the prefixed version.
We could alias it to the unprefixed version, but this could break pages which support both but expect them to behave differently (probably most notably older GWT apps - see bug 753453 comment 11 and others).
Assignee | ||
Comment 1•11 years ago
|
||
> Other than this, there's the issue of pages which only use the prefixed version.
I doubt there are many: the common pattern is to try the unprefixed version, then the prefixed ones, then fall back to setTimeout.
If we're worried about this, we could try telemetry to see how often the unprefixed version is even called nowadays...
A possibly-bigger problem is addons; if we care we can leave the prefixed version on ChromeWindow (possibly aliased to the unprefixed one, at that point).
Reporter | ||
Comment 2•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1)
> A possibly-bigger problem is addons; if we care we can leave the prefixed
> version on ChromeWindow (possibly aliased to the unprefixed one, at that
> point).
If this is indeed possibly a bigger problem which will make us consider aliasing, how about the following which, I think, would satisfy all the concerns raised so far:
1. We change the prefixed version to be a thin wrapper around the non prefixed version.
2. mozAnimationStartTime becomes the timestamp where performance.now() is 0 and is set once per page.
3. mozRequestAnimationFrame returns the arg from normal rAF + mozAnimationStartTime.
4. Cancel (does the prefixed version exists?) is a plain alias.
So we gain:
1. Remove the legacy code and be left with hopefully a very thin wrapper only.
2. Satisfy whoever expect the prefixed version to exist and its callback arg be time based.
3. No need to worry about addons or legacy pages or old GWT apps, while still improving functionality for them by providing a monotonic callback arg.
Or maybe we actually want to remove legacy APIs, not because it's broken and not worth the fixing effort but rather to just remove API cruft?
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 3•11 years ago
|
||
(Didn't mean to add the CC, I know bz doesn't like it)
Updated•11 years ago
|
Keywords: dev-doc-complete → dev-doc-needed
Assignee | ||
Comment 4•11 years ago
|
||
So I just glanced through https://mxr.mozilla.org/addons/search?string=mozrequestanimationframe&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
Some of the uses are window.mozRequestAnimationFrame || window.requestAnimationFrame.
Some are including jQuery.
A very few are using only-prefixed mozRequestAnimationFrame but not looking at the function arg.
I do think we want to remove the old code here if possible, and ideally remove the old API. Jorge, is it reasonable to just ask people to update their addons here, or do we need a deprecation period of some sort?
Flags: needinfo?(bzbarsky) → needinfo?(jorge)
Comment 5•11 years ago
|
||
I suspect there are very few add-ons that will be affected by this change, so I think it's reasonable just to ask people to update.
Flags: needinfo?(jorge)
Keywords: addon-compat
Updated•11 years ago
|
Keywords: site-compat
Assignee | ||
Comment 6•10 years ago
|
||
> I suspect there are very few add-ons that will be affected by this change, so I think
> it's reasonable just to ask people to update.
OK. So I took a look at addons mxr. Just searching for "mozRequestAnimationFrame" is not helpful because there are tons of addons that use the addons SDK, and that has a file (content-proxy.js) that has this string in a comment. :( Also addons including jQuery and the like, which use this API if present but work fine without it.
There are _some_ addons using mozAnimationStartTime and a few using mozCancelAnimationFrame. Some of these are, again, using libraries, but some will need to be updated.
Anyway, I guess I'll try to just do this and we see how things go.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 7•10 years ago
|
||
Ben, let me know whether you're comfortable reviewing this?
Attachment #8635083 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8635127 -
Flags: review?(bkelly)
Assignee | ||
Updated•10 years ago
|
Attachment #8635083 -
Attachment is obsolete: true
Attachment #8635083 -
Flags: review?(bkelly)
Comment 9•10 years ago
|
||
Comment on attachment 8635127 [details] [diff] [review]
Merged to khuey's window changes
Review of attachment 8635127 [details] [diff] [review]:
-----------------------------------------------------------------
I think you might have missed a few accoutrements:
- nsIFrameRequestCallback.idl and its moz.build entry
- toolkit/content/widgets/browser.xml references nsIFrameRequestCallback interface
- dom/locales/en-US/chrome/dom/dom.properties references mozRequestAnimationFrame
- same with webapprt/locales/en-US/webapprt/overrides/dom.properties
- js/src/jit-test/lib/bullet.js references mozRequestAnimationFrame
- toolkit/devtools/server/actors/canvas.js references mozRequestAnimationFrame
- some other 3rd party libraries reference it, but we probably don't need to change those
None of that looks particularly contentious, though. So r=me with action items addressed.
::: dom/base/nsDocument.cpp
@@ +10320,5 @@
> MOZ_ASSERT(!mOriginalDocument);
> }
>
> nsresult
> +nsIDocument::ScheduleFrameRequestCallback(FrameRequestCallback& aCallback,
Any reason you dropped const here?
::: dom/base/nsGlobalWindow.cpp
@@ -5485,5 @@
> return rv.StealNSResult();
> }
>
> -NS_IMETHODIMP
> -nsGlobalWindow::MozRequestAnimationFrame(nsIFrameRequestCallback* aCallback,
Remove nsIFrameRequestcallback.h include as well.
::: dom/interfaces/base/nsIDOMWindow.idl
@@ -458,5 @@
> * @see <http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/RequestAnimationFrame/Overview.html>
> */
> - // Argument is optional only so we can warn when it's null
> - long
> - mozRequestAnimationFrame([optional] in nsIFrameRequestCallback aCallback);
You can remove the nsIFrameRequestCallback forward declaration on line 8.
::: layout/base/nsRefreshDriver.cpp
@@ +1565,5 @@
> // else window is partially torn down already
> }
> + for (auto& callback : docCallbacks.mCallbacks) {
> + ErrorResult ignored;
> + callback->Call(timeStamp, ignored);
I think you need ignored.SuppressException() to avoid asserting if the callback throws an exception, right?
I guess the generated code for FrameRequestCallback only throws things like NS_ERROR_UNEXPECTED, so its not needed now. Seems like it might be nice to be safe, though.
Attachment #8635127 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 10•10 years ago
|
||
> I think you might have missed a few accoutrements:
Oh, nice catch!
> - nsIFrameRequestCallback.idl and its moz.build entry
> - toolkit/content/widgets/browser.xml references nsIFrameRequestCallback interface
> - dom/locales/en-US/chrome/dom/dom.properties references mozRequestAnimationFrame
> - same with webapprt/locales/en-US/webapprt/overrides/dom.properties
All removed. In fact, I can remove the nsIDocument::eMozBeforePaint thing too, and have done so.
> - js/src/jit-test/lib/bullet.js references mozRequestAnimationFrame
Yep. This is a third-party library that already prefers the unprefixed version, so I just didn't bother removing it from here.
> - toolkit/devtools/server/actors/canvas.js references mozRequestAnimationFrame
Good catch. This part needs to go.
> - some other 3rd party libraries reference it, but we probably don't need to change
> those
Yeah, indeed.
> Any reason you dropped const here?
Yes. The old thing was a callback holder, which was basically a struct storing an nsRefPtr<FrameRequestCallback> in it (in addition to some other stuff). The new thing is just a FrameRequestCallback reference. The callee here needs to save the FrameRequestCallback, so needs to addref it. That was totally possible when the arg was "const nsRefPtr<FrameRequestCallback>&" because the "operator T*" on nsRefPtr returns a non-const thing. But in the new setup we need to drop the const, since AddRef is clearly a non-const method. ;)
> Remove nsIFrameRequestcallback.h include as well.
> You can remove the nsIFrameRequestCallback forward declaration on line 8.
Done.
> I think you need ignored.SuppressException() to avoid asserting if the callback throws
> an exception, right?
As you noticed, only if certain types of exceptions are throw. But yeah, can't hurt to add it. Done.
Thank you for the thorough review!
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 13•9 years ago
|
||
This broke the Tree Style Tab extension -- I bisected the breakage down to this patch.
See https://github.com/piroor/treestyletab/issues/919 for details.
Comment 14•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #13)
> This broke the Tree Style Tab extension -- I bisected the breakage down to
> this patch.
>
> See https://github.com/piroor/treestyletab/issues/919 for details.
Update: looks like it's been fixed in the git repo, but finding an updated .xpi build that has the fix isn't easy. But http://piro.sakura.ne.jp/xul/xpi/nightly/treestyletab.xpi appears to be working.
Comment 15•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #14)
> But http://piro.sakura.ne.jp/xul/xpi/nightly/treestyletab.xpi appears to be
> working.
Actually it's not. I wrote more about this here:
https://blog.mozilla.org/nnethercote/2015/07/28/a-work-around-for-tree-style-tab-breakage-on-firefox-nightly-caused-by-mozrequestanimationframe-removal/
Comment 16•9 years ago
|
||
Added a comment in the compat table of:
https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame
and updated:
https://developer.mozilla.org/en-US/Firefox/Releases/42#Miscellaneous
Keywords: dev-doc-needed → dev-doc-complete
Comment 17•9 years ago
|
||
The site compatibility docs is here: https://www.fxsitecompat.com/en-US/docs/2015/mozrequestanimationframe-and-related-apis-have-been-removed/
You need to log in
before you can comment on or make changes to this bug.
Description
•