Support setTimeout as animation generators in the Canvas Debugger

RESOLVED FIXED in Firefox 39

Status

()

Firefox
Developer Tools: Canvas Debugger
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vporof, Assigned: jsantell)

Tracking

unspecified
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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 :)
(Reporter)

Updated

4 years ago
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.
(Reporter)

Comment 2

4 years ago
Created attachment 8384861 [details] [diff] [review]
wip

Shelving a very wip patch.
(Reporter)

Updated

4 years ago
Component: Developer Tools → Developer Tools: Canvas Debugger
See Also: → bug 985488
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1040883
(Reporter)

Updated

3 years ago
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)
(Reporter)

Comment 5

3 years ago
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
(Assignee)

Updated

3 years ago
Summary: Support setInterval and setTimeout as animation generators in the Canvas Debugger → Support setTimeout as animation generators in the Canvas Debugger
Created attachment 8567210 [details] [diff] [review]
978948-canvas-settimeout.patch

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)
(Reporter)

Comment 8

3 years ago
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+
Created attachment 8567530 [details] [diff] [review]
978948-canvas-settimeout.patch

Updated. Waiting for bug 985488 to land for rebasing
Attachment #8567210 - Attachment is obsolete: true
Attachment #8567530 - Flags: review+
(Assignee)

Updated

3 years ago
Blocks: 1135403
(Assignee)

Updated

3 years ago
Depends on: 985488
(Assignee)

Updated

3 years ago
No longer depends on: 985488
(Assignee)

Updated

3 years ago
Blocks: 985488
Created attachment 8567688 [details] [diff] [review]
978948-canvas-settimeout.patch

Rebased
Attachment #8567530 - Attachment is obsolete: true
Attachment #8567688 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c2c0f42ecb00
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c2c0f42ecb00
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.