Closed Bug 978948 Opened 10 years ago Closed 9 years ago

Support setTimeout as animation generators in the Canvas Debugger

Categories

(DevTools Graveyard :: Canvas Debugger, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: vporof, Assigned: jsantell)

References

Details

Attachments

(1 file, 3 obsolete files)

We should do something about these ancient ways of animating things. At the very least, displaying a huge red flashing banner PLEASE DON'T USE SETINTERVAL FOR ANIMATION should do the trick :)
Depends on: 917226
(In reply to Victor Porof [:vporof][:vp] from comment #0)
> We should do something about these ancient ways of animating things. At the
> very least, displaying a huge red flashing banner PLEASE DON'T USE
> SETINTERVAL FOR ANIMATION should do the trick :)

doesn't have to be flashing. A red notification would work though.
Attached patch wip (obsolete) — Splinter Review
Shelving a very wip patch.
Component: Developer Tools → Developer Tools: Canvas Debugger
See Also: → 985488
Attachment #8384861 - Attachment is obsolete: true
Do we have any intention of supporting this, especially once we make clear that it's recording a rAF frame (bug 985488)? Can also provide an even more explicit explanation of "Don't use setTimeout/setInterval *link*"
Flags: needinfo?(vporof)
I'd like us to support it. There's a lot of demos on the internet that *STILL* use setInterval. However, we should make it absolutely clear that it's a Bad Thing and the developers should feel Bad.
Flags: needinfo?(vporof)
Gonna take a look at this.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Summary: Support setInterval and setTimeout as animation generators in the Canvas Debugger → Support setTimeout as animation generators in the Canvas Debugger
Attached patch 978948-canvas-settimeout.patch (obsolete) — Splinter Review
As per discussion in IRC, changing this to only support setTimeout at this time, as it'll be more common than setInterval due to the popular rAF shim[0]that uses it, and similarities it shares to rAF in function signature.

This'll need to be rebased hard with the other canvas debugger patches. A few things are built out, as if the rAF/setTimeout paths are different, but those can be merged all into one ANIMATION_GENERATOR array, as their implementations are identical, just a matter of taste splitting them up in the actor.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4deecf570828

[0] http://www.paulirish.com/2011/requestanimationframe-for-smart-animating/
Attachment #8567210 - Flags: review?(vporof)
Comment on attachment 8567210 [details] [diff] [review]
978948-canvas-settimeout.patch

Review of attachment 8567210 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/canvas.js
@@ +335,5 @@
>        return;
>      }
> +    // Handle animations generated using setTimeout. While using
> +    // those timers is considered extremely poor practice, they're still widely
> +    // used on the web, especially for old demos; it's nice to support them as well.

Update the comment for recordAnimationFrame too
Attachment #8567210 - Flags: review?(vporof) → review+
Attached patch 978948-canvas-settimeout.patch (obsolete) — Splinter Review
Updated. Waiting for bug 985488 to land for rebasing
Attachment #8567210 - Attachment is obsolete: true
Attachment #8567530 - Flags: review+
Depends on: 985488
No longer depends on: 985488
Blocks: 985488
Rebased
Attachment #8567530 - Attachment is obsolete: true
Attachment #8567688 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c2c0f42ecb00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.