Last Comment Bug 876282 - Add unprefixed cancelAnimationFrame
: Add unprefixed cancelAnimationFrame
Status: RESOLVED FIXED
[parity-IE][parity-webkit]
: dev-doc-complete, regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on:
Blocks: unprefix 704063
  Show dependency treegraph
 
Reported: 2013-05-26 12:51 PDT by Tom Schuster [:evilpie]
Modified: 2013-05-30 00:48 PDT (History)
19 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed


Attachments
Patch v1 (1.72 KB, patch)
2013-05-28 06:15 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v1.1 (3.53 KB, patch)
2013-05-28 06:21 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Patch for aurora (7.92 KB, patch)
2013-05-28 13:14 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Tom Schuster [:evilpie] 2013-05-26 12:51:30 PDT
requestAnimationFrame was unprefixed in Bug 704063.
Comment 1 Masatoshi Kimura [:emk] 2013-05-26 15:38:00 PDT
Huh, only unprefixed requestAnimationFrame has been added without adding cancalAnimationFrame!?
Comment 2 Michał Gołębiowski [:m_gol] 2013-05-26 15:56:45 PDT
You're in for a lot of broken sites if you don't backport this change to Firefox 23...
Comment 3 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-27 01:16:03 PDT
Boris, do you agree this needs to go in the same release as unprefixing requestAnimationFrame?  (And do you want to do it, or should we find someone else?)
Comment 4 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-27 20:43:57 PDT
tracking until we get an answer to comment 3 - will untrack if this doesn't have to ship at the same time as requestAnimationFrame.
Comment 5 Michał Gołębiowski [:m_gol] 2013-05-28 03:03:34 PDT
Just to add one reason: the following polyfill from Paul Irish's blog breaks when only requestAnimationFrame is unprefixed:
http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
You can find this polyfill in many places all over the internet, like:
http://my.opera.com/emoller/blog/2011/12/20/requestanimationframe-for-smart-er-animating
https://gist.github.com/paulirish/1579671

This is a really bad idea to unprefix only one of them.
Comment 6 Boris Zbarsky [:bz] 2013-05-28 06:09:42 PDT
This was just an oversight, yeah.  We need to add an unprefixed cancelAnimationFrame.  Luckily, this should be trivial to implement: both the prefixed and unprefixed cancel should have the same behavior.
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2013-05-28 06:15:48 PDT
Created attachment 754791 [details] [diff] [review]
Patch v1

Will need a uuid rev for trunk and probably a new interface for backports
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2013-05-28 06:21:11 PDT
Created attachment 754795 [details] [diff] [review]
Patch v1.1
Comment 9 Boris Zbarsky [:bz] 2013-05-28 07:04:45 PDT
Comment on attachment 754795 [details] [diff] [review]
Patch v1.1

r=me with that uuid rev.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2013-05-28 13:14:22 PDT
Created attachment 754977 [details] [diff] [review]
Patch for aurora
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2013-05-28 13:15:59 PDT
(In reply to :Ms2ger from comment #11)
> Created attachment 754977 [details] [diff] [review]
> Patch for aurora

https://tbpl.mozilla.org/?tree=Try&rev=5427e53156ff
Comment 13 Boris Zbarsky [:bz] 2013-05-28 13:32:45 PDT
Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

r=me
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-05-28 18:43:06 PDT
https://hg.mozilla.org/mozilla-central/rev/e1c63a4904d1
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2013-05-29 00:09:50 PDT
Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 704063
User impact if declined: sites may break
Testing completed (on m-c, etc.): a simpler patch that does change uuids is on m-c. This one passes try.
Risk to taking this patch (and alternatives if risky): pretty low risk. Alternative is unsupporting unprefixed requestAnimationFrame
String or IDL/UUID changes made by this patch: none
Comment 16 Masatoshi Kimura [:emk] 2013-05-29 00:25:43 PDT
(In reply to Michał Gołębiowski from comment #5)
> Just to add one reason: the following polyfill from Paul Irish's blog breaks
> when only requestAnimationFrame is unprefixed:
> http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
> You can find this polyfill in many places all over the internet, like:
> http://my.opera.com/emoller/blog/2011/12/20/requestanimationframe-for-smart-
> er-animating
> https://gist.github.com/paulirish/1579671

Actually those polyfills will work because we didn't remove prefixed mozRequestAnimationFrame, mozCancelAnimationFrame and mozCancelRequestAnimationFrame.
But those polyfills will not use unprefixed functions if prefixed one is present. It is bad because the prefixed version doesn't support high-resolution timers. In short, the polyfills are broken anyway.
Comment 17 Michał Gołębiowski [:m_gol] 2013-05-29 03:26:37 PDT
(In reply to Masatoshi Kimura [:emk] from comment #16)
> Actually those polyfills will work because we didn't remove prefixed
> mozRequestAnimationFrame, mozCancelAnimationFrame and
> mozCancelRequestAnimationFrame.
I'm sorry but you're wrong. Specifically, this polyfill:
https://gist.github.com/paulirish/1579671
would cause Firefox to use unprefixed requestAnimationFrame (the first `for` loop would exit immediately) but cancelAnimationFrame would be defined as a function clearing a timeout that wasn't even set (since it assumes requestAnimationFrame is implemented using timeouts, too).

> But those polyfills will not use unprefixed functions if prefixed one is
> present. It is bad because the prefixed version doesn't support
> high-resolution timers. In short, the polyfills are broken anyway.
You're wrong about that, too. Read the code again.
Comment 18 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-29 03:43:19 PDT
Do we really need to worry about changing UUIDs on Aurora?  I thought we didn't promise stability until beta.  But maybe I'm misremembering.
Comment 19 Boris Zbarsky [:bz] 2013-05-29 05:30:36 PDT
> Do we really need to worry about changing UUIDs on Aurora?

We may not have to, but it's easier to play it safe.
Comment 20 Masatoshi Kimura [:emk] 2013-05-29 05:56:03 PDT
(In reply to Michał Gołębiowski from comment #17)
> (the first `for` loop would exit immediately)

Ah, I overlooked |&& !window.requestAnimationFrame| part, sorry.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-29 12:57:41 PDT
Comment on attachment 754977 [details] [diff] [review]
Patch for aurora

Dbaron: we don't 'worry' about UUIDs on Aurora, but still good to know when there are changes in uplifted patches.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-05-29 14:40:49 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e643d5826ac
Comment 24 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2013-05-30 00:48:41 PDT
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #21)
> Comment on attachment 754977 [details] [diff] [review]
> Patch for aurora
> 
> Dbaron: we don't 'worry' about UUIDs on Aurora, but still good to know when
> there are changes in uplifted patches.

Should we land the other patch on aurora, then, since it doesn't have a (small) codesize, runtime memory use, and performance hit?

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