862855 - Add the ability to show/hide more columns in the network panel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Right now it is not easy to see whether a request is on HTTP or HTTPS. Which is very valuable information while debugging or auditing.

Comment 1

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

It may also be nice to show other things too, like request type, local and remote IP, protocol etc. All columns should be able to be hidden or shown via a settings menu or context menu.

Comment 2

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

Dane MacMillan says in bug 927052:

"A very useful bit of information per request is the status code (200, 404, etc). The only way to view this information is by clicking the request. The color of the orbs only hint at the actual status code (green for 2xx, red for 4xx)."

See this short discussion:

https://hacks.mozilla.org/2013/06/network-monitor-now-in-firefox-beta/comment-page-1/#comment-2151474

Comment 4

[Tim Nguyen :ntim]

Attachment: Bug 862855 - Add the ability to show/hide more columns in the network panel.

Review commit: https://reviewboard.mozilla.org/r/125756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125756/

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for working on this!

I am having trouble to apply the patch.

patching file devtools/client/netmonitor/src/components/request-list-content.js
Hunk #1 FAILED at 214
Hunk #2 succeeded at 237 with fuzz 1 (offset 3 lines).
1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/request-list-content.js.rej
patching file devtools/client/netmonitor/src/components/request-list-item.js
Hunk #2 FAILED at 57
Hunk #3 succeeded at 78 with fuzz 1 (offset -1 lines).
Hunk #4 FAILED at 124
2 out of 4 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/request-list-item.js.rej

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Note: the attached patch is based on a patch from bug 1350234

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: header-context-menu.png

Thanks for the patch Tim!

Couple of comments:

* The context menu doesn't use check-box to indicate what column is currently visible (see the screenshot).
* If I remove all columns there is no way to get them back since the are with no col label doesn't support the context-menu.
* I think that it shouldn't be possible to remove the last column (the related item in the context menu should be disabled).

Honza

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125756/diff/1-2/

Comment 9

[Tim Nguyen :ntim]

(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> Created attachment 8853969 [details]
> header-context-menu.png
> 
> Thanks for the patch Tim!
> 
> Couple of comments:
> 
> * The context menu doesn't use check-box to indicate what column is
> currently visible (see the screenshot).
> * If I remove all columns there is no way to get them back since the are
> with no col label doesn't support the context-menu.
> * I think that it shouldn't be possible to remove the last column (the
> related item in the context menu should be disabled).
> 
> Honza

I've addressed all your comments, can you check if everything looks alright ?

Thanks!

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

https://reviewboard.mozilla.org/r/125756/#review128816

Very well done, just a couple of nits.

All points from my comment #7 fixed nicely!

Feedback+

Honza

::: devtools/client/locales/en-US/netmonitor.properties:530
(Diff revision 2)
>  # in the network toolbar for the performance analysis button.
>  netmonitor.toolbar.perf=Toggle performance analysis…
>  
> +# LOCALIZATION NOTE (netmonitor.toolbar.resetColumns): This is the label
> +# displayed in the network table header context menu.
> +netmonitor.toolbar.resetColumns=Reset columns

I think we should capitalize the second word too => "Reset Columns"

::: devtools/client/netmonitor/src/middleware/prefs.js:18
(Diff revision 2)
>  const { Prefs } = require("../utils/prefs");
>  const { getRequestFilterTypes } = require("../selectors/index");
>  
>  /**
>    * Whenever the User clicks on a filter in the network monitor, save the new
>    * filters for future tabs

Please update this comment.

::: devtools/client/netmonitor/src/request-list-header-context-menu.js:43
(Diff revision 2)
> +        label: L10N.getStr(`netmonitor.toolbar.${stringMap[column] || column}`),
> +        type: "checkbox",
> +        checked: shown,
> +        click: () => this.toggleColumn(column),
> +
> +        // We don't want to allow disabling the last visible column

nit: we don't want to allow hiding the last visible column.

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125756/diff/2-3/

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

https://reviewboard.mozilla.org/r/125756/#review128854

Looks great!

R+ assuming Try is green

Thanks Tim!

Honza

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125756/diff/3-4/

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8853655 [details]
Bug 862855 - Add the ability to show/hide more columns in the network panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125756/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/287de7a0ca04
Add the ability to show/hide more columns in the network panel. r=Honza

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/287de7a0ca04

Comment 17

[Sebastian Zartner [:sebo]]

I can verify that it's working as expected in latest Nightly 55.0a1 (2017-04-06).

Sebastian