Closed Bug 887605 Opened 7 years ago Closed 6 years ago

Add "Copy URL" context menu item

Categories

(DevTools :: Netmonitor, defect, P2)

defect

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)

We should add a Copy URL (or "Copy Location"?) menu item to the context menu of the requests list.
Whiteboard: [good-first-bug]
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.
See Also: → 861686
Priority: -- → P2
Duplicate of this bug: 888586
Whiteboard: [good-first-bug] → [good first bug][mentor=fayearthur@gmail.com][lang=js]
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.
File a new one, another good first bug!
(In reply to Heather Arthur [:harth] from comment #4)
> File a new one, another good first bug!

Bug 889944
Duplicate of this bug: 890285
May i take this bug?
Attachment #772006 - Flags: review?(fayearthur)
(In reply to lie.r.min.g from comment #7)
> May i take this bug?

Yes, thanks! Will review your patch soon.
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 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 (=
Attachment #772006 - Attachment is obsolete: true
Attachment #772006 - Flags: review?(fayearthur)
Attachment #773197 - Flags: review?(fayearthur)
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+
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]
https://hg.mozilla.org/mozilla-central/rev/9f58c0b28e37
Assignee: nobody → lie.r.min.g
Status: NEW → RESOLVED
Closed: 6 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 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.
Duplicate of this bug: 861686
Follow up is: bug 892413
(In reply to Victor Porof [:vp] from comment #17)
> Filed bug 887605.

D'oh, what harth said :)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.