For each resource in the list of requests, there should be a contgext menu item to open that resource in a new tab.

any starter points for this bug?

Hi Avinash, you're going to want to add a new <menuitem> to this menupopup here: https://hg.mozilla.org/integration/fx-team/file/17fe59f6c54a/browser/devtools/netmonitor/netmonitor.xul#l21 See the "resend" menuitem that already exists there, that'll be similar to what you have to do.

Avinash, I missed you on irc earlier, wanted to add that here's a code snippet for opening a new tab in the browser: https://gist.github.com/harthur/7c96d792f8678a372f94.

This is my first mozilla patch, please let me know if I'm missing anything. Thanks!

Just a quick drive by note before Heather gets here: please use 2 spaces (not tabs) like the rest of the code in this file does.

Comment on attachment 773482 [details] [diff] [review] Added Open in New Tab menu item. # HG changeset patch # User Ryan G. <digitalsmoke81@gmail.com> # Date 1373483409 25200 # Wed Jul 10 12:10:09 2013 -0700 # Node ID f2e72db3cc226eaa559654d6d325d8cacd5d5b22 # Parent dde4dcd6fa4678032cad2b65ae364f4f56d2b9e5 Bug 889944 - add "open in new tab" context menu item on resources diff --git a/browser/devtools/netmonitor/netmonitor-view.js b/browser/devtools/netmonitor/netmonitor-view.js --- a/browser/devtools/netmonitor/netmonitor-view.js +++ b/browser/devtools/netmonitor/netmonitor-view.js @@ -358,16 +358,28 @@ RequestsMenuView.prototype = Heritage.ex }) }); // Immediately switch to new request pane. this.selectedItem = newItem; }, /** + * Opens selected item in a new tab. + */ + newTabRequest: function() { + let win = Services.wm.getMostRecentWindow("navigator:browser") + let browser = win.getBrowser(); + + let selected = this.selectedItem.attachment; + + browser.selectedTab = browser.addTab(selected.url); + }, + + /** * Send a new HTTP request using the data in the custom request form. */ sendCustomRequest: function() { let selected = this.selectedItem.attachment; let data = Object.create(selected, { headers: { value: selected.requestHeaders.headers } }); diff --git a/browser/devtools/netmonitor/netmonitor.xul b/browser/devtools/netmonitor/netmonitor.xul --- a/browser/devtools/netmonitor/netmonitor.xul +++ b/browser/devtools/netmonitor/netmonitor.xul @@ -19,16 +19,20 @@ <popupset id="networkPopupSet"> <menupopup id="network-request-popup" onpopupshowing="NetMonitorView.RequestsMenu._onContextShowing(event);"> <menuitem id="request-menu-context-resend" label="&netmonitorUI.summary.resend;" accesskey="&netmonitorUI.summary.resend.accesskey;" oncommand="NetMonitorView.RequestsMenu.cloneSelectedRequest();"/> + <menuitem id="request-menu-context-newtab" + label="&netmonitorUI.summary.newtab;" + accesskey="&netmonitorUI.summary.newtab.accesskey;" + oncommand="NetMonitorView.RequestsMenu.newTabRequest();"/> </menupopup> </popupset> <box id="body" class="devtools-responsive-container" flex="1"> <vbox id="network-table" flex="1"> <toolbar id="requests-menu-toolbar" diff --git a/browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd b/browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd --- a/browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd +++ b/browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ -173,16 +173,26 @@ - on the button in the headers tab that opens a form to resend the currently displayed request --> <!ENTITY netmonitorUI.summary.resend "Resend"> <!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the access key - for the Resend menu item displayed in the context menu for a request --> <!ENTITY netmonitorUI.summary.resend.accesskey "R"> +<!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the label + - for the Open in New Tab menu item displayed in the context menu of the + - network container --> +<!ENTITY netmonitorUI.summary.newtab "Open in New Tab"> + +<!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the access key + - for the Open in New Tab menu item displayed in the context menu of the + - network container --> +<!ENTITY netmonitorUI.summary.newtab.accesskey "N"> + <!-- LOCALIZATION NOTE (debuggerUI.custom.newRequest): This is the label displayed - as the title of the new custom request form --> <!ENTITY netmonitorUI.custom.newRequest "New Request"> <!-- LOCALIZATION NOTE (debuggerUI.custom.query): This is the label displayed - above the query string entry in the custom request form --> <!ENTITY netmonitorUI.custom.query "Query String:">

