Bug 177896 - Web Inspector: Network Tab - Headers Detail View
Summary: Web Inspector: Network Tab - Headers Detail View
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 156636 157751 (view as bug list)
Depends on:
Blocks: 177981 177988
  Show dependency treegraph
 
Reported: 2017-10-04 13:54 PDT by Joseph Pecoraro
Modified: 2017-10-06 15:48 PDT (History)
9 users (show)

See Also:


Attachments
[IMAGE] Headers - http/1.1 (233.63 KB, image/png)
2017-10-04 13:55 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - h2 (226.38 KB, image/png)
2017-10-04 13:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - request data (236.38 KB, image/png)
2017-10-04 13:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - searchable (217.59 KB, image/png)
2017-10-04 13:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - still loading (174.43 KB, image/png)
2017-10-04 13:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - done loading (225.74 KB, image/png)
2017-10-04 13:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Cookies - both request and response (306.63 KB, image/png)
2017-10-04 13:59 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - http/1.1 (221.55 KB, image/png)
2017-10-05 16:51 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - h2 (also showing wrapping) (250.25 KB, image/png)
2017-10-05 16:51 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - error (215.03 KB, image/png)
2017-10-05 16:51 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - search (a follow-up patch) (223.42 KB, image/png)
2017-10-05 16:51 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - loading (123.70 KB, image/png)
2017-10-05 16:52 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Headers - done loading (166.40 KB, image/png)
2017-10-05 16:52 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (65.99 KB, patch)
2017-10-05 17:17 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (65.89 KB, patch)
2017-10-05 18:54 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (65.90 KB, patch)
2017-10-05 20:10 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (66.22 KB, patch)
2017-10-06 11:18 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-10-04 13:54:40 PDT
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
Comment 1 Joseph Pecoraro 2017-10-04 13:55:13 PDT
<rdar://problem/34071924>
Comment 2 Joseph Pecoraro 2017-10-04 13:55:51 PDT
Created attachment 322716 [details]
[IMAGE] Headers - http/1.1
Comment 3 Joseph Pecoraro 2017-10-04 13:56:03 PDT
Created attachment 322717 [details]
[IMAGE] Headers - h2
Comment 4 Joseph Pecoraro 2017-10-04 13:56:18 PDT
Created attachment 322719 [details]
[IMAGE] Headers - request data
Comment 5 Joseph Pecoraro 2017-10-04 13:56:32 PDT
Created attachment 322720 [details]
[IMAGE] Headers - searchable
Comment 6 Joseph Pecoraro 2017-10-04 13:56:45 PDT
Created attachment 322721 [details]
[IMAGE] Headers - still loading
Comment 7 Joseph Pecoraro 2017-10-04 13:56:58 PDT
Created attachment 322722 [details]
[IMAGE] Headers - done loading
Comment 8 Joseph Pecoraro 2017-10-04 13:59:03 PDT
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.
Comment 9 Joseph Pecoraro 2017-10-05 16:50:30 PDT
I got a round of feedback and help from some UI folks. Round 2 UI going up.
Comment 10 Joseph Pecoraro 2017-10-05 16:51:09 PDT
Created attachment 322936 [details]
[IMAGE] Headers - http/1.1
Comment 11 Joseph Pecoraro 2017-10-05 16:51:26 PDT
Created attachment 322937 [details]
[IMAGE] Headers - h2 (also showing wrapping)
Comment 12 Joseph Pecoraro 2017-10-05 16:51:39 PDT
Created attachment 322938 [details]
[IMAGE] Headers - error
Comment 13 Joseph Pecoraro 2017-10-05 16:51:59 PDT
Created attachment 322939 [details]
[IMAGE] Headers - search (a follow-up patch)
Comment 14 Joseph Pecoraro 2017-10-05 16:52:14 PDT
Created attachment 322940 [details]
[IMAGE] Headers - loading
Comment 15 Joseph Pecoraro 2017-10-05 16:52:26 PDT
Created attachment 322941 [details]
[IMAGE] Headers - done loading
Comment 16 Joseph Pecoraro 2017-10-05 17:17:18 PDT
Created attachment 322942 [details]
[PATCH] Proposed Fix
Comment 17 Build Bot 2017-10-05 17:19:51 PDT
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.
Comment 18 Timothy Hatcher 2017-10-05 17:55:08 PDT
Cool!
Comment 19 Joseph Pecoraro 2017-10-05 18:54:47 PDT
Created attachment 322962 [details]
[PATCH] Proposed Fix
Comment 20 Joseph Pecoraro 2017-10-05 20:10:35 PDT
Created attachment 322973 [details]
[PATCH] Proposed Fix
Comment 21 Devin Rousso 2017-10-06 00:37:02 PDT
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?
Comment 22 Sam Weinig 2017-10-06 03:19:18 PDT
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?
Comment 23 Joseph Pecoraro 2017-10-06 09:34:51 PDT
(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.
Comment 24 Joseph Pecoraro 2017-10-06 11:15:29 PDT
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).
Comment 25 Joseph Pecoraro 2017-10-06 11:18:53 PDT
Created attachment 323030 [details]
[PATCH] Proposed Fix
Comment 26 Sam Weinig 2017-10-06 14:03:53 PDT
(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.
Comment 27 Joseph Pecoraro 2017-10-06 14:06:19 PDT
*** Bug 157751 has been marked as a duplicate of this bug. ***
Comment 28 Joseph Pecoraro 2017-10-06 15:07:35 PDT
*** Bug 156636 has been marked as a duplicate of this bug. ***
Comment 29 Devin Rousso 2017-10-06 15:10:29 PDT
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.
Comment 30 Joseph Pecoraro 2017-10-06 15:36:51 PDT
> > 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.
Comment 31 Joseph Pecoraro 2017-10-06 15:39:17 PDT
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.
Comment 32 Joseph Pecoraro 2017-10-06 15:48:59 PDT
<https://trac.webkit.org/r223006>