909154 - Consider removing support for the prefixed mozRequestAnimationFrame

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Avi Halachmi (:avih)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Avi Halachmi (:avih)]

(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?

Comment 3

[Avi Halachmi (:avih)]

(Didn't mean to add the CC, I know bz doesn't like it)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the prefixed mozRequestAnimationFrame and its accoutrements

Ben, let me know whether you're comfortable reviewing this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to khuey's window changes

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1e94ccb9df

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a1e94ccb9df

Comment 13

[Nicholas Nethercote [inactive]]

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

[Nicholas Nethercote [inactive]]

(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

[Nicholas Nethercote [inactive]]

(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

[Jean-Yves Perrier [:teoli]]

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

Comment 17

[Kohei Yoshino]

The site compatibility docs is here: https://www.fxsitecompat.com/en-US/docs/2015/mozrequestanimationframe-and-related-apis-have-been-removed/