ouch! i was already working on this, been travelling for last 2 days and there's already a patch.. awesome!

:) Just realized I need to write a test for this. Any tips?

I'm going to review the patch since it's here now. Ryan, would've been nice to ask Avinash if they were working on this. Avinash, Sorry )=. If you use cURL, you could check out fixing bug 859059, or another one of the good first bugs, and assign yourself.

Sorry guys! Guess I got too excited about finding a bug I could actually work on. I'll be more careful about asking next time.

Comment on attachment 773567 [details] [diff] [review] Added Open in New Tab menu item. Review of attachment 773567 [details] [diff] [review]: ----------------------------------------------------------------- A couple things: 1) Only show the item if a request is selected, see _onContextShowing() 2) Put this menuitem at the top of the context menu. 3) You're right, you'll need a test. Check out the other tests in browser/devtools/netmonitor/test. You could select a request, call your newTab() function and check if a new tab was opened with the correct url. I think you can do that with: gBrowser.tabContainer.addEventListener("TabOpen",...) ::: browser/devtools/netmonitor/netmonitor-view.js @@ +362,5 @@ > this.selectedItem = newItem; > }, > > /** > + * Opens selected item in a new tab. Nit: looks like there's some trailing whitespace here and other places, gotta get rid of those. ::: browser/devtools/netmonitor/netmonitor.xul @@ +26,5 @@ > oncommand="NetMonitorView.RequestsMenu.cloneSelectedRequest();"/> > + <menuitem id="request-menu-context-newtab" > + label="&netmonitorUI.context.newtab;" > + accesskey="&netmonitorUI.context.newtab.accesskey;" > + oncommand="NetMonitorView.RequestsMenu.newTabRequest();"/> Your going to have rebase this patch I imagine, a new menu item was just added in bug 887605, sorry! ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ +177,5 @@ > <!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the access key > - for the Resend menu item displayed in the context menu for a request --> > <!ENTITY netmonitorUI.summary.resend.accesskey "R"> > > +<!-- LOCALIZATION NOTE (debuggerUI.custom.headers): This is the label copy paste? match this up with the entity name

Comment on attachment 773567 [details] [diff] [review] Added Open in New Tab menu item. Review of attachment 773567 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/netmonitor/netmonitor-view.js @@ +364,5 @@ > > /** > + * Opens selected item in a new tab. > + */ > + newTabRequest: function() { openRequestInTab()? more action-y to go along with other functions in this file.

Ryan, just checking in, you have time to finish this up?

Updated the patch. As far as the test goes, will need more direction or can hand it off to someone else. Haven't had time to really figure out how to write a successful test yet.

Thanks for the update. As for the test, the netmonitor tests are Mochitest Browser tests, so you can run them with the command `./mach mochitest-browser browser/devtools/netmonitor` from the top-level. Take a look at the patch in bug 887605, it adds a test. You could follow it pretty closely, but instead of copying to clipboard and waiting for the clipboard, you would call your new method, and wait for a new tab to open with "gBrowser.tabContainer.addEventListener("TabOpen",...)". Let me know if you run into anything, here or on irc (#devtools channel, my nick is harth).

Just a question, now that we have bug 882538 fixed, maybe it makes sense to use these methods to open the tab remotely? On the other hand, I can see it might make sense to open the request in the local browser instead.

(In reply to Philipp Kewisch [:Fallen] from comment #18) > Just a question, now that we have bug 882538 fixed, maybe it makes sense to > use these methods to open the tab remotely? On the other hand, I can see it > might make sense to open the request in the local browser instead. That's a good question. I think we'd need bug 891003 for that. But, I would personally rather it open locally.

Avinash, looks like Ryan doesn't have time. You could pick it up if you're still interested.

Hi Heather, is it the test that's left now ? or the updated patch from Ryan yet to be reviewed ?(In reply to Heather Arthur [:harth] from comment #20) > Avinash, looks like Ryan doesn't have time. You could pick it up if you're > still interested.

Adding tests to the previous patch from Ryan.

Comment on attachment 791061 [details] [diff] [review] Adding tests Review of attachment 791061 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for picking this up. A few tiny things, that I'll fix before checking in. ::: browser/devtools/netmonitor/netmonitor-view.js @@ +372,5 @@ > + let selected = this.selectedItem.attachment; > + > + browser.selectedTab = browser.addTab(selected.url); > + }, > + trailing whitespace here. ::: browser/devtools/netmonitor/test/browser_net_open_request_in_tab.js @@ +31,5 @@ > + > + aDebuggee.performRequests(1); > + function cleanUp(){ > + teardown(aMonitor); > + finish(); teardown(aMonitor).then(finish); ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ +188,5 @@ > > +<!-- LOCALIZATION NOTE (netmonitorUI.context.newtab): This is the label > + - for the Open in New Tab menu item displayed in the context menu of the > + - network container --> > +<!ENTITY netmonitorUI.context.newtab "Open in New Tab"> newtab -> newTab to go along with rest of style in this file.

Comment on attachment 791061 [details] [diff] [review] Adding tests Review of attachment 791061 [details] [diff] [review]: ----------------------------------------------------------------- Oops, one last thing: ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ +194,5 @@ > +<!-- LOCALIZATION NOTE (netmonitorUI.context.newtab.accesskey): This is the access key > + - for the Open in New Tab menu item displayed in the context menu of the > + - network container --> > +<!ENTITY netmonitorUI.context.newtab.accesskey "N"> > + Gonna make this "O" for "Open"

Backed out in https://hg.mozilla.org/integration/fx-team/rev/e1210bbb856d for browser-chrome bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=26663825&tree=Fx-Team

Okay, this failure only crops up when running all the devtools tests, not just the netmonitor ones. I get: 7:01.20 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_jsterm.js | uncaught exception - ReferenceError: ok is not defined at chrome://mochitests/content/browser/browser/devtools/netmonitor/test/browser_net_open_request_in_tab.js:24 This error occurs in the middle of the webconsole test run, or on the profiler test runs on tbpl. "ok is not defined" I've seen when the test runner is torn down before ok() gets called, so that callback is called after the test finishes. No doubt it's because we never remove the TabOpen listener. Doh, I should have caught that )=

removing the "TabOpen" EventListener using openUILinkIn(url, "tab") to open the tab

Comment on attachment 791622 [details] [diff] [review] 889944.patch So.. I asked to use openUILinkIn so that the new tab that opens up is opened near to the existing tab, which requires a third argument as an options object like {relatedToCurrent: true} , but after I looked, addTab can also do that with the second argument as the options object [0] . So please revert back to addTab itself and use the options object to the browser to open the new tab right next to the current one. [0] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1403 PS: 2 small things, since in this patch, you were no longer using the browser variable, there was no need to get it :) . But since you are reverting back to using browser.addTab, there is a shortcut to get the browser element, i.e. |win.gBrowser|

- revert back to addTab with the relatedToCurrent options object - use win.gBrowser ;)

LGTM, I am not a peer, so my r+ is not valid :) . Lets get it through try this time, but first, the patch has been bitrotted due to the "Edit and resend" bug. Can you please rebase and attach a patch ? I can push it to try after that.

