Stefan Arentz | :st3fan | ⏰ EST | he/him Reporter
	Description • 11 years ago

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.

	Victor Porof [:vporof][:vp]
	Comment 1 • 11 years ago

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.

	Victor Porof [:vporof][:vp]
	Comment 2 • 11 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 5 • 7 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 7 years ago

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

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 7 • 7 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 9 • 7 years ago

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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 10 • 7 years ago
mozreview-review

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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 12 • 7 years ago
mozreview-review

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

	Pulsebot
	Comment 15 • 7 years ago

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/287de7a0ca04
Add the ability to show/hide more columns in the network panel. r=Honza

	Carsten Book [:Tomcat]
	Comment 16 • 7 years ago
bugherder

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

	Sebastian Zartner [:sebo]
	Comment 17 • 7 years ago

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

Sebastian