Introduce a new column for protocol version

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: Honza, Assigned: Vangelis Katsikaros (unavailable until May '18), Mentored)

Tracking

(Blocks: 2 bugs, {dev-doc-needed, good-first-bug})

unspecified
Firefox 55
dev-doc-needed, good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +, firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve] [platform-rel-AmazonMusic] [platform-rel-Amazon])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

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

Honza
(Reporter)

Updated

a year ago
Depends on: 862855
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [netmonitor-reserve]
platform-rel: --- → +
Whiteboard: [netmonitor-reserve] → [netmonitor-reserve][platform-rel-AmazonMusic][platform-rel-Amazon]
Blocks: 1307743
QA Contact: ciprian.georgiu
Blocks: 1348737
No longer blocks: 1307743
Priority: P3 → P2
Whiteboard: [netmonitor-reserve][platform-rel-AmazonMusic][platform-rel-Amazon] → [netmonitor] [platform-rel-AmazonMusic] [platform-rel-Amazon]
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
Flags: needinfo?(bwinton)
I'm not sure what I'm needinfo-ed for…  Is there a question I need to answer?
Flags: needinfo?(bwinton)
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)
Flags: needinfo?(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.  ;)
Flags: needinfo?(bwinton)
(Reporter)

Comment 5

a year ago
(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

a year ago
Bug 862855 has been fixed, so this should be easier to act on now.
(Reporter)

Updated

a year ago
Mentor: odvarko
Keywords: good-first-bug

Updated

a year ago
Keywords: dev-doc-needed
(Reporter)

Updated

a year ago
Duplicate of this bug: 736882
(Reporter)

Comment 8

a year ago
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.
Flags: needinfo?(odvarko)

Comment 10

a year ago
You might want to get inspiration from bug 1344523 which also adds another column.
(Reporter)

Comment 11

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

Honza
Flags: needinfo?(odvarko)
@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.
Flags: needinfo?(ntim.bugs)

Comment 13

a year ago
(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.
Flags: needinfo?(ntim.bugs)
(Reporter)

Updated

a year ago
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
Priority: P2 → P3
Whiteboard: [netmonitor] [platform-rel-AmazonMusic] [platform-rel-Amazon] → [netmonitor-reserve] [platform-rel-AmazonMusic] [platform-rel-Amazon]
Comment hidden (mozreview-request)
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

a year ago
mozreview-review
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 hidden (mozreview-request)
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

a year ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 22

a year ago
mozreview-review
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.
Attachment #8856656 - Flags: review?(odvarko) → review+
(Assignee)

Comment 23

a year ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
(Reporter)

Comment 26

a year ago
(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 hidden (mozreview-request)
(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

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

Updated

a year ago
Blocks: 1356126

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e267643be30
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1

Updated

a year ago
Blocks: 1356868
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+

Updated

a year ago
Duplicate of this bug: 859049
You need to log in before you can comment on or make changes to this bug.