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/

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

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

(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, ...

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

Crude WIP.

This actually does something useful.

Getting there...

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.

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 ?

* "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.

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.

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23) Thank you :)

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.

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.

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.

(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!

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!

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.

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.

(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.

Addressed comments, finished the import/export mechanism, etc. Just a few more small frontend tests, and this should be ready to land.

Finishing touches.

Searchbox now works properly.

Preliminary try run looks green: https://bugzilla.mozilla.org/show_bug.cgi?id=917226

THIRTEEN YES.

Fixed a few papercuts after a testing session today with Kannan.

Green: https://tbpl.mozilla.org/?tree=Try&rev=57e3c5f4f7cd

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.

Comment on attachment 8388122 [details] [diff] [review] v15 (last stable version) Review of attachment 8388122 [details] [diff] [review]: ----------------------------------------------------------------- tagging mgoodwin for secreview

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.

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 :)

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

wow. I'll try this out. Updates as warranted.

Comment on attachment 8391396 [details] [diff] [review] v17 This is unstable.

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.

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?

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?

(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.

(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.

(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.

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/

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

It seems bug 982960 is the culprit for comment 54. Investigating.

CAN WE LAND THIS NOW?

Removed accidental whitespace changes in firefox.js

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?

Addressed comments.

One last try run before landing.

Comment on attachment 8397891 [details] [diff] [review] lol.patch Review of attachment 8397891 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! Best patch name! :D

Backed out for test failures: https://tbpl.mozilla.org/?tree=Fx-Team&rev=894e40121370&jobname=-other eg https://tbpl.mozilla.org/php/getParsedLog.php?id=36809053&tree=Fx-Team https://hg.mozilla.org/integration/fx-team/rev/882b91ce5a9f

(In reply to Ed Morley [:edmorley UTC+0] from comment #71) > Backed out for test failures: > https://tbpl.mozilla.org/?tree=Fx-Team&rev=894e40121370&jobname=-other > eg https://tbpl.mozilla.org/php/getParsedLog.php?id=36809053&tree=Fx-Team > > https://hg.mozilla.org/integration/fx-team/rev/882b91ce5a9f But I just fixed that!

Test fix: https://hg.mozilla.org/integration/fx-team/rev/9c456120ed57

Relanding: https://hg.mozilla.org/integration/fx-team/rev/955336deb0ff

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

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.

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

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?

(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.

Sorry for the lateness but does this need testing before we release Firefox 31?

There has been enough testing done on Nightly and Aurora.