861335 - Link network requests from console to network panel

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Mihai Sucan [:msucan]]

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.

Comment 2

[kud]

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

Comment 3

[Patrick Brosset <:pbro>]

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.

Comment 5

[Tim Nguyen :ntim]

Attachment: netmonitor-console.png

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

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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".

Comment 12

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(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).

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 16

[Joe Walker [:jwalker]]

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

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: WIP v1

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

Comment 18

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

This one works and pases all tests locally.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 21

[Jeff Griffiths (:canuckistani) (:⚡︎)]

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.

Comment 22

[Sebastian Zartner [:sebo]]

(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

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v3

That was just a one line change.

Comment 25

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

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

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Sure thing.

Comment 27

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

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.

Comment 28

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

(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`.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v4

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.

Comment 30

[Pulsebot]

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

Comment 31

[Carsten Book [:Tomcat]]

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

Comment 32

[Pulsebot]

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

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v5

Race fix.

Comment 36

[Pulsebot]

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

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e142ec232fe1

Comment 38

[Phil Ringnalda (:philor)]

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.

Comment 39

[Phil Ringnalda (:philor)]

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).

Comment 40

[Phil Ringnalda (:philor)]

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.

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 42

[Pulsebot]

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

Comment 43

[nayinain]

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

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 46

[Phil Ringnalda (:philor)]

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.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4601be431d56

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4601be431d56

Comment 49

[Sebastian Zartner [:sebo]]

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

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 51

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 52

[Sebastian Zartner [:sebo]]

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

Comment 53

[Panos Astithas (he/him) [:past] (please ni?)]

Whoops, I missed that incomplete comment, thanks Sebastian!

Comment 54

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 55

[Sayed Mohammad Amir [:smamir]]

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