Implement Header Panel

VERIFIED FIXED in Firefox 53

Status

()

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

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

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

Attachments

(3 attachments)

This is a breakdown task for bug 1308450 in order to integrate HTTP inspector into the Net panel.

- Implement Header Panel

Updated

2 years ago
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor]

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Priority: P1 → P2
(Assignee)

Updated

a year ago
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Iteration: --- → 53.4 - Jan 9
Priority: P2 → P1
(Assignee)

Updated

a year ago
Depends on: 1328828
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1329575
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Headers panel is ready as well. These is a workaround here to set height: calc(100vh - 135.5px); for fixing XUL layout issue like we did in .properties-view. Properties view will taller than view port without this workaround.
Comment hidden (mozreview-request)
Updated and rebased the patch on top of ParamsPanel (bug 1317650).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Note: there is a $(...) is null TypeError in mochitest (it's weird that only happen in non-e10s mode)

https://treeherder.mozilla.org/logviewer.html#?job_id=67856019&repo=try&lineNumber=11129

but I think it can be fixed after rebasing response panel (bug 1317651)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
mozreview-review
Comment on attachment 8824348 [details]
Bug 1317648 - Implement Headers Panel

https://reviewboard.mozilla.org/r/102878/#review104512

One additional issue:

- it would be great if the header values weren't displayed in quotes. It's confusing - is the real header value quoted or not? Headers like "ETag" contain quoted values - it can be hard to figure out whether the quotes were actually send over the wire, or if they were just added by the UI.

The String Rep has a noQuotes parameter, used e.g. in console.log.

The old XUL UI used the quotes too, but it would be a valuable and simple improvement anyway.

There are some UI glitches that I'll describe in a separate Bugzilla comment, because I want to attach a screenshot.

::: devtools/client/netmonitor/actions/filters.js:10
(Diff revision 8)
>  "use strict";
>  
>  const {
>    TOGGLE_REQUEST_FILTER_TYPE,
>    ENABLE_REQUEST_FILTER_TYPE_ONLY,
> +  SET_HEADERS_FILTER_TEXT,

Not needed anymore.

::: devtools/client/netmonitor/actions/filters.js:48
(Diff revision 8)
>  /**
> + * Set filter text in headers panel.
> + *
> + * @param {string} text - A filter text is going to be set
> + */
> +function setHeadersFilterText(text) {

Not needed anymore.

::: devtools/client/netmonitor/constants.js:10
(Diff revision 8)
>  "use strict";
>  
>  const general = {
>    CONTENT_SIZE_DECIMALS: 2,
>    FILTER_SEARCH_DELAY: 200,
> +  HEADERS_SIZE_DECIMALS: 3,

Please coordinate with Steve's work in bug 1317646 (common functions for time/size formatting). After Steve's and your patches land, all code should be using the common functions.

By the way, I think it would be better to display headers size as "645 B" instead of "0.645 KB". That's a change compared to previous XUL UI, but I think it'd be a simple and nice improvement.

::: devtools/client/netmonitor/constants.js:28
(Diff revision 8)
>    OPEN_SIDEBAR: "OPEN_SIDEBAR",
>    OPEN_STATISTICS: "OPEN_STATISTICS",
>    PRESELECT_REQUEST: "PRESELECT_REQUEST",
>    REMOVE_SELECTED_CUSTOM_REQUEST: "REMOVE_SELECTED_CUSTOM_REQUEST",
>    SELECT_REQUEST: "SELECT_REQUEST",
> +  SET_HEADERS_FILTER_TEXT: "SET_HEADERS_FILTER_TEXT",

This is no longer needed - all filtering is managed in PropertiesView internal state and doesn't need to go through Redux.

::: devtools/client/netmonitor/reducers/filters.js:11
(Diff revision 8)
>  
>  const I = require("devtools/client/shared/vendor/immutable");
>  const {
>    TOGGLE_REQUEST_FILTER_TYPE,
>    ENABLE_REQUEST_FILTER_TYPE_ONLY,
> +  SET_HEADERS_FILTER_TEXT,

Not neede anymore (this whole file doesn't need to change at all)

::: devtools/client/netmonitor/requests-menu-view.js:245
(Diff revision 8)
> +    if (requestHeaders && requestHeaders.headers &&
> +        requestHeaders.headers.length > 0) {
> +      for (let { value } of requestHeaders.headers) {
> +        requestHeaders.headers.value = yield gNetwork.getString(value);
> +      }
> +      yield this.store.dispatch(Actions.updateRequest(
> +        action.id,
> +        { requestHeaders },
> +        true,
> +      ));
> +    }

Extract this code to a separate function and then call it twice:

yield fetchHeaders(requestHeaders, "requestHeaders")
yield fetchHeaders(responseHeaders, "responseHeaders")

The whole updateRequest method will be eventually refactored to:
- be a part of an action creator
- fetch the detailed data lazily, only after a details tab is selected

::: devtools/client/netmonitor/shared/components/headers-panel.js:96
(Diff revision 8)
> +    if (responseHeaders && responseHeaders.headers.length > 0) {
> +      let section = `${RESPONSE_HEADERS} (${this.getSection(responseHeaders)})`;
> +      object[section] =
> +        responseHeaders.headers
> +          .reduce((acc, { name, value }) =>
> +            name ? Object.assign(acc, { [name]: value }) : acc
> +          , {});
> +      sectionNames.push(section);
> +    }

Extract a method and call it 3 times.

::: devtools/client/netmonitor/shared/components/headers-panel.js:110
(Diff revision 8)
> +          .reduce((acc, { name, value }) =>
> +            name ? Object.assign(acc, { [name]: value }) : acc
> +          , {});

There was an "arrayToDict" helper function in one of the patches, wasn't there? It should be used here.

::: devtools/client/netmonitor/shared/components/headers-panel.js:147
(Diff revision 8)
> +        div({ className: "tabpanel-summary-container headers-summary" },
> +          div({ className: "tabpanel-summary-label headers-summary-label" }, SUMMARY_URL),
> +          input({
> +            className: "tabpanel-summary-value textbox-input devtools-monospace",
> +            readOnly: true,
> +            value: decodeUnicodeUrl(url),

Every request record has a "urlDetails.unicodeUrl" where the decoded URL already is. Some strings derived from the URL are used at many places, so they're are cached. Avoids parsing/decoding the URL on every render many times.

::: devtools/client/netmonitor/shared/components/headers-panel.js:156
(Diff revision 8)
> +        div({ className: "tabpanel-summary-container headers-summary" },
> +          div({
> +            className: "tabpanel-summary-label headers-summary-label",
> +          }, SUMMARY_METHOD),
> +          input({
> +            className: "tabpanel-summary-value textbox-input devtools-monospace",
> +            readOnly: true,
> +            value: method,
> +          }),
> +        )

This code that renders a summary row is repeated many times, making the render() function very long and verbose. Extract the code to a method and call it several times.

::: devtools/client/netmonitor/shared/components/headers-panel.js:248
(Diff revision 8)
> +            textarea({
> +              value: writeHeaderText(requestHeaders.headers),
> +              readOnly: true,
> +            }),

Unlike the XUL version, this textarea can be resized (it has the resize handle in the corner). And when you actually perform the resize, it's not very nice to look at - the UI layout breaks.

::: devtools/client/netmonitor/shared/components/headers-panel.js:296
(Diff revision 8)
> +        requestHeadersFromUploadStream,
> +        responseHeaders,
> +      } = selectedRequest;
> +
> +      return {
> +        headersFilterText: state.filters.headersFilterText,

headersFilterText is not needed anymore.

Shouldn't this mapStateToProps function return just one prop, with the request record to display? I don't see a reason to extract requestHeaders and other fields when you're passing the selectedRequest anyway.

::: devtools/client/netmonitor/shared/components/headers-panel.js:307
(Diff revision 8)
> +    setHeadersFilterText: (text) =>
> +      dispatch(Actions.setHeadersFilterText(text)),

Not needed anymore.
Created attachment 8825727 [details]
headers-ui-compare.png

Attaching screenshot that describes several regressions from the XUL UI:

The colors of the "Edit and Resend" and "Raw headers" buttons have changed. Is that intentional? Looks like a CSS bug.

The styling of the raw headers textarea has regressed in several ways:
- the border should be a light gray color, subdued and consistent with other vertical and horizontal lines in the surrounding UI
- the textarea should have a bigger padding - so that the text isn't so close to the border lines
- the margins on the left and right edge should be the same. Now, the right margin is much smaller
- the textareas shouldn't be resizable. Now, when I resize one of them, the UI layout gets really ugly
Nice progress here, a few UI related comments.

(In reply to Jarda Snajdr [:jsnajdr] from comment #13)
> Comment on attachment 8824348 [details]
> - it would be great if the header values weren't displayed in quotes.
While I like the idea I think this is one of the UI changes we shouldn't do ad hoc and focus on refactoring (just like we discussed a few times before). If we do this, we should be consistent and find other place in the UI that need such change (e.g. we should probably do it for Cookie values too). I'll put this into UI suggestions list.

> By the way, I think it would be better to display headers size as "645 B"
Agreed, this should be consistent with the 'Transferred' and 'Size' columns.

(In reply to Jarda Snajdr [:jsnajdr] from comment #14)
> The styling of the raw headers textarea has regressed in several ways:
> - the border should be a light gray color, subdued and consistent with other
> vertical and horizontal lines in the surrounding UI
> - the textarea should have a bigger padding - so that the text isn't so
> close to the border lines
> - the margins on the left and right edge should be the same. Now, the right
> margin is much smaller
> - the textareas shouldn't be resizable. Now, when I resize one of them, the
> UI layout gets really ugly
I've put the comment into the list of suggestions, thanks Jarda! But, please let's try to keep the UI as it was. When we want to change any e.g. colors we need to make sure it's ok across all our themes (light, dark, Firebug).

Btw. I've filled a new bug for unifying colors for side panels (bug 1330300). 

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> I've put the comment into the list of suggestions, thanks Jarda! But, please
> let's try to keep the UI as it was. When we want to change any e.g. colors
> we need to make sure it's ok across all our themes (light, dark, Firebug).

All the raw headers styling issues I'm reporting are regressions - they are correct in the old XUL version and are broken in the React version.

I usually try to point out all issues I see and consider important. I think it's better than keeping them to myself. It's the patch author's responsibility to decide which issues to fix immediately, as followups, or never.
(In reply to Jarda Snajdr [:jsnajdr] from comment #16)
> All the raw headers styling issues I'm reporting are regressions - they are
> correct in the old XUL version and are broken in the React version.
Agreed, regressions should be fixed. I just wanted to point out that new UI features should be done separately.

Honza
Comment hidden (mozreview-request)
Updated my patch for fixing all mozreview issues as well as UI regressions on comment 14.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
mozreview-review
Comment on attachment 8824348 [details]
Bug 1317648 - Implement Headers Panel

https://reviewboard.mozilla.org/r/102878/#review104886

This patch is great, thanks for working on this! It's exciting to see the last refactored tab landing :)

::: devtools/client/framework/test/shared-head.js
(Diff revision 12)
> -
> -  return deferred.promise;
> +return deferred.promise;

Accidental whitespace change here?

::: devtools/client/netmonitor/details-view.js:19
(Diff revision 12)
>  const {
> -  decodeUnicodeUrl,
>  } = require("./request-utils");

Empty require?

::: devtools/client/netmonitor/reducers/filters.js:30
(Diff revision 12)
>    other: false,
>  });
>  
>  const Filters = I.Record({
>    requestFilterTypes: new FilterTypes({ all: true }),
> +  headersFilterText: "",

This field is not needed anymore.
Attachment #8824348 - Flags: review?(jsnajdr) → review+

Comment 23

a year ago
mozreview-review
Comment on attachment 8824348 [details]
Bug 1317648 - Implement Headers Panel

https://reviewboard.mozilla.org/r/102878/#review104890

The only remaining UI issue I am seeing is realated to the vertical scrollbar. The bottom-scroll-button isn't visible. See the attached screenshot.
Attachment #8824348 - Flags: review?(odvarko)
Created attachment 8826160 [details]
scrollbar-button.png
Comment hidden (mozreview-request)
Note that the problem is even more visible if the "Raw headers" are visible. It's also nicely visible on OSX in this case.

Honza

Comment 27

a year ago
mozreview-review
Comment on attachment 8824348 [details]
Bug 1317648 - Implement Headers Panel

https://reviewboard.mozilla.org/r/102878/#review104904

After offline discussion with Ricky we decided to fix the scroll bar issue as part of bug 1329068 (it should be easier to fix it when xul is removed entirely).

Thanks for the patch Ricky!

Honza
Attachment #8824348 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)

Comment 29

a year ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5fed6b5ba72
Implement Headers Panel r=Honza,jsnajdr

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5fed6b5ba72
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I've tested the Header Panel on latest Aurora 53.0a2 (2017-02-02) and the patch looks good. One question, comment 19 suggest that all UI regression and mozreview issues (mentioned in comment 14) were fixed. 

(Jarda Snajdr [:jsnajdr] from comment #14)
> - the textarea should have a bigger padding - so that the text isn't so close to the border lines

For me, it's still to close to the border lines as you can see here: http://imgur.com/a/7Mgyn.
Ricky, what do you think about this layout issue?
After offline discussion, we all agreed that this is a minor UI impact and should not be a layout issue.
OK. Thank you for clarification.
I will mark this bug as verified fixed based on comment 31.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.