Bug 16531 - Web Inspector: Show all headers in the Request Headers section of the Resource details sidebar
Summary: Web Inspector: Show all headers in the Request Headers section of the Resourc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 48226 (view as bug list)
Depends on:
Blocks: 170525
  Show dependency treegraph
 
Reported: 2007-12-20 08:27 PST by Danny Bloemendaal
Modified: 2017-05-02 03:39 PDT (History)
20 users (show)

See Also:


Attachments
patch (25.81 KB, patch)
2010-10-25 11:21 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
[IMAGE] Before - Missing Cookie Header (99.01 KB, image/png)
2017-04-04 20:12 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After - Include Cookie, Accept-Language, Accept-Encoding (118.94 KB, image/png)
2017-04-04 20:12 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (23.19 KB, patch)
2017-04-04 20:24 PDT, Joseph Pecoraro
timothy: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (865.89 KB, application/zip)
2017-04-04 21:37 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Danny Bloemendaal 2007-12-20 08:27:50 PST
I'm missing the cookie data that's being sent to the server in the request information when you click on an item in the Network section inside web inspector. (I have to debug a problem there so that's why I am missing that information).
Comment 1 Adam Roben (:aroben) 2008-01-29 11:09:44 PST
<rdar://problem/5712895>
Comment 2 Brian Weinstein 2009-11-20 16:58:46 PST
Now that we have a Cookie storage view, is this enough to have here, or should there be more?
Comment 3 Timothy Hatcher 2009-11-20 17:06:28 PST
I think showing them with resources might be nice, but can be confusing too.

We should only show the cookies that were sent to / set by the resource.
Comment 4 Brian Weinstein 2009-11-20 17:08:10 PST
Do we have that data from CFNetwork? I don't think we do, we can get all the cookies that the resource has access to, but not the ones just set by that resource.
Comment 5 Timothy Hatcher 2009-11-20 17:10:13 PST
We should have the Cookies header that they send back.
Comment 6 Tauren Mills 2010-05-17 22:34:33 PDT
(In reply to comment #2)
> Now that we have a Cookie storage view, is this enough to have here, or should there be more?

I was told that my post on the Chromium group relates to this issue.
http://groups.google.com/a/chromium.org/group/chromium-discuss/browse_thread/thread/c44361ebe6e03184#

Personally, I think that showing cookie information in the Resource tab is very important, and that it shouldn't only be available in the Cookie storage view. I'm not familiar with WebKit besides using Chrome, but I'm assuming this is referring to the "Cookies and Other Data" dialog.

I don't understand why cookies would not be included with the Request Headers and Response Headers in Developer Tools so that developers can get a very clear picture of everything related to a request. When debugging a cookie related problem, it is extremely cumbersome to have to go into a separate location to view cookie details. Having an indicator showing when a new cookie is set would be nice (perhaps bold it or something).

Also, the separate cookie storage view simply shows the state of cookies at a certain point in time - when you look at it. It doesn't give you an easy way to see which request received a cookie, or which ones sent a cookie. For instance, if there are a series of ajax requests, one of which receives a cookie, and the following ones send the cookie, it would be hard to determine which ones used cookies and which didn't.
Comment 7 Andrey Kosyakov 2010-10-25 10:18:23 PDT
*** Bug 48226 has been marked as a duplicate of this bug. ***
Comment 8 Andrey Kosyakov 2010-10-25 11:21:02 PDT
Created attachment 71772 [details]
patch

See https://bugs.webkit.org/show_bug.cgi?id=48226 for previous discussion of the patch.

> > WebCore/WebCore.gypi:4424
> > +            'inspector/front-end/ResourceCookiesView.js',
> 
> This is a tab, not a view.

Renamed to ResourceCookiesTab and moved it to ResourceView.js

> > WebCore/inspector/front-end/CookieItemsViewBase.js:33
> > +WebInspector.CookieItemsViewBase = function()
> 
> This could simply reside in CookieItemsView in form of helper methods / component.

Moving this to CookieItemsView.js, but leaving everything else intact (we do use a bit of polymorphism there).

> > WebCore/inspector/front-end/CookieParser.js:57
> > +                this._addCookie(kv, "request");
> 
> Could you use constants for these?

Done.

> > WebCore/inspector/front-end/DataGrid.js:320
> > +        var children = maxDescentLevel ? this._enumerateChildren(this, [], maxDescentLevel) : this.children;
> 
> So if I pass "1" as maxDescentLevel, the code will work as if it was "1".

Quite so, and similar statements hold true for all other possible values of maxDescentLevel ;-)
Changed the base for the level so that 0 is the old behavior (top level only) and 1 is one level below it, etc.

> > WebCore/inspector/front-end/DataGrid.js:404
> > +        if (!this._columnWidthsInitialized && this.element.enclosingNodeOrSelfWithNodeName("body")) {
> 
> You could also check this.element.offsetWidth for === 0.

Done.
Comment 9 Pavel Feldman 2010-10-26 02:37:41 PDT
Comment on attachment 71772 [details]
patch

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

> WebCore/inspector/front-end/CookieItemsView.js:30
> +WebInspector.CookieItemsViewBase = function()

Can we call this CookiesTable?
Comment 10 Andrey Kosyakov 2010-10-26 09:57:56 PDT
Manually committed r70529: http://trac.webkit.org/changeset/70529

Note this functionality depends on platform's ability to deliver Cookie/Set-Cookie headers from the network stack, which is not yet available on many platforms yet -- so this patch won't have immediate effect on these platforms. Chromium, for one, has this working.
Comment 11 Eric Seidel (no email) 2010-12-14 01:50:27 PST
Seems this is fixed.  Please consider using webkit-patch land, as it will close bugs for you when landing.
Comment 12 Andrey Kosyakov 2010-12-14 05:12:08 PST
I left it open on purpose (see comment #10), as this was only fixed for Chromium -- this functionality depends on the network stack supplying actual request headers, which is only implemented in case of Chromium presently. Hence, the cookies tab is currently enabled conditionally (off by default). I thought it wouldn't be appropriate to close this bug until it gets fixed in other browsers -- Safari in particular, given Radar references above.
Comment 13 Eric Seidel (no email) 2010-12-14 15:15:46 PST
Attachment 71772 [details] was posted by a committer and has review+, assigning to Andrey Kosyakov for commit.
Comment 14 Eric Seidel (no email) 2010-12-20 22:59:48 PST
Comment on attachment 71772 [details]
patch

Marking obsolete since this was landed.
Comment 15 Rob Colburn 2012-05-09 09:24:20 PDT
This is ticket is marked REOPENED, but should be marked CLOSED/FIXED.
See:
Comment #14 From Eric Seidel 2010-12-20 22:59:48 PST
Comment 16 Joseph Pecoraro 2017-04-04 20:08:13 PDT
This information is now available to WebKit on macOS via CFNetwork's public didFinishCollectingMetrics delegate message.

WebKit has access to a complete NSURLRequest which includes all of the request headers that were sent, including Cookie headers, Authorization headers (Basic Auth), and others (Host, Accept*) which may be added underneath WebKit.
Comment 17 Joseph Pecoraro 2017-04-04 20:12:02 PDT
Created attachment 306241 [details]
[IMAGE] Before - Missing Cookie Header
Comment 18 Joseph Pecoraro 2017-04-04 20:12:37 PDT
Created attachment 306242 [details]
[IMAGE] After - Include Cookie, Accept-Language, Accept-Encoding
Comment 19 Joseph Pecoraro 2017-04-04 20:24:59 PDT
Created attachment 306244 [details]
[PATCH] Proposed Fix

Some things I have to test:

1. Is it possible to send a request with duplicate header? if so I need a test for that
2. I have concerns about potential performance issues:
  - this makes a HTTPHeaderMap for every request and serializes between the NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
  - I suspect this will be normally be small but it could be large and if so I want to make sure we don't negatively impact load performance
Comment 20 Timothy Hatcher 2017-04-04 20:28:10 PDT
Comment on attachment 306244 [details]
[PATCH] Proposed Fix

Awesome!
Comment 21 youenn fablet 2017-04-04 20:33:41 PDT
(In reply to Joseph Pecoraro from comment #19)
> Created attachment 306244 [details]
> [PATCH] Proposed Fix
> 
> Some things I have to test:
> 
> 1. Is it possible to send a request with duplicate header? if so I need a
> test for that

Except for cookie headers, duplicated headers should be combined.

> 2. I have concerns about potential performance issues:
>   - this makes a HTTPHeaderMap for every request and serializes between the
> NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
>   - I suspect this will be normally be small but it could be large and if so
> I want to make sure we don't negatively impact load performance

This will happen in all cases or just when web inspector is set?
Comment 22 Build Bot 2017-04-04 21:37:55 PDT
Comment on attachment 306244 [details]
[PATCH] Proposed Fix

Attachment 306244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3475445

New failing tests:
http/tests/inspector/network/resource-request-headers.html
Comment 23 Build Bot 2017-04-04 21:37:57 PDT
Created attachment 306249 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 24 Joseph Pecoraro 2017-04-05 10:46:06 PDT
Oh, el-capitan doesn't use the NSURLSession path, so I'll have to mark this as failing there like I did for the WK1 ports.
Comment 25 Joseph Pecoraro 2017-04-05 16:21:37 PDT
(In reply to youenn fablet from comment #21)
> (In reply to Joseph Pecoraro from comment #19)
> > Created attachment 306244 [details]
> > [PATCH] Proposed Fix
> > 
> > Some things I have to test:
> > 
> > 1. Is it possible to send a request with duplicate header? if so I need a
> > test for that
> 
> Except for cookie headers, duplicated headers should be combined.

Yeah, I don't see a way for web developers to ever create a case of duplicate request headers (XHR and Fetch both have set/append so only 1 key). I may have missed a case where the browser might do it, but in that case I concluded it would be enough of an edge case that I can worry about it later.


> > 2. I have concerns about potential performance issues:
> >   - this makes a HTTPHeaderMap for every request and serializes between the
> > NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
> >   - I suspect this will be normally be small but it could be large and if so
> > I want to make sure we don't negatively impact load performance
> 
> This will happen in all cases or just when web inspector is set?

Bug 170525 convert only doing this when Web Inspector is open. If that gets reviewed positively I'll land this confidently (I don't want to impact performance of general browsing).
Comment 26 Joseph Pecoraro 2017-04-06 15:07:42 PDT
<https://trac.webkit.org/changeset/215062/webkit>