If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add response headers columns

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Developer Tools: Netmonitor
P3
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: ntim, Assigned: Michael Brennan)

Tracking

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

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

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
Keywords: good-first-bug
Priority: -- → P3
(Reporter)

Comment 1

5 months ago
Created attachment 8863132 [details]
chrome-response-headers.png
(Reporter)

Updated

5 months ago
Keywords: dev-doc-needed

Comment 2

4 months ago
please tell me how to fix this bug..i mean i am new to this network and dont get to see the code where i can make changes to fix the bug. please anyone help.
(Reporter)

Comment 3

4 months ago
(In reply to zmf0507 from comment #2)
> please tell me how to fix this bug..i mean i am new to this network and dont
> get to see the code where i can make changes to fix the bug. please anyone
> help.

This is more like a good next bug, but if you'd like to give this a shot, this should be similar to bug 1356871.
(Assignee)

Comment 4

3 months ago
zmf0507, are you working on this bug?  If not, I might be interested in taking it.
Flags: needinfo?(zmf0507)
(Assignee)

Updated

3 months ago
Assignee: nobody → brennan.brisad
Flags: needinfo?(zmf0507)
(Assignee)

Comment 5

3 months ago
Created attachment 8878949 [details] [diff] [review]
bug1360495.patch

Attaching a patch with the first two headers for feedback.

Since `shouldComponentUpdate` is present in many, perhaps all, columns I also added it to the new columns, for consistency.  Have the columns been profiled so that all of them should have an explicit `shouldComponentUpdate`?  Otherwise I would be happy to remove them from the new columns.
Attachment #8878949 - Flags: feedback?(ntim.bugs)
(Reporter)

Comment 6

3 months ago
Comment on attachment 8878949 [details] [diff] [review]
bug1360495.patch

Review of attachment 8878949 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this!
This is a great start, but I think a more scalable approach should be taken here. Eventually, we'd like to have a chrome like UI where you can customize which response headers to show, and even add your own.  While I'm fine with only having a limited set of response headers for this bug (and doing the UI in a different bug), I think we should do some ground work here to prepare for that UI. Here's what I would suggest doing:
- Have a list of response headers in constants.js
- Create a request-list-column-response-header.js component which takes a response header (in the props) and renders it into a column
- Potentially rework the pref system to store visible columns rather than hidden columns (otherwise, we'd have to put all response headers into the hiddenColumns pref, but that can be done in the last step when you manage to get everything working)

Please let us know if you need help (Honza should review this patch btw).

(In reply to Michael Brennan from comment #5)
> Since `shouldComponentUpdate` is present in many, perhaps all, columns I
> also added it to the new columns, for consistency.  Have the columns been
> profiled so that all of them should have an explicit
> `shouldComponentUpdate`?  Otherwise I would be happy to remove them from the
> new columns.

Without shouldComponentUpdate, the components update at *every single* state change, including the ones unrelated to the column.

I think this was added in some bug (can't remember which) to optimize performance.
Attachment #8878949 - Flags: feedback?(ntim.bugs) → feedback+
(Assignee)

Comment 7

3 months ago
Thanks for the feedback and the answer!

Great points!  It makes sense making it all more scalable.  I have two follow up questions.

The `Columns` record in reducers/ui.js explicitly lists all columns, should that one also be modified so that we don't need to list every possible response header there?  Those columns would be a special case then and we'd need to change the code to reflect the fact that `Columns` no longer provides an explicit list of all the possible columns.

Is localization needed?  I guess the headers are what they are, and it might not make sense to translate them.
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 8

3 months ago
(In reply to Michael Brennan from comment #7)
> Thanks for the feedback and the answer!
> 
> Great points!  It makes sense making it all more scalable.  I have two
> follow up questions.
> 
> The `Columns` record in reducers/ui.js explicitly lists all columns, should
> that one also be modified so that we don't need to list every possible
> response header there?  Those columns would be a special case then and we'd
> need to change the code to reflect the fact that `Columns` no longer
> provides an explicit list of all the possible columns.

Assuming you have something like 
let RESPONSE_HEADERS = ["Cache-Control", "Connection"]
in constants.js (you can do differently, this is just an example)

You could either do:

let cols = {
  ...
  size: true,
  ...
};
cols = Object.assign(cols, RESPONSE_HEADERS.reduce((acc, header) => Object.assign(acc, { [header]: false }), {}))

let Columns = new I.Record(cols);


Or you could use a separate record for response header columns:


let ResponseHeaderColumns = new I.Record(
  RESPONSE_HEADERS.reduce((acc, header) => Object.assign(acc, { [header]: false }), {})
);

Choose whatever you judge best :)

> Is localization needed?  I guess the headers are what they are, and it might
> not make sense to translate them.

That's my opinion as well, but you can ask :flod for a more valuable opinion.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 9

3 months ago
Created attachment 8880361 [details] [diff] [review]
bug1360495.patch

I'm attaching an updated patch following your suggestions, with the exception of the hiddenColumns point which I'll do later.
Attachment #8878949 - Attachment is obsolete: true
Attachment #8880361 - Flags: feedback?(ntim.bugs)
(Assignee)

Comment 10

3 months ago
Hi Francesco,
Is it alright to not provide localization for the response headers which might appear in the NetMonitor column headers, and in the context menu?  In that case they will be just be displayed as is, for instance "Cache-Control".

I am guessing this is how they already are displayed in the response panel.
Flags: needinfo?(francesco.lodolo)
It's definitely good to keep them hardcoded and not exposed for localization in the properties file.
Flags: needinfo?(francesco.lodolo)
(Reporter)

Comment 12

3 months ago
Comment on attachment 8880361 [details] [diff] [review]
bug1360495.patch

Review of attachment 8880361 [details] [diff] [review]:
-----------------------------------------------------------------

Works nicely on my machine ! I found a few quirks however: seems like the response header columns could use more space (you can do some CSS work for that), initially, the netmonitor is a bit of a mess since the columns are not hidden (hence the work I've suggested to improve the pref system).

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +494,5 @@
> +  width: 13%;
> +}
> +
> +/* Connection column */
> +.requests-list-connection {

Probably needs to be updated.

::: devtools/client/netmonitor/src/components/request-list-column-response-header.js
@@ +31,5 @@
> +    let header = getResponseHeader(this.props.item, this.props.header);
> +    let headerName = this.props.header;
> +    return (
> +      div({
> +        className: `requests-list-column requests-list-${headerName.toLowerCase()}`,

We probably don't want to style each column separately in the CSS (considering they will be able to add any header they want), I would give a common class and width to all response header columns.

::: devtools/client/netmonitor/src/components/request-list-header.js
@@ +89,5 @@
>          HEADERS.filter((header) => columns.get(header.name)).map((header) => {
>            let name = header.name;
>            let boxName = header.boxName || name;
> +          let label = header.noLocalization
> +              ? header.label || name

nit: You're not using header.label here.

You can simply do:
let label = header.noLocalization ?
  ? name : ...

(btw: 2 space indent on the ternary operator)

::: devtools/client/netmonitor/src/reducers/ui.js
@@ +44,5 @@
> +cols = Object.assign(
> +  cols,
> +  RESPONSE_HEADERS.reduce((acc, header) => Object.assign(acc, { [header]: false }), {})
> +);
> +const Columns = I.Record(cols);

I've just learnt that ES 2017 object spread is supported since Firefox 55

You could use that here for a prettier syntax:

const Columns = I.Record({
  status: true,
  method: true,
  (...)
  ...RESPONSE_HEADERS.reduce((acc, header) => ({ ...acc, [header]: false }), {}),
  waterfall: true,
});

::: devtools/client/netmonitor/src/utils/request-utils.js
@@ +383,5 @@
>    getAbbreviatedMimeType,
>    getEndTime,
>    getFormattedProtocol,
>    getResponseTime,
> +  getResponseHeader,

nit: alphabetical order

::: devtools/client/netmonitor/src/utils/sort-predicates.js
@@ +99,5 @@
> +                               getResponseHeader(second, header) || "");
> +  return result || waterfall(first, second);
> +}
> +
> +var responseHeaders = RESPONSE_HEADERS

nit: use const

@@ +161,5 @@
>    const result = compareValues(first.contentSize, second.contentSize);
>    return result || waterfall(first, second);
>  }
>  
> +let sorters = {

use const here too.
Attachment #8880361 - Flags: feedback?(ntim.bugs) → feedback+
(Assignee)

Comment 13

3 months ago
(In reply to Tim Nguyen :ntim from comment #12)

> Works nicely on my machine ! I found a few quirks however: seems like the
> response header columns could use more space (you can do some CSS work for
> that), initially, the netmonitor is a bit of a mess since the columns are
> not hidden (hence the work I've suggested to improve the pref system).

Unfortunately, Bug 1360457 really messes up almost all my columns (my screen resolution is a bit low, I suspect that makes it  worse), so it's hard to do the CSS here.  So I might attempt to guess this common width I'll assign to the common column class.

> I've just learnt that ES 2017 object spread is supported since Firefox 55

ESLint gives me a parsing error.  Someone else has to enable the syntax in ESLint before I can use it, no?
Flags: needinfo?(ntim.bugs)
(Reporter)

Comment 14

3 months ago
(In reply to Michael Brennan from comment #13)
> (In reply to Tim Nguyen :ntim from comment #12)
> 
> > Works nicely on my machine ! I found a few quirks however: seems like the
> > response header columns could use more space (you can do some CSS work for
> > that), initially, the netmonitor is a bit of a mess since the columns are
> > not hidden (hence the work I've suggested to improve the pref system).
> 
> Unfortunately, Bug 1360457 really messes up almost all my columns (my screen
> resolution is a bit low, I suspect that makes it  worse), so it's hard to do
> the CSS here.  So I might attempt to guess this common width I'll assign to
> the common column class.
I think giving the same width as the cause column for the response header columns should be enough.

> > I've just learnt that ES 2017 object spread is supported since Firefox 55
> 
> ESLint gives me a parsing error.  Someone else has to enable the syntax in
> ESLint before I can use it, no?

Just filed bug 1375689, thanks for catching this issue.
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 15

3 months ago
Created attachment 8881136 [details] [diff] [review]
bug1360495.patch

I've assumed that I can use object spread.  So this patch depends on Bug 1375689.
Attachment #8880361 - Attachment is obsolete: true
Attachment #8881136 - Flags: review?(odvarko)
Comment on attachment 8881136 [details] [diff] [review]
bug1360495.patch

Review of attachment 8881136 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me!

Just couple of nit comments.

R+

Honza

::: devtools/client/netmonitor/src/components/request-list-column-response-header.js
@@ +12,5 @@
> +const { getResponseHeader } = require("../utils/request-utils");
> +
> +const { div } = DOM;
> +
> +const RequestListColumnResponseHeader = createClass({

Please make a comment explaning the purpose of this component.

::: devtools/client/netmonitor/src/components/request-list-header.js
@@ +89,5 @@
>          HEADERS.filter((header) => columns.get(header.name)).map((header) => {
>            let name = header.name;
>            let boxName = header.boxName || name;
> +          let label = header.noLocalization
> +            ? name : L10N.getStr(`netmonitor.toolbar.${header.label || name}`);

The name of the header-column is usually quite long and not fitting into the (rather short) header-label. It would be great if there is a tooltip otherwise it's hard to figure out what is the header associated with the column. It's ok to do this in another bug as a follow up.
Attachment #8881136 - Flags: review?(odvarko) → review+
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1388551f5096cc4221860d759c7a5e64469e5f20

Honza
(In reply to Michael Brennan from comment #15)
> I've assumed that I can use object spread.  So this patch depends on Bug
> 1375689.

Using object spread causes problems when loading the Netmonitor on top of the Launchpad. I am seeing an error:

Error: Cannot find module "../utils/sort-predicates"

(and also for reducers/ui.js)

If I comment out the `...responseHeaders` (in sort-predicates.js) it's fine.
(and similarly fix the ui.js)

Also, object is not supported in the Release channel yet, so the Launchpad would be working only in Nightly (not good).

Please use Object.assign() instead.

Honza
(Reporter)

Comment 19

3 months ago
Comment on attachment 8881136 [details] [diff] [review]
bug1360495.patch

Review of attachment 8881136 [details] [diff] [review]:
-----------------------------------------------------------------

> Also, object is not supported in the Release channel yet, so the Launchpad would be working only in Nightly (not good).

Good point! I agree in this case.

::: devtools/client/netmonitor/src/request-list-header-context-menu.js
@@ +51,3 @@
>        let entry = {
>          id: `request-list-header-${column}-toggle`,
> +        label: label,

nit: that's the same as:

label,

::: devtools/client/preferences/devtools.js
@@ +160,5 @@
>  pref("devtools.netmonitor.panes-network-details-width", 550);
>  pref("devtools.netmonitor.panes-network-details-height", 450);
>  pref("devtools.netmonitor.filters", "[\"all\"]");
> +pref("devtools.netmonitor.visibleColumns",
> +  "[\"cause\",\"contentSize\",\"domain\",\"file\",\"method\",\"status\",\"transferred\",\"type\",\"waterfall\"]"

It would be great if the order actually matched the actual column order.
(Reporter)

Updated

3 months ago
Depends on: 1377013
(Assignee)

Comment 20

3 months ago
Created attachment 8882143 [details] [diff] [review]
bug1360495.patch

Thanks for the review! I've addressed all comments.
Attachment #8881136 - Attachment is obsolete: true
(Assignee)

Updated

3 months ago
Depends on: 1377094
(Assignee)

Comment 21

3 months ago
I was about to add the `checkin-needed` keyword and saw that there was a `dev-doc-needed` there.  Is that something that should be done in this bug, or will someone else do that?  Can I proceed with the checkin?
Flags: needinfo?(odvarko)
Flags: needinfo?(odvarko)
Attachment #8882143 - Flags: review+
(In reply to Michael Brennan from comment #21)
> I was about to add the `checkin-needed` keyword and saw that there was a
> `dev-doc-needed` there.  Is that something that should be done in this bug,
> or will someone else do that?  Can I proceed with the checkin?

Yes, just go ahead. The MDN team shoiuld take care of the docs.

Thanks!
Honza
(Reporter)

Updated

3 months ago
Keywords: checkin-needed

Comment 23

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b475a96cb8aa
Add response header columns in NetMonitor. r=Honza
Keywords: checkin-needed

Comment 24

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b475a96cb8aa
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
The response header column is added.This Feature is implemented on Latest Beta.

Fixing bug is verified on Latest Beta--
Build ID   :20170821193225
User Agent :Mozilla/5.0 (Windows NT 6.1; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0

Tested OS-- Windows7 32bit
QA Whiteboard: [testday-20170818]
You need to log in before you can comment on or make changes to this bug.