Closed
Bug 887605
Opened 11 years ago
Closed 11 years ago
Add "Copy URL" context menu item
Categories
(DevTools :: Netmonitor, defect, P2)
DevTools
Netmonitor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: harth, Assigned: lie.r.min.g)
References
Details
(Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
harth
:
review+
|
Details | Diff | Splinter Review |
We should add a Copy URL (or "Copy Location"?) menu item to the context menu of the requests list.
Reporter | ||
Updated•11 years ago
|
Whiteboard: [good-first-bug]
Comment 1•11 years ago
|
||
While we're at it, I think the context menu should also be available on the URL displayed in the Headers section, as described in bug 861686.
Updated•11 years ago
|
Priority: -- → P2
Updated•11 years ago
|
Whiteboard: [good-first-bug] → [good first bug][mentor=fayearthur@gmail.com][lang=js]
Comment 3•11 years ago
|
||
I've also received requests to to have an open resource in new tab context menu item, which might be able to tag along on this bug. If not, I'll file a new bug.
Reporter | ||
Comment 4•11 years ago
|
||
File a new one, another good first bug!
Comment 5•11 years ago
|
||
(In reply to Heather Arthur [:harth] from comment #4)
> File a new one, another good first bug!
Bug 889944
Assignee | ||
Comment 7•11 years ago
|
||
May i take this bug?
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #772006 -
Flags: review?(fayearthur)
Reporter | ||
Comment 9•11 years ago
|
||
(In reply to lie.r.min.g from comment #7)
> May i take this bug?
Yes, thanks! Will review your patch soon.
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 772006 [details] [diff] [review]
Add copy url context menu in the network panel
Review of attachment 772006 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch, this looks great and works well. Two things:
1) Make sure to hide the copy menu item if there is no item selected, see the _onContextShowing() function in netmonitor-view.js.
2) This'll need a basic test. See the tests in browser/devtools/netmonitor/test. I would test selecting the first request, calling copyUrl(), and seeing if the clipboard contains the url you want.
::: browser/devtools/netmonitor/netmonitor-view.js
@@ +383,5 @@
> + */
> + copyUrl: function() {
> + let selected = this.selectedItem.attachment;
> +
> + clipboardHelper.copyString(selected.url, this.document);
Looks like you added some trailing whitespace here.
::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd
@@ +179,5 @@
> <!ENTITY netmonitorUI.summary.resend.accesskey "R">
>
> +<!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the label displayed
> + - on the context menu that copy selected request's url in new tab -->
> +<!ENTITY netmonitorUI.summary.copyUrl "Copy url">
Let's use "Copy URL" (all caps), to stay consistent with the rest of the UI.
I totally understand why you used the label "netmonitorUI.summary.copyUrl" similar to the Resend item, but that's because the Resend item is on a button in the summary panel. The copy url thing isn't showing there, so we should name it something like "netmonitorUI.context.copyUrl"
Reporter | ||
Comment 11•11 years ago
|
||
Comment on attachment 772006 [details] [diff] [review]
Add copy url context menu in the network panel
Review of attachment 772006 [details] [diff] [review]:
-----------------------------------------------------------------
Oh, one more thing.
Thanks!
::: browser/devtools/netmonitor/netmonitor.xul
@@ +23,5 @@
> <menuitem id="request-menu-context-resend"
> label="&netmonitorUI.summary.resend;"
> accesskey="&netmonitorUI.summary.resend.accesskey;"
> oncommand="NetMonitorView.RequestsMenu.cloneSelectedRequest();"/>
> + <menuitem id="request-menu-context-copy-url"
Put the "Copy URL" item above the "Resend" item, it's more common (=
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #772006 -
Attachment is obsolete: true
Attachment #772006 -
Flags: review?(fayearthur)
Attachment #773197 -
Flags: review?(fayearthur)
Reporter | ||
Comment 13•11 years ago
|
||
Comment on attachment 773197 [details] [diff] [review]
Add copy url context menu in the network panel
Review of attachment 773197 [details] [diff] [review]:
-----------------------------------------------------------------
This is great, thanks Minarto. Two minor issues, that I will fix myself before I check it in in a few minutes:
::: browser/devtools/netmonitor/netmonitor-view.js
@@ +1188,5 @@
> */
> _onContextShowing: function() {
> + let resendElement = $("#request-menu-context-resend");
> + resendElement.hidden = !this.selectedItem || this.selectedItem.attachment.isCustom;
> +
Trailing whitespace here.
@@ +1190,5 @@
> + let resendElement = $("#request-menu-context-resend");
> + resendElement.hidden = !this.selectedItem || this.selectedItem.attachment.isCustom;
> +
> + let copyUrlElement = $("#request-menu-context-copy-url");
> + copyUrlElement.hidden = !this.selectedItem || this.selectedItem.attachment.isCustom;
Should put `!this.selectedItem || this.selectedItem.attachment.isCustom` in a variable to avoid repeating that code.
Attachment #773197 -
Flags: review?(fayearthur) → review+
Reporter | ||
Comment 14•11 years ago
|
||
http://hg.mozilla.org/integration/fx-team/rev/9f58c0b28e37
Thanks again for this crucial addition.
Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js] → [good first bug][mentor=fayearthur@gmail.com][lang=js][fixed-in-fx-team]
Comment 15•11 years ago
|
||
Assignee: nobody → lie.r.min.g
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=fayearthur@gmail.com][lang=js][fixed-in-fx-team] → [good first bug][mentor=fayearthur@gmail.com][lang=js]
Target Milestone: --- → Firefox 25
Comment 16•11 years ago
|
||
Comment on attachment 773197 [details] [diff] [review]
Add copy url context menu in the network panel
Review of attachment 773197 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for this path. I would like a followup for fixing the things I mention below.
::: browser/devtools/netmonitor/netmonitor-view.js
@@ +383,5 @@
> + */
> + copyUrl: function() {
> + let selected = this.selectedItem.attachment;
> +
> + clipboardHelper.copyString(selected.url, this.document);
Nit: why "this.document"? Isn't it more obvious to just use "document"? This is not a jsm.
::: browser/devtools/netmonitor/test/browser_net_copy_url.js
@@ +11,5 @@
> +
> + let { NetMonitorView } = aMonitor.panelWin;
> + let { RequestsMenu } = NetMonitorView;
> +
> + RequestsMenu.lazyUpdate = false;
This is really not necessary.
@@ +15,5 @@
> + RequestsMenu.lazyUpdate = false;
> +
> + waitForNetworkEvents(aMonitor, 1).then(() => {
> + let imageRequest = RequestsMenu.getItemAtIndex(0);
> + RequestsMenu.selectedItem = imageRequest;
Why is this named imageRequest? It is not an image :)
@@ +17,5 @@
> + waitForNetworkEvents(aMonitor, 1).then(() => {
> + let imageRequest = RequestsMenu.getItemAtIndex(0);
> + RequestsMenu.selectedItem = imageRequest;
> +
> + waitForClipboard(RequestsMenu.selectedItem.attachment.url, function(){ RequestsMenu.copyUrl() } , cleanUp, cleanUp);
This is a very awkwardly formatted and very long line. While the 80 chars wrapping rules are not a restriction, it's generally nice to avoid things getting to 123 chars, especially in these trivial cases.
Why aren't you reusing the previous variable instead of rewriting RequestsMenu.etc.etc.? Also, shouldn't we do something different like ok(false, "whatever") on the error callback?
@@ +24,5 @@
> + aDebuggee.performRequests(1);
> +
> + function cleanUp(){
> + teardown(aMonitor);
> + finish();
This needs to be teardown(aMonitor).then(finish) otherwise you might end up with leaks or oranges.
Comment 17•11 years ago
|
||
Filed bug 887605.
Reporter | ||
Comment 19•11 years ago
|
||
Follow up is: bug 892413
Comment 20•11 years ago
|
||
(In reply to Victor Porof [:vp] from comment #17)
> Filed bug 887605.
D'oh, what harth said :)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•