1360495 - Add response headers columns

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Comment 1

[Tim Nguyen :ntim]

Attachment: chrome-response-headers.png

Comment 2

[zmf0507]

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.

Comment 3

[Tim Nguyen :ntim]

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

Comment 4

[Michael Hoffmann]

zmf0507, are you working on this bug?  If not, I might be interested in taking it.

Comment 5

[Michael Hoffmann]

Attachment: 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.

Comment 6

[Tim Nguyen :ntim]

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.

Comment 7

[Michael Hoffmann]

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.

Comment 8

[Tim Nguyen :ntim]

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

Comment 9

[Michael Hoffmann]

Attachment: bug1360495.patch

I'm attaching an updated patch following your suggestions, with the exception of the hiddenColumns point which I'll do later.

Comment 10

[Michael Hoffmann]

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.

Comment 11

[Francesco Lodolo [:flod]]

It's definitely good to keep them hardcoded and not exposed for localization in the properties file.

Comment 12

[Tim Nguyen :ntim]

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.

Comment 13

[Michael Hoffmann]

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

Comment 14

[Tim Nguyen :ntim]

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

Comment 15

[Michael Hoffmann]

Attachment: bug1360495.patch

I've assumed that I can use object spread.  So this patch depends on Bug 1375689.

Comment 16

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

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.

Comment 17

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1388551f5096cc4221860d759c7a5e64469e5f20

Honza

Comment 18

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

(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

Comment 19

[Tim Nguyen :ntim]

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.

Comment 20

[Michael Hoffmann]

Attachment: bug1360495.patch

Thanks for the review! I've addressed all comments.

Comment 21

[Michael Hoffmann]

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?

Comment 22

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

(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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b475a96cb8aa
Add response header columns in NetMonitor. r=Honza

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b475a96cb8aa

Comment 25

[Saheda Reza [:Antora]]

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