Bug 177896 – Web Inspector: Network Tab - Headers Detail View

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Web Inspector: Network Tab - Headers Detail View

Start filling out the different Detail Views in the New Network Tab.

The Headers Detail view should cover general metadata about the HTTP Request / Response. Including:

    - General HTTP Information
    - Request Headers
    - Response Headers
    - Request Data / Query String Data
    - Redirect information

[Joseph Pecoraro]

<rdar://problem/34071924>

[Joseph Pecoraro]

Created attachment 322716 [details]
[IMAGE] Headers - http/1.1

[Joseph Pecoraro]

Created attachment 322717 [details]
[IMAGE] Headers - h2

[Joseph Pecoraro]

Created attachment 322719 [details]
[IMAGE] Headers - request data

[Joseph Pecoraro]

Created attachment 322720 [details]
[IMAGE] Headers - searchable

[Joseph Pecoraro]

Created attachment 322721 [details]
[IMAGE] Headers - still loading

[Joseph Pecoraro]

Created attachment 322722 [details]
[IMAGE] Headers - done loading

[Joseph Pecoraro]

Created attachment 322723 [details]
[IMAGE] Cookies - both request and response

I'm adding a preliminary Cookies Detail view screenshot here because this is the first time we are coming up with the Design language of these Detail views.

Here the similarity is a bunch of Sections, with a title, not collapsible, with contents slightly inset. I'm happy to iterate on the design here. These were the only two detail views I worked through so far so and so I shared their basic styles.

[Joseph Pecoraro]

I got a round of feedback and help from some UI folks. Round 2 UI going up.

[Joseph Pecoraro]

Created attachment 322936 [details]
[IMAGE] Headers - http/1.1

[Joseph Pecoraro]

Created attachment 322937 [details]
[IMAGE] Headers - h2 (also showing wrapping)

[Joseph Pecoraro]

Created attachment 322938 [details]
[IMAGE] Headers - error

[Joseph Pecoraro]

Created attachment 322939 [details]
[IMAGE] Headers - search (a follow-up patch)

[Joseph Pecoraro]

Created attachment 322940 [details]
[IMAGE] Headers - loading

[Joseph Pecoraro]

Created attachment 322941 [details]
[IMAGE] Headers - done loading

[Joseph Pecoraro]

Created attachment 322942 [details]
[PATCH] Proposed Fix

[Build Bot]

Attachment 322942 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:82:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Cool!

[Joseph Pecoraro]

Created attachment 322962 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 322973 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 322973 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=322973&action=review

This looks awesome (both code-wise and visually)!  I am in the middle of a build so I can't test it now, but I'll download it tomorrow and give it another look :)

> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:178
>  FAIL: scheme should be: 'http'

Sucks that we have all these FAILs :(

> Source/WebInspectorUI/ChangeLog:31
> +        New event whenever metrics change. This is the first even that will allow

Typo: first even that -> first event that

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:282
> +}

NIT: missing ;

> Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:300
> +}

Ditto (282).

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:82
> +    padding: 0 20px 20px 20px;

Style: I'm not a fan of adding extra newlines to CSS, but we have done it in the past, so I'm not sure what the ruling is on that.  Also, you can shorthand this:

    padding: 0 20px 20px;

> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:35
> +    margin-left: 10px;

-webkit-margin-start: 10px;

> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:41
> +    padding: 2px 0 2px 7px;

To provide RTL support, you might want to split this into two properties.

    padding: 2px 0;
    -webkit-padding-start: 7px;

> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:45
> +    color: var(--console-secondary-text-color) !important;

Is the "!important" absolutely necessary?

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:27
> +    border-left-color: var(--network-system-color);

Do you want to add RTL support for this?

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:31
> +    border-left-color: var(--network-header-color);

Ditto (27).

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:35
> +    border-left-color: var(--network-error-color);

Ditto (27).

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:49
> +    margin-left: var(--resource-headers-value-indent);

-webkit-margin-start: var(--resource-headers-value-indent);

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:55
> +    margin-left: calc(var(--resource-headers-value-indent) * -1);

-webkit-margin-start: calc(var(--resource-headers-value-indent) * -1);

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:30
> +        super(null);

NIT: add a const variable.

    const representedObject = null;
    super(representedObject);

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:50
> +    initialLayout()

Add call to `super.initialLayout();`.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:79
> +    layout()

Add call to `super.layout();`

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:117
> +        let displayKey = key + (!!value ? ": " : "");

I think you can drop the `!!` since its value isn't actually needed.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:160
> +        let source = this._responseSourceDisplayString(this._resource.responseSource) || emDash;

NIT: instead of having the `emDash` here, would it be better to have it as the `default` return value above (142) so that if other callers are added they don't need to do the same?

[Sam Weinig]

Purely feedback based on the images. 

What does the 'colon header' syntax (e.g. :status) as seen in the 'Headers - h2' image indicate?
Why is Query String in the Headers pane?
What will a resource with multiple redirect responses look like?

[Joseph Pecoraro]

