The Network monitor panel should have a new column (in the request-list) that shows protocol version (HTTP/1.1, HTTP/2, etc.) Honza

Instead of create a new protocol column, since ALL HTTP/2 requests must be encrypted by HTTPS(encrypted request will show a green lock icon

I'm not sure what I'm needinfo-ed for… Is there a question I need to answer?

Sorry Blake, I put some emojis and it eats the rest of comment... Instead of create a new protocol column, since ALL HTTP/2 requests must be encrypted by HTTPS(encrypted request will show a green lock icon in `domain` column). Is it reasonable if we provide a new kind of lock icon to denote its encrypted and the request is going through HTTP/2? (Ex: a lock icon with `2 in it :~) We can use tooltip to hint user its going through HTTP/2 as well. (With this approach we don't need to wait for Bug 862855 before implement this bug)

So, that would be a way to show it, but I don't really feel like that's where people would expect to find that information, and it seems like an odd overriding of a common icon to try and show more information… I think that a better way might be to change the method to `GET/2` (or `POST/2` or whatever), although even there, I'm not sure how that would work with stuff the server pushes to the client… (It might be nice to show all HTTP/2 requests/responses that use the same connection slightly indented under that connection or something.) Anyways, since we don't have a designer, waiting for a new lock icon probably won't be any quicker than waiting for bug 862855, so we might as well not do that. ;)

(In reply to Blake Winton (:bwinton) (:☕️) from comment #4) > Anyways, since we don't have a designer, waiting for a new lock icon > probably won't be any quicker than waiting for bug 862855, so we might as > well not do that. ;) Yes, I am also voting for fixing bug 862855 first and then append new columns. Note that there is yet another bug report (bug 1344523) asking for new column with IP address and I am expecting more such requests in the future. Honza

Bug 862855 has been fixed, so this should be easier to act on now.

The new "Protocol" column should also indicate whether SPDY is used. See also bug 736882. We might also want to introduce Scheme column (http, https, data ..) Can be done as part of this bug or separate bug report. Honza

I'd be happy to give it a go. From a quick grep I understand I must hook a "createFactory" in http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/request-list-item.js#134 , similar to the status one, and read the value of "httpVersion" (although I guess this doesn't take care of other schemas?). Also, I am not sure what else must change in order to make the column appear.

You might want to get inspiration from bug 1344523 which also adds another column.

Yep, agreed. Following the bug 344523 is the best way to go. @Vangelis: So, should I assign this one to you? Honza

@Honza, yes I'd be happy to give it a go. @Tim, thanks for the help, it certainly shows the way! After a "hg pull -u", working on > hg wip > @ 352132:731639fccc70 cbook tip central merge mozilla-inbound to mozilla-central a=merge I don't see "const HEADERS = [" spanning in multiple lines (https://bugzilla.mozilla.org/attachment.cgi?id=8856206&action=diff#a/devtools/client/netmonitor/src/components/request-list-header.js_sec2 ) but "const { HEADERS } = require("../constants");" I guess it's a matter of a patch not "having landed" yet? If some hg magic can help, let me know. PS I hope I use the "needinfo?" is the correct way, and I am not producing extra noise.

(In reply to Vangelis Katsikaros from comment #12) > @Honza, yes I'd be happy to give it a go. > > @Tim, thanks for the help, it certainly shows the way! > > After a "hg pull -u", working on > > hg wip > > @ 352132:731639fccc70 cbook tip central merge mozilla-inbound to mozilla-central a=merge > > I don't see "const HEADERS = [" spanning in multiple lines > (https://bugzilla.mozilla.org/attachment.cgi?id=8856206&action=diff#a/ > devtools/client/netmonitor/src/components/request-list-header.js_sec2 ) but > "const { HEADERS } = require("../constants");" > > I guess it's a matter of a patch not "having landed" yet? If some hg magic > can help, let me know. > > PS I hope I use the "needinfo?" is the correct way, and I am not producing > extra noise. That was moved in devtools/client/netmonitor/src/constants.js but the rest should stay the same.

https://bugzilla.mozilla.org/attachment.cgi?id=8856656 adds the column and simply shows the http version. Issues ignored so far: * scheme column (http, https, data ..), as mentioned above * it seems httpVersion is not available in requests other than GET (see demo.png)?

Comment on attachment 8856656 [details] Bug 1345489 - Introduce a new column for protocol version. https://reviewboard.mozilla.org/r/128582/#review131000 ::: devtools/client/netmonitor/src/constants.js:131 (Diff revision 1) > boxName: "icon-and-file", > canFilter: false, > }, > { > + name: "protocol", > + canFilter: true, If you use canFilter: true, you have to add a new case in isFlagFilterMatch in devtools/client/netmonitor/src/utils/filter-text-utils.js canFilter defines whether we want to support a flag for the specified column in the search input (for example you can search status-code:300 in the searchbox to search by status code). ::: devtools/client/netmonitor/src/reducers/ui.js:25 (Diff revision 1) > > const Columns = I.Record({ > status: true, > method: true, > file: true, > + protocol: true, We probably want to hide this new column by default to save space (people will have to turn it on via the context menu). So this should be set to false. You'll need to do changes in: - devtools/client/preferences/devtools.js (devtools.netmonitor.hiddenColumns) If this lands before bug 1344523, you'll also have to change: - devtools/client/netmonitor/src/middleware/prefs.js

Tim thanks for the feedback. I applied all your changes and made sure the filtering works if there is no value. Issues still ignored: * scheme (http, https, data ..), as mentioned above * it seems httpVersion is not available in requests other than GET (see demo.png)?

Comment on attachment 8856656 [details] Bug 1345489 - Introduce a new column for protocol version. https://reviewboard.mozilla.org/r/128582/#review131354 ::: devtools/client/netmonitor/src/middleware/prefs.js:38 (Diff revision 2) > case RESET_COLUMNS: > - Prefs.hiddenColumns = []; > + Prefs.hiddenColumns = ["protocol"]; > break; I would remove this altogether, and use TOGGLE_COLUMN's code: https://bugzilla.mozilla.org/show_bug.cgi?id=1344523#c27

Comment on attachment 8856656 [details] Bug 1345489 - Introduce a new column for protocol version. https://reviewboard.mozilla.org/r/128582/#review131988 Great work here! Please see my inline comments Honza ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:359 (Diff revision 3) > .requests-list-security-and-domain { > width: 14vw; > } > > +.requests-list-protocol { > + width: 8vw; We can decrease to 7vw to save space ::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1329 (Diff revision 3) > > .requests-list-security-and-domain { > width: 16vw; > } > > + .requests-list-protocol { Why it's here again? ::: devtools/client/netmonitor/src/middleware/prefs.js (Diff revision 3) > .map(([column, shown]) => column); > break; > - > - case RESET_COLUMNS: > - Prefs.hiddenColumns = []; > - break; I am seeing a collision here when applying this patch. Will need rebase on current HEAD before landing. ::: devtools/client/netmonitor/src/utils/sort-predicates.js:54 (Diff revision 3) > const result = compareValues(firstUrl, secondUrl); > return result || waterfall(first, second); > } > > +function protocol(first, second) { > + const result = compareValues(first.protocol, second.protocol); Should be: const result = compareValues(first.httpVersion, second.httpVersion); ... to fix the sorting.

Comment on attachment 8856656 [details] Bug 1345489 - Introduce a new column for protocol version. https://reviewboard.mozilla.org/r/128582/#review131988 > Why it's here again? I saw the rest of the content "duplicated" in the `@media` block, so I blindly copied it there too. Should I remove it?

Rebased and resolved conflicts. >::: devtools/client/netmonitor/src/middleware/prefs.js:38 >(Diff revision 2) >> case RESET_COLUMNS: >> - Prefs.hiddenColumns = []; >> + Prefs.hiddenColumns = ["protocol"]; >> break; > >I would remove this altogether, and use TOGGLE_COLUMN's code: https://bugzilla.mozilla.org/show_bug.cgi?id=1344523#c27 Took care of this now that the other bug has landed.

(In reply to Vangelis Katsikaros from comment #23) > Comment on attachment 8856656 [details] > Bug 1345489 - Introduce a new column for protocol version. > > https://reviewboard.mozilla.org/r/128582/#review131988 > > > Why it's here again? > > I saw the rest of the content "duplicated" in the `@media` block, so I > blindly copied it there too. Should I remove it? Yes please! Otherwise it's ready to land. Thanks, Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #26) > (In reply to Vangelis Katsikaros from comment #23) > > Comment on attachment 8856656 [details] > > Bug 1345489 - Introduce a new column for protocol version. > > > > https://reviewboard.mozilla.org/r/128582/#review131988 > > > > > Why it's here again? > > > > I saw the rest of the content "duplicated" in the `@media` block, so I > > blindly copied it there too. Should I remove it? > Yes please! > > Otherwise it's ready to land. > > Thanks, > Honza Done!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1e267643be30 Introduce a new column for protocol version. r=Honza

This issue is verified as fixed on latest Nightly 55.0a1 (2017-04-19) on the following OSes: Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS 10.11.6. The protocol's new column works as expected.