All users were logged out of Bugzilla on October 13th, 2018

Link network requests from console to network panel

RESOLVED FIXED in Firefox 43

Status

P1
normal
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: msucan, Assigned: past)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-complete})

Trunk
Firefox 43
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox43+ fixed, relnote-firefox 43+)

Details

(Whiteboard: [console-papercuts][polish-backlog][testday-20151127])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Priority: -- → P2
(Reporter)

Updated

5 years ago
Depends on: 868382
(Reporter)

Updated

5 years ago
Duplicate of this bug: 986693

Comment 2

4 years ago
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.
Whiteboard: [console-papercuts]
Summary: Remove the Network Panel → Link network requests from console to network panel
Duplicate of this bug: 1125985
Depends on: 862341
Created attachment 8555335 [details]
netmonitor-console.png

Here's how things can possibly look, I don't know how hard that would be though.
Assignee: nobody → rFobic
Duplicate of this bug: 1135710
Whiteboard: [console-papercuts] → [console-papercuts][devedition-40]
Priority: P2 → P1
I'd like an estimate of how hard this would be.
Flags: needinfo?(rFobic)
Flags: needinfo?(past)
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.
Flags: needinfo?(past)
(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.
Depends on: 1149634
(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.
Flags: needinfo?(past)
I definitely got it wrong then, thanks for clearing it up everyone!
No longer depends on: 1149634
Flags: needinfo?(past)
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.
Assignee: rFobic → past
Flags: needinfo?(rFobic)
Whiteboard: [console-papercuts][devedition-40] → [console-papercuts][polish-backlog]
Status: NEW → ASSIGNED
Created attachment 8656650 [details] [diff] [review]
WIP v1

Parking my WIP. Works when the netmonitor hasn't been opened yet, breaks otherwise.
Created attachment 8657106 [details] [diff] [review]
Patch v2

This one works and pases all tests locally.
Attachment #8656650 - Attachment is obsolete: true
Attachment #8657106 - Flags: review?(vporof)
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.
Created attachment 8657541 [details] [diff] [review]
Patch v3

That was just a one line change.
Attachment #8657106 - Attachment is obsolete: true
Attachment #8657106 - Flags: review?(vporof)
Attachment #8657541 - Flags: review?(vporof)
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.
Attachment #8657541 - Flags: review?(vporof) → review+
(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`.
Created attachment 8658143 [details] [diff] [review]
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.
Attachment #8657541 - Attachment is obsolete: true
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4580145&repo=fx-team
Flags: needinfo?(past)
Looks like it's hitting a race that didn't occur in my testing. I'm testing out some fixes.
Flags: needinfo?(past)
Alright, race identified and fixed in the test. Let's give it another try.
Created attachment 8659275 [details] [diff] [review]
Patch v5

Race fix.
Attachment #8658143 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e142ec232fe1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
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.
Blocks: 1203804
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

Comment 43

3 years ago
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.
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.
https://hg.mozilla.org/mozilla-central/rev/4601be431d56
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This bug has been closing and reopening for too long. Let's file a followup for this.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Flags: needinfo?(past)
Resolution: --- → FIXED
Depends on: 1204481
Depends on: 1204484
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
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
Keywords: dev-doc-complete
Whoops, I missed that incomplete comment, thanks Sebastian!
QA Whiteboard: [good first verify]
Added to release notes for beta 43, with Sebastian's wording and the first link.
tracking-firefox43: --- → +
Depends on: 1211525
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
Whiteboard: [console-papercuts][polish-backlog] → [console-papercuts][polish-backlog][testday-20151127]
Blocks: 1064458
relnote-firefox: ? → 43+

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.