(In reply to Sam Weinig from comment #22)
> What does the 'colon header' syntax (e.g. :status) as seen in the 'Headers - h2' image indicate?

These are what HTTP/2 calls "pseudo-headers" implicit in every request/response. The spec denotes them as :name.
https://http2.github.io/http2-spec/#rfc.section.8.1.2.3

I'm synthesizing these above the headers because I think it gives a more complete picture of the request/response and allows for an easy way to distinguish use of http/2 compared to http/1. We don't need to display them as :name, we could instead just color them differently, maybe use italics, or something else. I included the colon in the name since it may make it easier for developers who don't recognize them to search and discover what they are.

Some browser tools include these and some do not. Chrome does, Firefox and Edge do not.

> Why is Query String in the Headers pane?

I think this question is mostly why is the pane titled "Headers" and includes non-header things? This is something I've struggled with as well (Not just for this pane but the others too). Suggestions are welcome.

Broadly, each major browser has a section named "Headers" that includes at least a Summary and Request/Response headers. We've matched that name in the interest of familiarity. Chrome also includes this extra contextual data (query string + request data) in their Headers section. Firefox and Edge both have a "Params" section for query string parameters, and even more sections for data.

I think the most appropriate name would probably be "Transcript". I want this section to include all of the back and forth metadata sent for this load. In the vast majority of cases this ends up being just the headers, but in some cases there is extra data, like Request Data. In those cases, including that data inside this existing pane avoids having lots of small purpose panes, which would eventually just mean more clicking for users.

I see the query string parameters as just another form of Request Data. I expect this contextual data to only occasionally be useful, which is why it is pushed down to the bottom, with the primary focus of the section being the headers themselves. Because query strings can be inconvenient to parse, including a section that breaks out the components should be nice for the developers that make use of it.


> What will a resource with multiple redirect responses look like?

Glad someone is paying attention! In the interesting of keeping this section a chronological timeline my plan was to just lay it all out in order (Request -> Redirect -> Response):

So ultimately something like:

    Summary:
       URL: ...
       Status: ...
       Preflight: (→)

    Request:
       POST /index.html HTTP/1.1
       Header: ...
       Header: ...
       Header: ...

    Request Data:
       MIME Type: ...
       Boundary: ...
       Data: (→)

    Redirect 1:
       HTTP/1.1 30x ...
       Location: ...
       
       POST /new-index.html ...
       New Header: ...
       New Header: ...
       New Header: ...
       
    Redirect 2:
       HTTP/1.1 30x ...
       Location: ...
       
       GET /even-newer-index.html ...
       (unchanged)

    Response:
        HTTP/1/1 200 ...
        Header: ...
        Header: ...       
        Header: ...

    Query String:
        a: b
        c: d

Notes:
- A single redirect wouldn't need #s.
- A redirect that ends up sending new / different headers should display all headers
- A redirect that has identical headers would show the new request line and a literal string like "unchanged" with some style.
- Query string is at the bottom because it can be a huge list. But maybe it should go up near the top / Request Data.
- Order is intended such that scrolling down you go through time. Other browsers include "Response" at the top, which I found confusing.
- If this load was preceded by a preflight request, provide an association link in the Summary. The Preflight will have its entire own request/response transcript.

Open Questions:
- Should we show the entire URL sequence (at least original and final) in the Summary section? Maybe.

Redirects are the only explicit FIXME comment in this patch, because Web Inspector currently discards redirect data and doesn't show it at all right now <https://webkit.org/b/150005>. Adding support for redirects will therefore be a larger task that I'll end up tackling. In the interest of achieving parity so that we can switch to the new network tab I left this for later.

[Joseph Pecoraro]

Comment on attachment 322973 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=322973&action=review

Thanks for the comments. New patch in a sec.

>> LayoutTests/inspector/unit-tests/url-utilities-expected.txt:178
>>  FAIL: scheme should be: 'http'
> 
> Sucks that we have all these FAILs :(

Yep. One day we should move from `parseURL()` to `new URL(...)`.

>> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:41
>> +    padding: 2px 0 2px 7px;
> 
> To provide RTL support, you might want to split this into two properties.
> 
>     padding: 2px 0;
>     -webkit-padding-start: 7px;

Ahh, this is the first task where I forgot to do RTL testing! Thanks for the pointers, I'll fix up RTL support.

>> Source/WebInspectorUI/UserInterface/Views/ResourceDetailsSection.css:45
>> +    color: var(--console-secondary-text-color) !important;
> 
> Is the "!important" absolutely necessary?

Nope, but I want it. Anyone who has marked a section as incomplete should expect it.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:30
>> +        super(null);
> 
> NIT: add a const variable.
> 
>     const representedObject = null;
>     super(representedObject);

I don't think we want to do this everywhere. Here it would just be noise.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:50
>> +    initialLayout()
> 
> Add call to `super.initialLayout();`.

I actually haven't seen other places doing this for initialLayout or layout. I was hoping that would be the plan of record for View layout code. Otherwise we'd have ~4 wasted lines in every view that ultimately does nothing.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:160
>> +        let source = this._responseSourceDisplayString(this._resource.responseSource) || emDash;
> 
> NIT: instead of having the `emDash` here, would it be better to have it as the `default` return value above (142) so that if other callers are added they don't need to do the same?

I'm going to keep this as is. I'm using emDash here only because I want the Summary Section to not be variable height (so always have the same number of rows). Other users may decide to ignore an empty value and should check against empty / null and not emDash (or check the responseSource directly).

[Joseph Pecoraro]

Created attachment 323030 [details]
[PATCH] Proposed Fix

[Sam Weinig]

(In reply to Joseph Pecoraro from comment #23)
> (In reply to Sam Weinig from comment #22)
> > What does the 'colon header' syntax (e.g. :status) as seen in the 'Headers - h2' image indicate?
> 
> These are what HTTP/2 calls "pseudo-headers" implicit in every
> request/response. The spec denotes them as :name.
> https://http2.github.io/http2-spec/#rfc.section.8.1.2.3
> 
> I'm synthesizing these above the headers because I think it gives a more
> complete picture of the request/response and allows for an easy way to
> distinguish use of http/2 compared to http/1. We don't need to display them
> as :name, we could instead just color them differently, maybe use italics,
> or something else. I included the colon in the name since it may make it
> easier for developers who don't recognize them to search and discover what
> they are.
> 
> Some browser tools include these and some do not. Chrome does, Firefox and
> Edge do not.

Cool. I think you should keep them. 

> 
> > Why is Query String in the Headers pane?
> 
> I think this question is mostly why is the pane titled "Headers" and
> includes non-header things? This is something I've struggled with as well
> (Not just for this pane but the others too). Suggestions are welcome.
> 
> Broadly, each major browser has a section named "Headers" that includes at
> least a Summary and Request/Response headers. We've matched that name in the
> interest of familiarity. Chrome also includes this extra contextual data
> (query string + request data) in their Headers section. Firefox and Edge
> both have a "Params" section for query string parameters, and even more
> sections for data.
> 
> I think the most appropriate name would probably be "Transcript". I want
> this section to include all of the back and forth metadata sent for this
> load. In the vast majority of cases this ends up being just the headers, but
> in some cases there is extra data, like Request Data. In those cases,
> including that data inside this existing pane avoids having lots of small
> purpose panes, which would eventually just mean more clicking for users.
> 
> I see the query string parameters as just another form of Request Data. I
> expect this contextual data to only occasionally be useful, which is why it
> is pushed down to the bottom, with the primary focus of the section being
> the headers themselves. Because query strings can be inconvenient to parse,
> including a section that breaks out the components should be nice for the
> developers that make use of it.

Makes sense. Maybe 'Metadata'? It's a little weird that cookies have their own pane, as they are metadata as well.

[Joseph Pecoraro]

*** Bug 157751 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 156636 has been marked as a duplicate of this bug. ***

[Devin Rousso]

Comment on attachment 323030 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=323030&action=review

r=me

> Source/WebInspectorUI/ChangeLog:32
> +        New event whenever metrics change. This is the first event that will allow
> +        a client to react to a Protocol change.

What does this mean?  I think this can be reworded a bit for clarity.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:82
> +    padding: 0 20px 20px;

I tried messing with this a bit, and I think you could decrease the horizontal padding here to 10px and the vertical padding of `.resource-details > section` (ResourceDetailsSection.css), and you'll get a bit more space to work with.  Just a thought :)

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:26
> +body[dir] .resource-headers > section > .details {

I'm confused as to what the purpose of `body[dir]` is?  Is it necessary?

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:53
> +    /* Don't include indents in RTL */

I tried this locally, and it does seem really funky.  I wonder why it does this :/

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:37
> +        this._resource.addEventListener(WI.Resource.Event.MetricsDidChange, this._resourceMetricsDidChange, this);
> +        this._resource.addEventListener(WI.Resource.Event.RequestHeadersDidChange, this._resourceRequestHeadersDidChange, this);
> +        this._resource.addEventListener(WI.Resource.Event.ResponseReceived, this._resourceResponseReceived, this);

Can these event listeners be moved to initialLayout()?  It seems they all call `needsLayout()` anyway.

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:82
> > +    padding: 0 20px 20px;
> 
> I tried messing with this a bit, and I think you could decrease the
> horizontal padding here to 10px and the vertical padding of
> `.resource-details > section` (ResourceDetailsSection.css), and you'll get a
> bit more space to work with.  Just a thought :)

I suggest you do this in a follow-up. Otherwise I'll just be approximating your idea. A before/after screenshot would be great.

[Joseph Pecoraro]

Comment on attachment 323030 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=323030&action=review

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:26
>> +body[dir] .resource-headers > section > .details {
> 
> I'm confused as to what the purpose of `body[dir]` is?  Is it necessary?

Sorry yeah this was cryptic. It raises the specificity enough to meet and therefore override the `body[dir=...] .resource-details ...` equivalent selectors.

>> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:53
>> +    /* Don't include indents in RTL */
> 
> I tried this locally, and it does seem really funky.  I wonder why it does this :/

This is in part because the text in these do not include RTL text. I'll await feedback from QA folks here with more knowledge of RTL.

[Joseph Pecoraro]

<https://trac.webkit.org/r223006>