887605 - Add "Copy URL" context menu item

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Heather Arthur [:harth]]

We should add a Copy URL (or "Copy Location"?) menu item to the context menu of the requests list.

Comment 1

[Victor Porof [:vporof][:vp]]

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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 4

[Heather Arthur [:harth]]

File a new one, another good first bug!

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Heather Arthur [:harth] from comment #4)
> File a new one, another good first bug!

Bug 889944

Comment 7

[Minarto Margoliono]

May i take this bug?

Comment 8

[Minarto Margoliono]

Attachment: Add copy url context menu in the network panel

Comment 9

[Heather Arthur [:harth]]

(In reply to lie.r.min.g from comment #7)
> May i take this bug?

Yes, thanks! Will review your patch soon.

Comment 10

[Heather Arthur [:harth]]

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"

Comment 11

[Heather Arthur [:harth]]

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

Comment 12

[Minarto Margoliono]

Attachment: Add copy url context menu in the network panel

Comment 13

[Heather Arthur [:harth]]

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.

Comment 14

[Heather Arthur [:harth]]

http://hg.mozilla.org/integration/fx-team/rev/9f58c0b28e37

Thanks again for this crucial addition.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9f58c0b28e37

Comment 16

[Victor Porof [:vporof][:vp]]

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

[Victor Porof [:vporof][:vp]]

Filed bug 887605.

Comment 19

[Heather Arthur [:harth]]

Follow up is: bug 892413

Comment 20

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #17)
> Filed bug 887605.

D'oh, what harth said :)