Build a canvas inspection tool

RESOLVED FIXED in Firefox 31

Status

RESOLVED FIXED
5 years ago
7 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

(Blocks: 2 bugs, {dev-doc-needed})

unspecified
Firefox 31
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 22 obsolete attachments)

3.98 MB, image/png
Details
v21
253.65 KB, patch
vporof
: review+
Details | Diff | Splinter Review
2.32 KB, patch
jryans
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Chrome has a new tool. It's useful. [0]
We should abstract this to work with both webgl and 2d contexts.

[0] http://www.html5rocks.com/en/tutorials/canvas/inspection/
(Assignee)

Comment 1

5 years ago
After bug 910953 lands, the logic (backend) for this should be super easy to implement.
(Assignee)

Updated

5 years ago
Depends on: 910953
Nical tells me that we already have the plumbery to capture those frames and Bas should be a good contact.
(In reply to Anthony Ricaud (:rik) from comment #2)
> Nical tells me that we already have the plumbery to capture those frames and
> Bas should be a good contact.

More precisely, we record Moz2D drawing commands that we can replay. Moz2D is the drawing API we use for canvas 2D (and on some platforms, to draw web content). Bas has made an external tool to replay drawing commands. It is not integrated in firefox though.
See: http://www.basschouten.com/blog1.php/presenting-the-azure-drawing-recorder
(Assignee)

Comment 4

5 years ago
(In reply to Nicolas Silva [:nical] from comment #3)
> (In reply to Anthony Ricaud (:rik) from comment #2)

Do you reckon it would be hard to abstract Moz2D calls in a way that allows a separate similar interface to work with WebGL? (since we don't yet have such inspection methods for a webgl context, overwriting methods is the way to go).

Ideally the resulting tool would be able to inspect both 2D and WebGL contexts in the exact same manner. That is, for example, analyzing the state on each draw call.
(In reply to Victor Porof [:vp] from comment #4)
> (In reply to Nicolas Silva [:nical] from comment #3)
> > (In reply to Anthony Ricaud (:rik) from comment #2)
> 
> Do you reckon it would be hard to abstract Moz2D calls in a way that allows
> a separate similar interface to work with WebGL? (since we don't yet have
> such inspection methods for a webgl context, overwriting methods is the way
> to go).

I am not sure at which level you would see this common interface.
Do you mean a common interface as in "the same interface for JS bindings?"
Moz2D's recorder does not expose any recording feature to JS, nor does WebGL.

bjacob from the gfx team tells me this would be best done at the js level (like you did for the WebGL tool that inspects shaders).

He also underlined that canvas 2d is built on top of Moz2D which means our current recording infrastructure will record the lower level of API calls. A canvas2D call can produce several Moz2D calls and the recorders will record those two (which isn't interesting for web developers). And that's a good point, I spoke abit too fast about the Moz2D recording stuff.

> 
> Ideally the resulting tool would be able to inspect both 2D and WebGL
> contexts in the exact same manner. That is, for example, analyzing the state
> on each draw call.

I would love such a tool.
Can I add that it would be great if it was possible for each draw call to display the current context coordinates system?

For me, in canvas, it's often hard to debug what's going on when you've applied transformations such as rotate or translate. I think it would be pretty useful to have an option, in each draw call, to overlay the 2 X and Y axis, so that you know immediately where's your 0/0 origin and which directions are the 2 axis pointing to.
Oh and, that reminds me that it would also be great to be able to inspect what's the current context styles: strokeSize, fillStyle, ...
(Assignee)

Comment 8

5 years ago
(In reply to Patrick Brosset from comment #7)
> Oh and, that reminds me that it would also be great to be able to inspect
> what's the current context styles: strokeSize, fillStyle, ...

That's an absolute requirement!

(In reply to Patrick Brosset from comment #6)
> Can I add that it would be great if it was possible for each draw call to
> display the current context coordinates system?
> 

And this is a very good idea. Inspecting numbers is boring and useless. Visually conveying this information is much more interesting. Maybe (re)using what you did for CSS transforms? (although that'd require some tweaks to work properly in 3D)
(Assignee)

Comment 9

5 years ago
Created attachment 8377562 [details] [diff] [review]
v1 wip

Crude WIP.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 10

5 years ago
Created attachment 8377912 [details] [diff] [review]
v2 wip

This actually does something useful.
Attachment #8377562 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 8378625 [details] [diff] [review]
v3 wip

Getting there...
Attachment #8377912 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 8379418 [details] [diff] [review]
v4 wip
Attachment #8378625 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 8379916 [details] [diff] [review]
v5 demo
Attachment #8379418 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 8380044 [details] [diff] [review]
v6 (still wip)
Attachment #8379916 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 8380934 [details] [diff] [review]
v7 wip

Added ability to display the stack of each function and jump to the debugger. Also, stepping over/in/out now works, and also syncs with the debugger.
Attachment #8380044 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Here's how going back and forth between the debugger and this tool feels like: http://youtu.be/pcW8Ry5OVA0
This looks amazing Victor!
(In reply to Victor Porof [:vporof][:vp] from comment #16)
> Here's how going back and forth between the debugger and this tool feels
> like: http://youtu.be/pcW8Ry5OVA0

This looks super awesome. In the video, when you step over, it steps over the calls, but often times you pressed resume button and it still stopped after 5-6 calls, was it because you had a breakpoint in debugger for those calls ?

Also, what about WebGL call stepping ?
(Assignee)

Comment 19

5 years ago
* "Resume" goes to the next draw call.
* "Step over" goes over the current context call.
* "Step out" gets out of the animation frame (typically to the next requestAnimationFrame call).
* "Step in" goes to the next non-context call.
Hmm. This makes sense, and I think tooltips on the stepping buttons will make sure that users are aware of the different behavior of similar looking button set.
(Assignee)

Comment 21

5 years ago
Created attachment 8381264 [details] [diff] [review]
v8 wip

Can't sleep.

Cleaned up code a bit, made some perf optimizations, revamped the call stack UI, and added a slider for quickly glancing through calls.
Attachment #8380934 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
Created attachment 8381265 [details]
screenshot
(In reply to Victor Porof [:vporof][:vp] from comment #22)
> Created attachment 8381265 [details]
> screenshot
The UI looks amazing!
(Assignee)

Comment 24

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)

Thank you :)
(Assignee)

Comment 25

5 years ago
This landed: bug 966591. It'd be nice if we were able to debug hit regions using this tool, as easily as:
* drawing individual regions separately, colored differently for each id, or
* showing the hit region id of a pixel when hovering the preview panel using the mouse.
(Assignee)

Comment 26

5 years ago
Created attachment 8382506 [details] [diff] [review]
v9

Documented and localized everything. More UI polish here and there. This should be ready for some feedback. Modulo tests, I believe this is a decent first version of the tool.
Attachment #8381264 - Attachment is obsolete: true
Attachment #8382506 - Flags: feedback?(rcampbell)
Comment on attachment 8382506 [details] [diff] [review]
v9

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

::: toolkit/devtools/content-observer.js
@@ +28,5 @@
> +   * Starts listening for the required observer messages.
> +   */
> +  startListening: function() {
> +    Services.obs.addObserver(
> +      this._onContentGlobalCreated, "content-document-global-created", false);

Note that this notification is not fired when a page is loaded from the BF cache.

For example, with the notification observer attached:
1) go to google.com -> Notification fires (multiple times)
2) Go to any other webpage now, say, nytimes.com -> Notification fires (multiple times)
3) Press browser back button -> No notification.
(Assignee)

Comment 28

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #27)
> Comment on attachment 8382506 [details] [diff] [review]

Does it claim to do so?
Absolutely love it :)

I think we could clean up button icons because reusing the debugger ones but having a slightly different meaning is kinda confusing IMO.

Great work!
(Assignee)

Comment 30

5 years ago
I talked to benwa today and he really liked the buttons' functionality. I think this is just a matter of getting better icons.

Darrin, halp!
Flags: needinfo?(dhenein)
Yeah functionality is superb; was referring to the icons.
(In reply to Victor Porof [:vporof][:vp] from comment #28)
> (In reply to Girish Sharma [:Optimizer] from comment #27)
> > Comment on attachment 8382506 [details] [diff] [review]
> 
> Does it claim to do so?

I really am not sure about how you are using it but this comment

  /**
   * Invoked whenever the current tab actor's document global is created.
   */
  _onGlobalCreated: function(window) {

made me think that you are using it to detect newly added document object in the tab. So I was just giving a heads up about the fact that BF cached documents don't emit that notification and thus your `_onGlobalCreated` method won't be called.
(Assignee)

Updated

5 years ago
Blocks: 978948
Comment on attachment 8382506 [details] [diff] [review]
v9

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

I have no problems with any of this. Lovely WIP patch with many tests!

I do agree that the icons are a little strange given their different meanings here. Would like something specific for this tool.

::: browser/devtools/debugger/debugger.xul
@@ -433,5 @@
>          </tabs>
>          <tabpanels flex="1">
>            <tabpanel id="variables-tabpanel">
>              <vbox id="expressions"/>
> -            <splitter class="devtools-horizontal-splitter devtools-invisible splitter"/>

aha! that's where that gripper was coming from!

if this isn't landing soon we should land it separately.

::: browser/themes/shared/devtools/canvasdebugger.inc.css
@@ +5,5 @@
> +%filter substitution
> +%define darkCheckerboardBackground #000
> +%define lightCheckerboardBackground #fff
> +%define checkerboardCell rgba(128,128,128,0.25)
> +%define checkerboardPattern url(""), linear-gradient(45deg, @checkerboardCell@ 25%, transparent 25%, transparent 75%, @checkerboardCell@ 75%, @checkerboardCell@), linear-gradient(45deg, @checkerboardCell@ 25%, transparent 25%, transparent 75%, @checkerboardCell@ 75%, @checkerboardCell@)

mm. much line.

::: toolkit/devtools/content-observer.js
@@ +28,5 @@
> +   * Starts listening for the required observer messages.
> +   */
> +  startListening: function() {
> +    Services.obs.addObserver(
> +      this._onContentGlobalCreated, "content-document-global-created", false);

-__-

::: toolkit/devtools/server/actors/call-watcher.js
@@ +103,5 @@
> +   */
> +  _generateArgsPreview: function() {
> +    let { caller, args } = this.details;
> +
> +    // XXX: All of this sucks. Make this smarter, so that the frontend

good enough for a v0.1 or does this need a rewrite for landing?

@@ +116,5 @@
> +      if (typeof arg == "object") {
> +        return "Object";
> +      }
> +      if (caller && caller.toString().contains("WebGLRenderingContext")) {
> +        // XXX: This doesn't handle combined bitmasks.

again, blocker for landing or go ahead until you've got a better solution?

::: toolkit/devtools/server/actors/webgl.js
@@ +8,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  const events = require("sdk/event/core");
>  const protocol = require("devtools/server/protocol");
> +const { ContentObserver } = require("devtools/content-observer");

this gets its own place now? keen.
Attachment #8382506 - Flags: feedback?(rcampbell) → feedback+
(Assignee)

Comment 34

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #33)
> Comment on attachment 8382506 [details] [diff] [review]
> v9
> 

Sweet! Thanks for the feedback!

> Review of attachment 8382506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have no problems with any of this. Lovely WIP patch with many tests!
> 
> I do agree that the icons are a little strange given their different
> meanings here. Would like something specific for this tool.
> 

Definitely. I asked Darrin for icons.

> 
> ::: toolkit/devtools/server/actors/call-watcher.js
> @@ +103,5 @@
> > +   */
> > +  _generateArgsPreview: function() {
> > +    let { caller, args } = this.details;
> > +
> > +    // XXX: All of this sucks. Make this smarter, so that the frontend
> 
> good enough for a v0.1 or does this need a rewrite for landing?
> 

Nah, this is good enough for a v1. I don't want to spend too much time on it yet.

> @@ +116,5 @@
> > +      if (typeof arg == "object") {
> > +        return "Object";
> > +      }
> > +      if (caller && caller.toString().contains("WebGLRenderingContext")) {
> > +        // XXX: This doesn't handle combined bitmasks.
> 
> again, blocker for landing or go ahead until you've got a better solution?
> 

Definitely not a blocker. I'll add bug numbers in the comments.
(Assignee)

Updated

5 years ago
Blocks: 978957
(Assignee)

Updated

5 years ago
Blocks: 978960
(Assignee)

Updated

5 years ago
Blocks: 978964
(Assignee)

Comment 35

5 years ago
Created attachment 8384857 [details] [diff] [review]
v10

Addressed comments, finished the import/export mechanism, etc. Just a few more small frontend tests, and this should be ready to land.
Attachment #8382506 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 978969
(Assignee)

Updated

5 years ago
Blocks: 978973
(Assignee)

Updated

5 years ago
No longer blocks: 978973
Flags: needinfo?(dhenein)
(Assignee)

Updated

5 years ago
Blocks: 978973
(Assignee)

Updated

5 years ago
Component: Developer Tools → Developer Tools: Canvas Debugger
(Assignee)

Comment 36

5 years ago
Created attachment 8385622 [details] [diff] [review]
v11

Finishing touches.
Attachment #8381265 - Attachment is obsolete: true
Attachment #8384857 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 979925
(Assignee)

Comment 37

5 years ago
Created attachment 8386215 [details] [diff] [review]
v12

Searchbox now works properly.
Attachment #8385622 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8381265 - Attachment is obsolete: false
(Assignee)

Comment 38

5 years ago
Preliminary try run looks green: https://bugzilla.mozilla.org/show_bug.cgi?id=917226
(Assignee)

Comment 39

5 years ago
Created attachment 8387681 [details] [diff] [review]
v13

THIRTEEN YES.
Attachment #8386215 - Attachment is obsolete: true
Attachment #8387681 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Blocks: 981182
(Assignee)

Updated

5 years ago
Blocks: 981183
(Assignee)

Updated

5 years ago
Blocks: 981184
(Assignee)

Updated

5 years ago
Blocks: 981185
(Assignee)

Comment 40

5 years ago
Created attachment 8387958 [details] [diff] [review]
v14

Fixed a few papercuts after a testing session today with Kannan.
Attachment #8387681 - Attachment is obsolete: true
Attachment #8387681 - Flags: review?(rcampbell)
Attachment #8387958 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Blocks: 981303
(Assignee)

Comment 42

5 years ago
Created attachment 8388122 [details] [diff] [review]
v15 (last stable version)

Some more perf optimizations. Components.stack is really slow compared to try { throw new Error() }. Added some custom front methods that save roundtrips to the server as well.
Attachment #8387958 - Attachment is obsolete: true
Attachment #8387958 - Flags: review?(rcampbell)
Attachment #8388122 - Flags: review?(rcampbell)
Comment on attachment 8388122 [details] [diff] [review]
v15 (last stable version)

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

tagging mgoodwin for secreview
Attachment #8388122 - Flags: review?(mgoodwin)
(Assignee)

Comment 44

5 years ago
List of caveats we found so far:
* this tool only works with requestAnimationFrame (bug 978948)
* canvas contexts inside iframes can't be inspected (bug 981748)
* animations with 300+ draw calls choke the entire browser while taking a snapshot (bug 981303)
Comment on attachment 8388122 [details] [diff] [review]
v15 (last stable version)

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

I'm almost completely happy with this; see my comment on inline scripts below.

::: browser/devtools/canvasdebugger/canvasdebugger.xul
@@ +23,5 @@
> +        <hbox id="snapshots-controls"
> +              class="devtools-toolbarbutton-group">
> +          <toolbarbutton id="record-snapshot"
> +                         class="devtools-toolbarbutton"
> +                         oncommand="SnapshotsListView._onRecordButtonClick()"

We're trying to phase out inline scripts to pave the way for CSP (see bug 960728). Could we use addEventListener from canvasdebugger.js instead (for this and the few other instances in canvasdebugger.xul)?

Note, due to bug 371900, if you may need to leave a minimal value (e.g. oncommand=";") for key handling to work properly. If this is the case, make this bug block bug 978115 so we can find it for cleanup once that issue is fixed.
Attachment #8388122 - Flags: review?(mgoodwin) → review-
(Assignee)

Comment 46

5 years ago
Created attachment 8390260 [details] [diff] [review]
v16

A considerable amount of work had to go into this for fixing bug 981303 (point 3 in comment 44). Since my laptop is kernel panicking lately, I'm safekeeping my progress here :)
Attachment #8388122 - Attachment is obsolete: true
Attachment #8388122 - Flags: review?(rcampbell)
(Assignee)

Comment 47

5 years ago
Created attachment 8391396 [details] [diff] [review]
v17

Progress! As of comment 46, changes have been made to the frontend to account for the (now) very different backend behavior and API.
(Assignee)

Updated

5 years ago
Attachment #8390260 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Duplicate of this bug: 981303
wow.

I'll try this out. Updates as warranted.
(Assignee)

Comment 50

5 years ago
Comment on attachment 8391396 [details] [diff] [review]
v17

This is unstable.
Attachment #8391396 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8388122 - Attachment description: v15 → v15 (last stable version)
Attachment #8388122 - Attachment is obsolete: false
(Assignee)

Comment 51

5 years ago
Created attachment 8392837 [details] [diff] [review]
v18 (stable optimized version)
Attachment #8388122 - Attachment is obsolete: true
(Assignee)

Comment 52

5 years ago
Comment on attachment 8392837 [details] [diff] [review]
v18 (stable optimized version)

Rob, please play with this a bit and let me know if everything works properly.
Attachment #8392837 - Flags: feedback?
hunk failed to apply in:

browser/themes/osx/devtools/layoutview.css.rej

 .theme-light .theme-body {
-  background-image: url(layout-background-grid.png), radial-gradient(circle at 50% 70%, hsl(210,53%,45%) 0%, hsl(210,54%,33%) 100%);
 }

intended?
Created attachment 8392985 [details]
missing timeline screens.png
I've applied the patch and played with the inspector a little bit. First of all, awesome work! The UI makes sense and is fast and easy to use. I like it a lot.

I have 2 questions which, I think, should be handled in follow-up bugs, but just to have your view on those, here they are:
- what about pages that have several canvas contexts? For now, I think it only works with the first one it finds. Is that right?
- could the current solution be easily extended to have a record start/stop button instead of the requestanimationframe snapshot?
(Assignee)

Comment 56

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #53)
> hunk failed to apply in:
> 
> browser/themes/osx/devtools/layoutview.css.rej
> 
>  .theme-light .theme-body {
> -  background-image: url(layout-background-grid.png), radial-gradient(circle
> at 50% 70%, hsl(210,53%,45%) 0%, hsl(210,54%,33%) 100%);
>  }
> 
> intended?

That is accidental. I'll remove it from the patch.
(Assignee)

Comment 57

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #54)
> Created attachment 8392985 [details]
> missing timeline screens.png

I can't reproduce this. Do you have any STR? I wanna fix this asap.
(Assignee)

Updated

5 years ago
Flags: needinfo?(rcampbell)
(Assignee)

Comment 58

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #55)
> I've applied the patch and played with the inspector a little bit. First of
> all, awesome work! The UI makes sense and is fast and easy to use. I like it
> a lot.
> 
> I have 2 questions which, I think, should be handled in follow-up bugs, but
> just to have your view on those, here they are:
> - what about pages that have several canvas contexts? For now, I think it
> only works with the first one it finds. Is that right?

Yeah, these are not (fully) supported yet. The HTMLCanvasElement prototype methods are instrumented, but recording an animation frame works only on once canvas element at a time. Definitely fixable in a followup, and we should get some nice UI for choosing which canvas to work on.

> - could the current solution be easily extended to have a record start/stop
> button instead of the requestanimationframe snapshot?

Sure. However, I'm a bit scared by allowing too much recording to happen, since this isn't easy on the memory or cpu.
(Assignee)

Updated

5 years ago
Blocks: 985109
(Assignee)

Updated

5 years ago
Blocks: 985111
(Assignee)

Comment 59

5 years ago
Created attachment 8393299 [details] [diff] [review]
v19 (rebased for fx-team tip)
Attachment #8392985 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8393299 - Attachment description: v 19 (rebased for fx-team tip) → v19 (rebased for fx-team tip)
(Assignee)

Comment 60

5 years ago
Discussed with Rob about this on IRC and concluded that it's probably related to his build configuration. The builds at [0] work properly.

[0] https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/vporof@mozilla.com-4827ea0e21dc/
Flags: needinfo?(rcampbell)
(Assignee)

Updated

5 years ago
Blocks: 985269
(Assignee)

Comment 61

5 years ago
Upon further investigation, I can reproduce comment 54. It seems that a platform change broke things, since everything was fine before the merge :(

Bisecting...
(Assignee)

Updated

5 years ago
Depends on: 982960
(Assignee)

Comment 62

5 years ago
It seems bug 982960 is the culprit for comment 54. Investigating.
Blocks: 985488
(Assignee)

Updated

5 years ago
Depends on: 985685
(Assignee)

Updated

5 years ago
Attachment #8392837 - Attachment is obsolete: true
Attachment #8392837 - Flags: feedback?
(Assignee)

Comment 63

5 years ago
Created attachment 8397085 [details] [diff] [review]
v20

CAN WE LAND THIS NOW?
Attachment #8393299 - Attachment is obsolete: true
Attachment #8397085 - Flags: review?(rcampbell)
(Assignee)

Comment 64

5 years ago
Created attachment 8397104 [details] [diff] [review]
v20

Removed accidental whitespace changes in firefox.js
Attachment #8397085 - Attachment is obsolete: true
Attachment #8397085 - Flags: review?(rcampbell)
Attachment #8397104 - Flags: review?(rcampbell)
Comment on attachment 8397104 [details] [diff] [review]
v20

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

nice use of Task.jsm in canvasdebugger.js. Impressive stuff. Couple of questions, but I can't fault any of this.

::: browser/devtools/canvasdebugger/canvasdebugger.js
@@ +74,5 @@
> +const SNAPSHOT_DATA_EXPORT_MAX_BLOCK = 1000; // ms
> +const SNAPSHOT_DATA_DISPLAY_DELAY = 10; // ms
> +const SCREENSHOT_DISPLAY_DELAY = 100; // ms
> +const STACK_FUNC_INDENTATION = 14; // px
> +const CALLS_LIST_SERIALIZER_IDENTIFIER = "Recorded Animation Frame Snapshot";

this is a surprisingly english string to be used for a "filetype". I'd have expected IMAGE/BMP or something.

@@ +313,5 @@
> +
> +    Task.spawn(function*() {
> +      // Wait for a few milliseconds between presenting the function calls,
> +      // screenshot and thumbnails, to allow each component being
> +      // sequentially drawn. This gives the illusion of snappiness.

I like that the snappiness is only an illusion.

::: toolkit/devtools/server/actors/call-watcher.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const {Cc, Ci, Cu, Cr} = require("chrome");

what is up with this addonism?

::: toolkit/devtools/server/actors/canvas.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const {Cc, Ci, Cu, Cr} = require("chrome");

we're using this instead of just pulling them off Components now? addon-sdkism.

No references to any of these. Are you just pulling this in to gain access to chrome? What's the dealio, yo?
Attachment #8397104 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 66

5 years ago
Created attachment 8397377 [details] [diff] [review]
v21

Addressed comments.
Attachment #8397377 - Flags: review+
(Assignee)

Comment 67

5 years ago
One last try run before landing.
(Assignee)

Comment 69

5 years ago
Created attachment 8397891 [details] [diff] [review]
lol.patch
Attachment #8397891 - Flags: review?(jryans)
Comment on attachment 8397891 [details] [diff] [review]
lol.patch

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

LGTM!  Best patch name! :D
Attachment #8397891 - Flags: review?(jryans) → review+
The original landing also caused browser-chrome failures (the suite takes an age to complete):
https://tbpl.mozilla.org/php/getParsedLog.php?id=36816997&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=36815572&tree=Fx-Team

Will these also be fixed by the followup?
(In reply to Ed Morley [:edmorley UTC+0] from comment #75)
> Will these also be fixed by the followup?

Seems they were not, still occurred after comment 74:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36821476&tree=Fx-Team

Backed out 955336deb0ff again, as well as the follow-up 9c456120ed57:
remote:   https://hg.mozilla.org/integration/fx-team/rev/7b9fab28c591
remote:   https://hg.mozilla.org/integration/fx-team/rev/88e9a2e07261
(In reply to Ed Morley [:edmorley UTC+0] from comment #76)
> Backed out 955336deb0ff again, as well as the follow-up 9c456120ed57:
> remote:   https://hg.mozilla.org/integration/fx-team/rev/7b9fab28c591
> remote:   https://hg.mozilla.org/integration/fx-team/rev/88e9a2e07261

qbackout generated an unhelpful commit message; fixed them in:
https://hg.mozilla.org/integration/fx-team/rev/5ab9f9afe189
https://hg.mozilla.org/integration/fx-team/rev/46a27f44a9e6
(Assignee)

Comment 78

5 years ago
As discussed on IRC with edmorley and ryanvm, the need for a backout is most likely an artifact of a different issue, not the tests in this patch having any particular problems themselves.

It seems the only thing we can do is just wait for bug 819963 to land.
Depends on: 819963
(Assignee)

Updated

5 years ago
Attachment #8397104 - Attachment is obsolete: true
(Assignee)

Comment 79

5 years ago
This failure [0] which happened immediately after the backout makes comment 78 definitive. Today wasn't a good day for science :)

As soon as devtools run in their own batch, I'm landing this again. This has already started on Cedar [1], and should come over to fx-team soon.

[0] https://tbpl.mozilla.org/php/getParsedLog.php?id=36831629&tree=Fx-Team
[1] https://tbpl-dev.allizom.org/?tree=Cedar
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d5882d4e8888
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31

Comment 82

5 years ago
How to test/use the canvas debugger?
Reloading the page and then clicking the record button starts the recording, but it never seems to stop?
(Assignee)

Comment 83

5 years ago
(In reply to Alfred Kayser from comment #82)
> How to test/use the canvas debugger?
> Reloading the page and then clicking the record button starts the recording,
> but it never seems to stop?

Good to know: bug 978948 and bug 985109 are not fixed yet. So your test page needs to use requestAnimationFrame and have a single canvas.
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
Sorry for the lateness but does this need testing before we release Firefox 31?
Flags: needinfo?(vporof)
(Assignee)

Comment 85

5 years ago
There has been enough testing done on Nightly and Aurora.
Flags: needinfo?(vporof)
QA Whiteboard: [qa-]

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.