I've made my own version too.

- Rebased - minor changes to the test

Comment on attachment 791996 [details] [diff] [review] 889944.patch >+ openRequestInTab: function() { >+ let win = Services.wm.getMostRecentWindow("navigator:browser"); >+ let browser = win.gBrowser; >+ >+ let selected = this.selectedItem.attachment; >+ >+ browser.selectedTab = browser.addTab(selected.url,{ relatedToCurrent: true }); use this instead of gBrowser: win.openUILinkIn(selected.url, "tab", { relatedToCurrent: true });

(In reply to Girish Sharma [:Optimizer] from comment #29) > Comment on attachment 791622 [details] [diff] [review] > 889944.patch > > So.. I asked to use openUILinkIn so that the new tab that opens up is opened > near to the existing tab, which requires a third argument as an options > object like {relatedToCurrent: true} , but after I looked, addTab can also > do that with the second argument as the options object [0] . So please > revert back to addTab itself and use the options object to the browser to > open the new tab right next to the current one. No, openUILinkIn is the higher-level API and smarter than addTab. You should use it.

Comment on attachment 791849 [details] [diff] [review] 889944-add-open-in-new-tab-context-menu-item-on-resources.patch Avinash has already taken this bug, cancelling review on this patch.

Comment on attachment 791996 [details] [diff] [review] 889944.patch Review of attachment 791996 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Avinash, and sorry for the back and forth on addTab. Let's go with OpenUILinkIn() then. One more thing to fix, and this should be good to go. r-ing because we need to fix this: ::: browser/devtools/netmonitor/test/browser_net_open_request_in_tab.js @@ +21,5 @@ > + RequestsMenu.selectedItem = requestItem; > + > + gBrowser.tabContainer.addEventListener("TabOpen",function(event){ > + ok(true, "A new tab has been opened "); > + gBrowser.tabContainer.removeEventListener("TabOpen", this, false); "this" won't do it. You'll need to give the function a name: addEventListener("TabOpen", function onOpen(event) { removeEventListener("TabOpen", onOpen, false); }, false); ::: browser/locales/en-US/chrome/browser/devtools/netmonitor.dtd @@ +186,5 @@ > - for the "Edit and Resend" menu item displayed in the context menu for a request --> > <!ENTITY netmonitorUI.summary.editAndResend.accesskey "R"> > > +<!-- LOCALIZATION NOTE (netmonitorUI.context.newTab): This is the label > + - for the Open in New Tab menu item displayed in the context menu of the trailing whitespace @@ +191,5 @@ > + - network container --> > +<!ENTITY netmonitorUI.context.newTab "Open in New Tab"> > + > +<!-- LOCALIZATION NOTE (netmonitorUI.context.newTab.accesskey): This is the access key > + - for the Open in New Tab menu item displayed in the context menu of the trailing whitespace

using openUILinkIn, giving name to the test function eventListener

Comment on attachment 795064 [details] [diff] [review] 889944.patch Review of attachment 795064 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Thanks again Avinash! One last try run: https://tbpl.mozilla.org/?tree=Try&rev=b669571c069d

I told this personally to Avinash too and am repeating here too: The test attached in this patch is not doing what it actually should do, it directly calls the method which creates a new tab, and then it just checks for the creation of a new tab. What the test actually is testing is whether the openUILinkIn method opens a new tab or not. In reality, the test should be testing the feature "add a context menu entry that opens the url in a new tab" by actually simulating the context menu click and checking the new tab's url. We might as well land this bug and improve the test in a followup as this is a very much requested feature, that even I am looking forward too. But as of current test, any regression related to this feature could not be detected using the tests alone. I apologize if I sound rude here.

(In reply to Girish Sharma [:Optimizer] from comment #40) > The test attached in this patch is not doing what it actually should do, it > directly calls the method which creates a new tab, and then it just checks > for the creation of a new tab. What the test actually is testing is whether > the openUILinkIn method opens a new tab or not. In reality, the test should > be testing the feature "add a context menu entry that opens the url in a new > tab" by actually simulating the context menu click and checking the new > tab's url. Sure, we can add a UI-driven test in a separate bug. This is an API test, and it tests the core functionality. The context menu calls the API method directly with no arguments. That could change, but I think this is sufficient.