Bug 855544 landed and now we can remove the network panel from the web console. All network logging messages in the web console output should open the specific network request in the netmonitor.

After discussing on IRC, I would like to say my opinion (instead of creating another issue). 1/ The popup is unpleasant and not really efficient to read. Mostly about the content of the answer, you can't read the JSON or whatever else. 2/ It could be great to have network panel as independent like the console is. For instance my layout is: INSPECTOR | RULES ----------------- CONSOLE Could be great to have something like INSPECTOR | RULES ------------------- CONSOLE | NETWORK

The idea of a split network panel (like the split console) is interesting. Network requests are logged in the console, but you get far less information about them as you do in the network panel.

Here's how things can possibly look, I don't know how hard that would be though.

I'd like an estimate of how hard this would be.

It depends on what solution we want. Just switching to the netmonitor panel and selecting the request would be easier than using the netmonitor sidebar in the console panel, as in comment 5. The latter would also need Jordan's refactoring to split the sidebar from netmonitor-view.js. For the first option, which I think I prefer at least for now, I'd say it's medium difficulty. At any rate I don't think fixing this before bug 862341 would be all that useful for end users.

(In reply to Panos Astithas [:past] from comment #8) > It depends on what solution we want. Just switching to the netmonitor panel > and selecting the request would be easier than using the netmonitor sidebar > in the console panel, as in comment 5. The latter would also need Jordan's > refactoring to split the sidebar from netmonitor-view.js. For the first > option, which I think I prefer at least for now, I'd say it's medium > difficulty. I prefer the former rather than the latter, at least for our current purposes. It might be a bit jarring to get switched to a different tool. > At any rate I don't think fixing this before bug 862341 would be all that > useful for end users. Agreed.

Adding a dependency to bug 1149634 per comment 9. Jeff I hope I got your intent right, but if not, let me know.

(In reply to Jeff Griffiths (:canuckistani) from comment #9) > (In reply to Panos Astithas [:past] from comment #8) > > It depends on what solution we want. Just switching to the netmonitor panel > > and selecting the request would be easier than using the netmonitor sidebar > > in the console panel, as in comment 5. The latter would also need Jordan's > > refactoring to split the sidebar from netmonitor-view.js. For the first > > option, which I think I prefer at least for now, I'd say it's medium > > difficulty. > > I prefer the former rather than the latter, at least for our current > purposes. It might be a bit jarring to get switched to a different tool. I got lost trying to parse Jeff's reply... Jeff says "I prefer the former", but Panos' former is "Just switching to the netmonitor panel". Jeff then says "It might be a bit jarring to get switched to a different tool", which Jeff just said he preferred... I guess Jeff meant "I prefer the latter" instead of "former".

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11) ... > I got lost trying to parse Jeff's reply... Jeff says "I prefer the former", > but Panos' former is "Just switching to the netmonitor panel". Jeff then > says "It might be a bit jarring to get switched to a different tool", which > Jeff just said he preferred... Sorry about that. > I guess Jeff meant "I prefer the latter" instead of "former". The original intent is to do something better than the network pop-up when a network request is clicked on in the console. My original user experience idea was just to switch to the net monitor and select the request, which would open the side panel in the netmonitor. While this may be jarring, it's a lot better than what we do now. I'm especially in favour of doing this if it's really easy. I think I would be even more in favour of there being a 'netmonitor' mini icon that users could click on to switch to the netmonitor, because this is the same interaction we use in other places to switch between tools. So - clicking on network requests would do nothing, but clicking on the mini-icon would switch to the netmonitor. Hope this is clear(er).

(In reply to Jeff Griffiths (:canuckistani) from comment #12) > The original intent is to do something better than the network pop-up when a > network request is clicked on in the console. My original user experience > idea was just to switch to the net monitor and select the request, which > would open the side panel in the netmonitor. While this may be jarring, it's > a lot better than what we do now. I'm especially in favour of doing this if > it's really easy. > > I think I would be even more in favour of there being a 'netmonitor' mini > icon that users could click on to switch to the netmonitor, because this is > the same interaction we use in other places to switch between tools. So - > clicking on network requests would do nothing, but clicking on the mini-icon > would switch to the netmonitor. > > Hope this is clear(er). Okay, it sounds like we *don't* need to wait on the netmonitor refactor then, since we're switching to the netmonitor, not trying to re-use it's sub-panel in the console. Flagging Panos to make sure I haven't totally confused everything.

I definitely got it wrong then, thanks for clearing it up everyone!

Irakli, if you are not currently working on this or would like me to take this off your plate, I'd be happy to do so.

Panos: I think you can skip on getting permission from Irakli for taking devtools work off his hands.

Parking my WIP. Works when the netmonitor hasn't been opened yet, breaks otherwise.

This one works and pases all tests locally.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32d08f8001cc

It may be hard to see, but try is actually green. Passing "--tag devtools" to "mach try" seems to confuse treeherder a lot. Going through the dt1/dt2 tests that aren't green and grepping for "Failed:" doesn't come up with any failures.

Looks great! One issue I immediately ran into is this scenario: 1. in the net monitor, set the filter to 'css' 2. click on a network request for a js file expected: not sure - should we reset the filter in the netmonitor to 'all' and ensure we can see the selected request? should we warn that due to filtering the selected request is hidden? actual: the request is not visible in the netmonitor table, but the variable viewer does display the details of the selected request - this is confusing.

(In reply to Jeff Griffiths (:canuckistani) from comment #21) > expected: not sure - should we reset the filter in the netmonitor to 'all' > and ensure we can see the selected request? should we warn that due to > filtering the selected request is hidden? I think it makes more sense to reset the filter, because the user action is to display the request. You may want to tell the user that the filter got reset, though I don't believe it's necessary. Sebastian

I agree that we should reset the filter. I had actually thought about this originally, but it must have slipped my mind.

That was just a one line change.

This patch is pretty big. Give me one more day to finish reviewing it.

Sure thing.

Comment on attachment 8657541 [details] [diff] [review] Patch v3 Review of attachment 8657541 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/netmonitor/netmonitor-controller.js @@ +331,5 @@ > + * A promise resolved once the task finishes. > + */ > + inspectRequest: function(requestId) { > + function inspector() { > + for (let item of NetMonitorView.RequestsMenu.items) { No need for `NetMonitorView.RequestsMenu.items`, the menu itself implements the iterator protocol. for (let item of NetMonitorView.RequestsMenu) should work IIRC. @@ +332,5 @@ > + */ > + inspectRequest: function(requestId) { > + function inspector() { > + for (let item of NetMonitorView.RequestsMenu.items) { > + if (item._value === requestId) { _value has a public accesor, `value`. Side-note: since you only want to find a single item here, you can use `getItemForPredicate` on `NetMonitorView.RequestsMenu` to be more elegant. @@ +338,5 @@ > + found = true; > + deferred.resolve(); > + NetMonitorView.RequestsMenu.filterOn("all"); > + NetMonitorView.RequestsMenu.selectedItem = item; > + NetMonitorView.RequestsMenu._onSelect({ detail: item }); Use the `selectedItem` setter instead on `NetMonitorView.RequestsMenu`, instead of calling private methods. For an API overview, see https://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/ViewHelpers.jsm#748 @@ +348,5 @@ > + > + // Look for the request in the existing ones or wait for it to appear, if > + // the network monitor is still loading. > + let found = false; > + let deferred = promise.defer(); Nit: would like to see these variables declared before being seen by humans when reading `inspector`. If eslint annoyingly complains about hoisted functions not being declared at the top, make `inspector` a variable as well. ::: browser/devtools/webconsole/test/browser_webconsole_netlogging.js @@ +192,5 @@ > // Open the NetworkPanel. The functionality of the NetworkPanel is tested > // within separate test files. > + hud.ui.openNetworkPanel(lastRequest.actor).then(() => { > + let toolbox = gDevTools.getToolbox(hud.target); > + is(toolbox.currentToolId, "netmonitor", "Network panel was opened"); Would like to see a check here for the required item being currently selected in the netmonitor UI. ::: browser/devtools/webconsole/webconsole.js @@ +1971,4 @@ > { > + let toolbox = gDevTools.getToolbox(this.owner.target); > + if (!toolbox) { > + return; When can this happen? A comment would be nice. @@ +1975,4 @@ > } > + return toolbox.selectTool("netmonitor").then(panel => { > + return panel.panelWin.NetMonitorController.inspectRequest(requestId); > + }); This function returns undefined in some cases, or a promise in others. Either rewrite it in Task.js (not sure if the old webconsole uses it at all) or return the same type in every case.

(In reply to Victor Porof [:vporof][:vp] from comment #27) > Comment on attachment 8657541 [details] [diff] [review] > > @@ +338,5 @@ > > + found = true; > > + deferred.resolve(); > > + NetMonitorView.RequestsMenu.filterOn("all"); > > + NetMonitorView.RequestsMenu.selectedItem = item; > > + NetMonitorView.RequestsMenu._onSelect({ detail: item }); > > Use the `selectedItem` setter instead on `NetMonitorView.RequestsMenu`, > instead of calling private methods. > Forgot to mention: if you want to force the "select" callback to be invoked, even if the item is already selected (why would you want to do that btw? a comment to clarify would be nice), you can use `forceSelect` on `NetMonitorView.RequestsMenu`.

Thanks for the review! (In reply to Victor Porof [:vporof][:vp] from comment #27) > Side-note: since you only want to find a single item here, you can use > `getItemForPredicate` on `NetMonitorView.RequestsMenu` to be more elegant. Love it. > Use the `selectedItem` setter instead on `NetMonitorView.RequestsMenu`, > instead of calling private methods. Right, forcing the callback wasn't necessary after all. > ::: browser/devtools/webconsole/webconsole.js > @@ +1971,4 @@ > > { > > + let toolbox = gDevTools.getToolbox(this.owner.target); > > + if (!toolbox) { > > + return; > > When can this happen? A comment would be nice. Good point. We do that often when the method is in an async path where the toolbox could be destroyed in the interim, but this is a click handler, so it shouldn't be possible to ever happen.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4580145&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/ecbb521c3cd4

Looks like it's hitting a race that didn't occur in my testing. I'm testing out some fixes.

Alright, race identified and fixed in the test. Let's give it another try.

Race fix.

Experimentally backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d77d81aaff99 to see if it's causing bug 1203804. FWIW, I wouldn't count on those Try results being the least bit useful, since although I didn't know what chunk to look at to translate my 2-of-4 failure into your whatever-of-7, at least one chunk I looked at was still running tests at the time it timed out, and thus didn't have a chance to leak.

Oh, I see, we do run 7 chunks on Linux debug, we just mostly don't run most of the chunks. Neither Linux32 nor Linux64 dt2 actually finished (not that that would actually make a difference, since the bug 1203804 failure apparently only happens while on top of something pushed to mozilla-inbound in the last day or so).

And it turns out this was it. When you go looking for what in https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c75f9ca74a29&tochange=68ca35b8034b disagreed with you, I'd probably start by looking at bug 1196430 catching a leak we formerly didn't catch.

I'm fairly confident that this patch wasn't at fault for the leaks. The last try run from yesterday was also green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda3152e793c

The patch break the Network Panel in Browser Console(Ctrl-Shift-J). :(

We've wanted to remove messages originating from tabs from the browser console for a long time, so this is to be expected. I didn't handle this case cleanly in this patch however, so I'll file a followup to do just that.

Backed out again for leaks: https://treeherder.mozilla.org/logviewer.html#?job_id=4641706&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/2bfa1927d504

Which didn't do any good, because the backout just moved browser_perf-recording-selected-04.js from leaking in dt2 to leaking in dt1. Starting to look like any devtools patch which touches tests is at risk of either moving a leaking test from one chunk to another, possibly from a chunk which is mostly skipped to one which is run all the time or possibly from running as the first test in a chunk when it has lots of time to wind up not leaking to running as the last test in another chunk when it has no time to hide its leak.

Panos, in comment 23 you said you'll reset the filter when a file isn't displayed with currently selected filter. Though trying the feature in the current Nightly (2015-09-13) the filter is not reset. So the UX is currently this: You have 'JS' set as filter within the 'Network' panel. Within the 'Console' panel you click on a CSS ressource. The display switches to the 'Network' panel, though in there nothing happens. So the user is wondering why he can't see the details to the request he clicked on. Sebastian

This bug has been closing and reopening for too long. Let's file a followup for this.

Release Note Request (optional, but appreciated) [Why is this notable]: [Suggested wording]: [Links (documentation, blog post, etc)]:

Let me provide the info: Release Note Request (optional, but appreciated) [Why is this notable]: It unifies the UI for network request information. [Suggested wording]: Network requests in Console panel link to Network panel instead of opening the info in a popup. [Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#HTTP_requests Furthermore I documented this change here: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#HTTP_requests https://developer.mozilla.org/en-US/Firefox/Releases/43 Sebastian

Whoops, I missed that incomplete comment, thanks Sebastian!

Added to release notes for beta 43, with Sebastian's wording and the first link.

Reproduced with Firefox Nightly 28.0a1(20131206030202) on Windows 7 64 bit. Network requests are linked to Console panel. Verified as fixed with Firefox Beta 43.0b7(20151126120800) UA: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:28.0) Gecko/20100101 Firefox/28.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0