Introduce a new column for protocol version

VERIFIED FIXED in Firefox 55

Status

enhancement
P1
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: Honza, Assigned: vkatsikaros, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-needed, good-first-bug})

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +, firefox55 verified)

Details

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

Attachments

(2 attachments)

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

Honza
Depends on: 862855
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [netmonitor-reserve]
platform-rel: --- → +
Whiteboard: [netmonitor-reserve] → [netmonitor-reserve][platform-rel-AmazonMusic][platform-rel-Amazon]
QA Contact: ciprian.georgiu
Blocks: netmonitor-phaseII
No longer blocks: netmonitor-html
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)
(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

2 years ago
Bug 862855 has been fixed, so this should be easier to act on now.
Mentor: odvarko
Keywords: good-first-bug

Updated

2 years ago
Keywords: dev-doc-needed
Duplicate of this bug: 736882
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
Assignee

Comment 9

2 years ago
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

2 years ago
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
Flags: needinfo?(odvarko)
Assignee

Comment 12

2 years ago
@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

2 years 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)
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)
Assignee

Comment 15

2 years ago
Posted image demo.png
Assignee

Comment 16

2 years ago
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

2 years 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)
Assignee

Comment 19

2 years ago
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

2 years 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

2 years 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

2 years 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?
Assignee

Comment 24

2 years ago
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)
(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)
Assignee

Comment 28

2 years ago
(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

2 years 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

2 years ago
Blocks: 1356126

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e267643be30
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.3 - Apr 17
Priority: P3 → P1
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
Flags: qe-verify+

Updated

2 years ago
Duplicate of this bug: 859049

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.