Closed Bug 985488 Opened 11 years ago Closed 10 years ago

The Canvas debugger should handle cases where there are no animation loop

Categories

(DevTools Graveyard :: Canvas Debugger, defect)

Product:
DevTools Graveyard
Component:
Canvas Debugger
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: pbro, Assigned: jsantell)

References

Details

Attachments

(2 files, 2 obsolete files)

canvasdebuggerenhancement.gif
522.56 KB, image/gif
Details
985488-canvas-debugger-refactor.patch
96.91 KB, patch
jsantell
: review+
Details | Diff | Splinter Review 
STR:

- Go to http://codepen.io/captainbrosset/full/lHpnK
- Open the canvas debugger
- Hit the record button

==> A snapshot appears, with the label "loading..." that never goes away
==> The record button remains in active state
==> Clicking on the snapshot produces errors in the console:

*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: screenshot is null
Full stack: CallsListView<.showScreenshot@chrome://browser/content/devtools/canvasdebugger.js:697:1
SnapshotsListView<._onSelect/<@chrome://browser/content/devtools/canvasdebugger.js:324:7
TaskImpl_run@resource://gre/modules/Task.jsm:282:1
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:118:11
then@resource://gre/modules/commonjs/sdk/core/promise.js:43:43
resolve@resource://gre/modules/commonjs/sdk/core/promise.js:185:11
setTimeout_timer@resource://gre/modules/Timer.jsm:30:5

*************************

We should somehow find a way to handle these cases and let the user know there's no animation in the page.
Depends on: 917226
See Also: → 978948
I think some things would help this:

* Change the message "Loading..." to something like "Waiting for a requestAnimationFrame call", which indicates that rAF is needed for this, as well as that the tools aren't "loading" anything.
* After 5-10s, stop the actor (need a new method on the actor), and display a message saying "no requestAnimationFrame loops found" or something.

This clarifies the restrictions, and explains how it works a bit.

Thoughts?
Flags: needinfo?(vporof)
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Flags: needinfo?(vporof)
Blocks: 1134876
Here's a little demo of what I got cookin'.
Depends on: 1122766
Sorry that this is huge, Victor.
Attachment #8566889 - Flags: review?(vporof)
The text for this should change in lieu of bug 978948, supporting setTimeout.

Should we explicitly say "waiting for requestAnimationFrame/setTimeout cycle"? Or just not mention setTimeout at all? Two messages need to be considered, the "recording" state, as well as the "failed" state, which can contain more debugging information (Detected no setTimeout/rAF cycles, maybe make the timeout a pref and mention to increase that if needed?)
Comment on attachment 8566889 [details] [diff] [review]
985488-canvas-debugger-refactor.patch

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

NIIIICE

::: browser/devtools/canvasdebugger/callslist.js
@@ +1,1 @@
> +/**

use strict.

::: browser/devtools/canvasdebugger/snapshotslist.js
@@ +1,1 @@
> +/**

use strict?

@@ +94,5 @@
> +
> +  /**
> +   * Removes the last snapshot added, in the event no requestAnimationFrame loop was found.
> +   */
> +  removeSnapshot: function () {

s/removeSnapshot/removeLastSnapshot/

::: browser/devtools/canvasdebugger/test/doc_no-canvas.html
@@ +8,5 @@
> +    <title>Canvas inspector test page</title>
> +  </head>
> +
> +  <body>
> +  </body>

Would be cool to have a few tests where there's a setTimeout and/or rAF and no canvas as well. Ideally, this case will still show 'waiting for a canvas etc..'

::: browser/devtools/canvasdebugger/test/head.js
@@ +39,5 @@
>  waitForExplicitFinish();
>  
>  let gToolEnabled = Services.prefs.getBoolPref("devtools.canvasdebugger.enabled");
>  
> +gDevTools.testing = true;

I giggle every time I see this.
Attachment #8566889 - Flags: review?(vporof) → review+
Blocks: 1135403

        Attachment #8566889 -
        Attachment is obsolete: true

        Attachment #8567545 -
        Flags: review+
Blocks: 978948

    
    

    
  
Made bug 1135403 to check the scenario of rAF/sT existing but no canvas/render calls
No longer blocks: 978948
Depends on: 978948

    
    

    
  


  
rebased

        Attachment #8567545 -
        Attachment is obsolete: true

        Attachment #8568340 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/acf0699ae844
Status: ASSIGNED → RESOLVED
Closed: 10 years ago

          status-firefox39:
          --- → fixed
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: