Closed
Bug 827140
Opened 12 years ago
Closed 9 years ago
Copying stack traces in the UI to the clipboard
Categories
(DevTools :: Debugger, defect, P3)
DevTools
Debugger
Tracking
(firefox46 fixed)
VERIFIED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: tetsuharu, Assigned: b4bomsy, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=js][lang=xul])
Attachments
(2 files, 4 obsolete files)
2.84 KB,
patch
|
vporof
:
feedback+
|
Details | Diff | Splinter Review |
10.28 KB,
patch
|
b4bomsy
:
review+
|
Details | Diff | Splinter Review |
If we implement the feature of copying stack trace, the bug report will be easy. And useful.
Updated•12 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Priority: -- → P3
Comment 1•12 years ago
|
||
I won't be able to look at this very soon.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Comment 2•12 years ago
|
||
I'm expecting this patch will be rejected, because I'm extracting the stack trace from the menu items. There has to be a cleaner way to get the full stack.
Assignee: nobody → ajvincent
Attachment #763177 -
Flags: review?(vporof)
Comment 3•12 years ago
|
||
Comment on attachment 763177 [details] [diff] [review]
Copy stack to clipboard from context menu (first pass)
Review of attachment 763177 [details] [diff] [review]:
-----------------------------------------------------------------
Hey! Thanks for the patch!
You can use the frameCache in StackFrames from DebuggerController, if you really want to, but then you'd have to go parsing urls and converting them to unicode all over again. So I advise against that. However, as you might have guessed, using the menu items is not that nice. See below.
Just need to add a test and address some minor issues, other than that, f+.
::: browser/devtools/debugger/debugger-controller.js
@@ +590,5 @@
> for (let frame of this.activeThread.cachedFrames) {
> this._addFrame(frame);
> }
> +
> + DebuggerView.StackFrames.addCopyCommand();
Could you make this operation the last one please?
::: browser/devtools/debugger/debugger-toolbar.js
@@ +443,5 @@
>
> this._framesCache.set(aDepth, stackframeItem);
> },
>
> + getStack: function() {
Might want to rename this to "getStackAsString". Please document this method as well.
@@ +444,5 @@
> this._framesCache.set(aDepth, stackframeItem);
> },
>
> + getStack: function() {
> + // XXX ajvincent There has to be a better way to do this than iterating over menuitem elements...
Yup.
in _addFrame, when you append a stack frame item to this container, add the following information:
this.push(...
attachment: {
sourceLocation: aSourceLocation,
lineNumber: aLineNumber,
...
},
...
});
And here:
var report = "";
for (let frameItem in this) {
let { depth, sourceLocation, lineNumber }
report += depth + ". " + sourceLocation + ": " + lineNumber + "\n";
}
@@ +454,5 @@
> + report += msg + "\n";
> + }
> + return report;
> + },
> +
Nit: trailing whitespace.
@@ +455,5 @@
> + }
> + return report;
> + },
> +
> + addCopyCommand: function() {
Please document this function.
@@ +459,5 @@
> + addCopyCommand: function() {
> + this._menupopup.appendChild(document.createElement("menuseparator"));
> + let copyItem = document.createElement("menuitem");
> + copyItem.setAttribute("label", "Copy");
> + let _this = this;
I haven't seen this _this for a while! There's a better way!
copyItem.addEventListener("command", () => {
let s = this.getStack();
...
});
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions#Lexical_this
@@ +462,5 @@
> + copyItem.setAttribute("label", "Copy");
> + let _this = this;
> + copyItem.addEventListener("command", function() {
> + let s = _this.getStack();
> + const gClipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
Why const? Why gClipboardHelper, since it's not a global? This would be better:
let clipboardHelper = Cc["@mozilla.org/widget/clipboardhelper;1"]
.getService(Ci.nsIClipboardHelper)
@@ +466,5 @@
> + const gClipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"]
> + .getService(Components.interfaces.nsIClipboardHelper);
> + gClipboardHelper.copyString(s);
> + }, false);
> + this._menupopup.appendChild(copyItem);
Is this menuitem ever removed? Does it appear twice when you get a fresh stack?
Attachment #763177 -
Flags: review?(vporof) → feedback+
Comment 4•12 years ago
|
||
[10:42] victorporof WeirdAl: see browser_dbg_aaa_run_first_leaktest.js
[10:42] victorporof it's a barebones test that does nothing
[10:42] victorporof but it gets you a stacktrace!
[10:42] victorporof clone it and add your stuff in the performTest callback
Comment 5•11 years ago
|
||
no activity for > 6mos. Alex, reassign yourself if you see yourself working on this again soon. Otherwise, someone else should finish this.
Assignee: ajvincent → nobody
Whiteboard: [mentor=vporof][lang=js][lang=xul]
Updated•11 years ago
|
Mentor: vporof
Whiteboard: [mentor=vporof][lang=js][lang=xul] → [lang=js][lang=xul]
Summary: Enable to copying stack trace → Enable to copying stack trace / call stack
Summary: Enable to copying stack trace / call stack → Enable copying stack trace / call stack
Updated•10 years ago
|
Summary: Enable copying stack trace / call stack → Copying stack traces to the clipboard
Updated•10 years ago
|
Summary: Copying stack traces to the clipboard → Copying stack traces in the UI to the clipboard
Updated•10 years ago
|
Blocks: dbg-frontend
Assignee | ||
Comment 8•10 years ago
|
||
I'm working on this. A quick question.
What context menu would we ideally want to put the copy command unto?
ATM i'm creating a context menu for the Stack frames view which would have the copy command, is that the best place for it?
Assignee | ||
Comment 9•10 years ago
|
||
What context menu would we ideally want to put the copy command unto?
ATM i've created a context menu for the Stack frames view which would have the copy command, is that the best place for it?
Attachment #8629932 -
Flags: feedback?(vporof)
Comment 10•10 years ago
|
||
Comment on attachment 8629932 [details] [diff] [review]
827140wip.patch
Review of attachment 8629932 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. But it's gonna need a test. Let me know if you need any help with that.
This place is good for the context menu. It doesn't prevent us from adding it to different places too in the future.
Attachment #8629932 -
Flags: feedback?(vporof) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
I took a stab at the tests.
i'm having an issue with the second test (browser/devtools/debugger/test/browser_dbg_stack-contextmenu-02.js), i seem to have hit a block.
i get this
21 INFO TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_stack-contextmenu-02.js | Timed out while polling clipboard for pasted data -
Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:wait:947
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForClipboard/wait/<:964
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:EventLoop.prototype.enter:339
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype._pushThreadPause:534
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype._pauseAndRespond:735
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/script.js:ThreadActor.prototype.onDebuggerStatement:1795
http://example.com/browser/browser/devtools/debugger/test/doc_recursion-stack.html:simpleCall:14
chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:this.call:18
chrome://mochitests/content/browser/browser/devtools/debugger/test/code_frame-script.js:null:71
null:null:0
i'm assuming it can't find the string copied to the clipboard...but not sure.
Would appreciate help here. Thanks
Attachment #8629932 -
Attachment is obsolete: true
Attachment #8636715 -
Flags: feedback?(vporof)
Updated•10 years ago
|
Assignee: nobody → b4bomsy
Comment 12•10 years ago
|
||
Comment on attachment 8636715 [details] [diff] [review]
bug827140.patch
Please ask :jlongster for feedback/review since he's now the debugger maintainer.
Attachment #8636715 -
Flags: feedback?(vporof)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jlong)
Comment 13•10 years ago
|
||
Comment on attachment 8636715 [details] [diff] [review]
bug827140.patch
Review of attachment 8636715 [details] [diff] [review]:
-----------------------------------------------------------------
I'm assuming you are having problems because your `STACK_STRING` did not have a newline at the end, and the code which gets the stack as a string was always appending a newline to each line, even the last one.
::: browser/devtools/debugger/views/stack-frames-view.js
@@ +116,5 @@
> + var report = "";
> + for (let frameItem of this) {
> + let { attachment: { depth, url, line }} = frameItem;
> + report += depth + ". " + url + ": " + line + "\n";
> + }
You are always adding a newline add the end. Do this instead:
return [...this].map(frameItem => {
const { attachment: { depth, url, line }} = frameItem;
return depth + ". " + url + ": " + line;
}).join("\n");
I might have an error in there, but you get the idea.
Updated•10 years ago
|
Flags: needinfo?(jlong)
Assignee | ||
Comment 14•9 years ago
|
||
:jlongster thanks.
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8636715 -
Attachment is obsolete: true
Attachment #8657243 -
Flags: review?(jlong)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8657243 -
Attachment is obsolete: true
Attachment #8657243 -
Flags: review?(jlong)
Attachment #8657259 -
Flags: review?(jlong)
Comment 17•9 years ago
|
||
+1
When paused in the debugger, when viewing the call stack, I'd like to be able to somehow copy the call stack into the copy buffer at any given step/break point.
For example, I'm debugging my code, hit a break point, step around, view the stack trace tab, right click + copy call stack, paste the call stack into an email to a fellow developer.
The contents of the call stack might be equivalent to `(new Error).stack`, from where the debugger is currently paused (file, line, col, fn name).
Comment 18•9 years ago
|
||
Comment on attachment 8657259 [details] [diff] [review]
bug827140.patch
Review of attachment 8657259 [details] [diff] [review]:
-----------------------------------------------------------------
Looks great! A few nits about the format: is it really useful to include the depth information here? I think most people would be surprised by that. Nick what do you think? Right now the stack on Error objects is a little different than the stack we have in the debugger, and I don't think we can get something like "function name" right now. For now, at least having "url:line:column" is a good start.
I think we should follow the Error stack format: remove the space between url and line, and make sure to add column in there. It should just be another property on the frame object. r+ if you do this, or convince me differently :)
Attachment #8657259 -
Flags: review?(jlong) → review+
Comment 19•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #18)
> Nick
> what do you think? Right now the stack on Error objects is a little
> different than the stack we have in the debugger, and I don't think we can
> get something like "function name" right now.
I mean, the function names are listed in the UI/DOM elements created within the debugger under the stack trace tab. Couldn't their contents just be iterated and copied into the copy buffer? I could file a separate bug as a follow up?
Comment 20•9 years ago
|
||
(In reply to Nick Desaulniers [:\n] from comment #19)
> (In reply to James Long (:jlongster) from comment #18)
> > Nick
> > what do you think? Right now the stack on Error objects is a little
> > different than the stack we have in the debugger, and I don't think we can
> > get something like "function name" right now.
>
> I mean, the function names are listed in the UI/DOM elements created within
> the debugger under the stack trace tab. Couldn't their contents just be
> iterated and copied into the copy buffer? I could file a separate bug as a
> follow up?
Oh, you're totally right.
Hubert, can you change the format to the following?
const { attachment: { title, url, line, column }} = frameItem;
return title + '@' + url + ":" + line + ':' + column;
That `title` is what the view made for the frame, which is just whatever "display name" the Debugger.Frame object has, which is likely just the function name.
The `column` is not actually saved in the `frameItem` right now, so you'll also need to add it here: https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/views/stack-frames-view.js#L100. Just add a `column: aFrame.column` property.
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8657259 -
Attachment is obsolete: true
Attachment #8672768 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #20)
> (In reply to Nick Desaulniers [:\n] from comment #19)
> > (In reply to James Long (:jlongster) from comment #18)
> > > Nick
> > > what do you think? Right now the stack on Error objects is a little
> > > different than the stack we have in the debugger, and I don't think we can
> > > get something like "function name" right now.
> >
> > I mean, the function names are listed in the UI/DOM elements created within
> > the debugger under the stack trace tab. Couldn't their contents just be
> > iterated and copied into the copy buffer? I could file a separate bug as a
> > follow up?
>
> Oh, you're totally right.
>
> Hubert, can you change the format to the following?
>
> const { attachment: { title, url, line, column }} = frameItem;
> return title + '@' + url + ":" + line + ':' + column;
>
> That `title` is what the view made for the frame, which is just whatever
> "display name" the Debugger.Frame object has, which is likely just the
> function name.
>
> The `column` is not actually saved in the `frameItem` right now, so you'll
> also need to add it here:
> https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/
> views/stack-frames-view.js#L100. Just add a `column: aFrame.column` property.
i've made the neccessary changes
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(jlong)
Comment 23•9 years ago
|
||
Looks good, sorry it took so long to get around to this again. I just applied it locally and everything works. Let's land this.
Flags: needinfo?(jlong)
Keywords: checkin-needed
Comment 24•9 years ago
|
||
Keywords: checkin-needed
Comment 25•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Assignee | ||
Comment 26•9 years ago
|
||
Nice! Thanks :jlongster for your help.
Comment 27•9 years ago
|
||
[bugday-20160323]
STR:
1. Tools -> Options -> Web Developer -> Debugger
2. Debugger will open. It is having two sub tabs: Sources and Call stack.
3. Now pause debugger.
4. Go to js window and js code.
5. Paste it into Email for more debugging.
Status: RESOLVED,FIXED --> VERIFIED
Component:
Name Firefox
Version 46.0b2
Build ID 20160314144540
Update Channel beta
User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS Windows 7 SP1 x86_64
Actual Results:
As expected
Expected Results:
Able to copy stack trace into copy buffer and able send it to developer.
Flags: needinfo?(saneyuki.s.snyk)
Reporter | ||
Comment 28•9 years ago
|
||
I verified this bug has been fixed. Thank you!
Status: RESOLVED → VERIFIED
Flags: needinfo?(saneyuki.s.snyk)
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•