Build a canvas inspection tool

RESOLVED FIXED in Firefox 31

Status

defect
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

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

6 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

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

Updated

6 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

6 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

6 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
Posted patch v1 wip (obsolete) — Splinter Review
Crude WIP.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Assignee

Comment 10

5 years ago
Posted patch v2 wip (obsolete) — Splinter Review
This actually does something useful.
Attachment #8377562 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
Posted patch v3 wip (obsolete) — Splinter Review
Getting there...
Attachment #8377912 - Attachment is obsolete: true
Assignee

Comment 12

5 years ago
Posted patch v4 wip (obsolete) — Splinter Review
Attachment #8378625 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
Posted patch v5 demo (obsolete) — Splinter Review
Attachment #8379418 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
Posted patch v6 (still wip) (obsolete) — Splinter Review
Attachment #8379916 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
Posted patch v7 wip (obsolete) — Splinter Review
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
Posted patch v8 wip (obsolete) — Splinter Review
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
Posted image 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
Posted patch v9 (obsolete) — Splinter Review
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
Posted patch v10 (obsolete) — Splinter Review
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
Posted patch v11 (obsolete) — Splinter Review
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
Posted patch v12 (obsolete) — Splinter Review
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
Posted patch v13 (obsolete) — Splinter Review
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
Posted patch v14 (obsolete) — Splinter Review
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
Posted patch v15 (last stable version) (obsolete) — Splinter Review
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
Posted patch v16 (obsolete) — Splinter Review
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
Posted patch v17 (obsolete) — Splinter Review
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
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?
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
Posted patch v19 (rebased for fx-team tip) (obsolete) — Splinter Review
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
Posted patch v20 (obsolete) — Splinter Review
CAN WE LAND THIS NOW?
Attachment #8393299 - Attachment is obsolete: true
Attachment #8397085 - Flags: review?(rcampbell)
Assignee

Comment 64

5 years ago
Posted patch v20 (obsolete) — Splinter Review
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
Posted patch v21Splinter Review
Addressed comments.
Attachment #8397377 - Flags: review+
Assignee

Comment 67

5 years ago
One last try run before landing.
Assignee

Comment 69

5 years ago
Posted patch lol.patchSplinter Review
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
Closed: 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

Last year
Product: Firefox → DevTools

Updated

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