1345489 - Introduce a new column for protocol version

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

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

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

Honza

Comment 1

[[e-mail]]

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

Comment 2

[Blake Winton (:bwinton) (:☕️)]

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

Comment 3

[[e-mail]]

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)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

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

Comment 5

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

(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

Comment 6

[Tim Nguyen :ntim]

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

Comment 8

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

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

Comment 9

[Vangelis Katsikaros]

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.

Comment 10

[Tim Nguyen :ntim]

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

Comment 11

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

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

Honza

Comment 12

[Vangelis Katsikaros]

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

Comment 13

[Tim Nguyen :ntim]

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

Comment 14

[Vangelis Katsikaros]

Attachment: Bug 1345489 - Introduce a new column for protocol version.

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

Comment 15

[Vangelis Katsikaros]

Attachment: demo.png

Comment 16

[Vangelis Katsikaros]

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 17

[Tim Nguyen :ntim]

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

Comment 18

[Vangelis Katsikaros]

Comment on attachment 8856656 [details]
Bug 1345489 - Introduce a new column for protocol version.

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

Comment 19

[Vangelis Katsikaros]

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 20

[Tim Nguyen :ntim]

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 21

[Vangelis Katsikaros]

Comment on attachment 8856656 [details]
Bug 1345489 - Introduce a new column for protocol version.

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

Comment 22

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

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 23

[Vangelis Katsikaros]

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?

Comment 24

[Vangelis Katsikaros]

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.

Comment 25

[Vangelis Katsikaros]

Comment on attachment 8856656 [details]
Bug 1345489 - Introduce a new column for protocol version.

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

Comment 26

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

(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

Comment 27

[Vangelis Katsikaros]

Comment on attachment 8856656 [details]
Bug 1345489 - Introduce a new column for protocol version.

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

Comment 28

[Vangelis Katsikaros]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e267643be30
Introduce a new column for protocol version. r=Honza

Comment 30

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/1e267643be30

Comment 31

[Ciprian Georgiu, Desktop QA]

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.