Support retrieval of raw headers from network stack

Report actual receive and response headers from network stack as part of resource response.
This requires special load flag on request and CanGetRawCookies permission on the requesting
renderer.

BUG=none
TEST=none

[caseq, 2010-08-16 14:33:10]

Can you have a first look before I send to others, please?

[pfeldman, 2010-08-16 15:06:09]

Overall looks good. Let us look at the webkit side of the change. We will then
be able to send this for review to the network guys.

http://codereview.chromium.org/3133016/diff/11001/12002
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/3133016/diff/11001/12002#newcode65
chrome/browser/net/chrome_net_log.h:65: }
missing blank line.

http://codereview.chromium.org/3133016/diff/11001/12004
File chrome/browser/net/raw_headers_observer.h (right):

http://codereview.chromium.org/3133016/diff/11001/12004#newcode18
chrome/browser/net/raw_headers_observer.h:18: class RawHeadersObserver : public
ChromeNetLog::Observer {
Rename this to DevToolsObserver and add it from DevToolsManager when needed?

http://codereview.chromium.org/3133016/diff/11001/12004#newcode20
chrome/browser/net/raw_headers_observer.h:20: typedef
ResourceLoaderBridge::RawHeadersInfo URLRequestRecord;
Rename to RawHeadersInfo? Also, I would use new struct here.

http://codereview.chromium.org/3133016/diff/11001/12004#newcode23
chrome/browser/net/raw_headers_observer.h:23: RawHeadersObserver() { }
Use chrome style please.

http://codereview.chromium.org/3133016/diff/11001/12004#newcode32
chrome/browser/net/raw_headers_observer.h:32: typedef base::hash_map<uint32,
scoped_refptr<URLRequestRecord> >
Do you really need all the information from URLRequestRecord?

http://codereview.chromium.org/3133016/diff/11001/12006
File chrome/browser/renderer_host/async_resource_handler.h (right):

http://codereview.chromium.org/3133016/diff/11001/12006#newcode16
chrome/browser/renderer_host/async_resource_handler.h:16: class ChromeNetLog;
Mind alphabetic order.

http://codereview.chromium.org/3133016/diff/11001/12007
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3133016/diff/11001/12007#newcode145
chrome/browser/renderer_host/resource_dispatcher_host.cc:145:
!policy->CanReadRawCookies(child_id)) {
How does raw cookies access correlate with raw headers?

http://codereview.chromium.org/3133016/diff/11001/12008
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3133016/diff/11001/12008#newcode2019
chrome/chrome_browser.gypi:2019: 'browser/net/raw_headers_observer.h',
Mind the alphabetic order please.

http://codereview.chromium.org/3133016/diff/11001/12012
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/3133016/diff/11001/12012#newcode155
webkit/glue/resource_loader_bridge.h:155: struct RawHeadersInfo :
base::RefCounted<RawHeadersInfo> {
RawHttpHeadersInfo ?

http://codereview.chromium.org/3133016/diff/11001/12012#newcode212
webkit/glue/resource_loader_bridge.h:212: scoped_refptr<RawHeadersInfo>
raw_headers;
Do you envision pointers to the instance co-existing? I am not sure saving the
copy time here is worth the complexity introduced.

[caseq, 2010-08-17 15:51:49]

- Moved net log observer to browser/debugger, renamed accordingly
- Attach/detach the observer on demand from DevToolsManager
- Renamed RawHeadersInfo to a more generic DevToolsInfo.

http://codereview.chromium.org/3133016/diff/11001/12002
File chrome/browser/net/chrome_net_log.h (right):

http://codereview.chromium.org/3133016/diff/11001/12002#newcode65
chrome/browser/net/chrome_net_log.h:65: }
On 2010/08/16 15:06:09, pfeldman wrote:
> missing blank line.

Done^WGone. The method is no longer there.

http://codereview.chromium.org/3133016/diff/11001/12004
File chrome/browser/net/raw_headers_observer.h (right):

http://codereview.chromium.org/3133016/diff/11001/12004#newcode18
chrome/browser/net/raw_headers_observer.h:18: class RawHeadersObserver : public
ChromeNetLog::Observer {
On 2010/08/16 15:06:09, pfeldman wrote:
> Rename this to DevToolsObserver and add it from DevToolsManager when needed?

Done.

http://codereview.chromium.org/3133016/diff/11001/12004#newcode20
chrome/browser/net/raw_headers_observer.h:20: typedef
ResourceLoaderBridge::RawHeadersInfo URLRequestRecord;
On 2010/08/16 15:06:09, pfeldman wrote:
> Rename to RawHeadersInfo? Also, I would use new struct here.

As discussed, we generalized this further by renaming to DevToolsInfo. I suggest
we keep the same struct until we need to maintain any state there that won't be
passed over to ResponseInfo.

http://codereview.chromium.org/3133016/diff/11001/12004#newcode23
chrome/browser/net/raw_headers_observer.h:23: RawHeadersObserver() { }
On 2010/08/16 15:06:09, pfeldman wrote:
> Use chrome style please.

I'm trying to:
E:\caseq\chromium\src>find chrome/ -name \*.h | xargs grep "() {\s*}" | wc -l
1031

http://codereview.chromium.org/3133016/diff/11001/12004#newcode32
chrome/browser/net/raw_headers_observer.h:32: typedef base::hash_map<uint32,
scoped_refptr<URLRequestRecord> >
On 2010/08/16 15:06:09, pfeldman wrote:
> Do you really need all the information from URLRequestRecord?

Yes, it's just meant to be exactly the state we maintain per an URLRequest (I
think there could be some confusion to URLRequestRecord defined in other
places). Anyway, it got renamed.

http://codereview.chromium.org/3133016/diff/11001/12006
File chrome/browser/renderer_host/async_resource_handler.h (right):

http://codereview.chromium.org/3133016/diff/11001/12006#newcode16
chrome/browser/renderer_host/async_resource_handler.h:16: class ChromeNetLog;
On 2010/08/16 15:06:09, pfeldman wrote:
> Mind alphabetic order.

Done.

http://codereview.chromium.org/3133016/diff/11001/12007
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/3133016/diff/11001/12007#newcode145
chrome/browser/renderer_host/resource_dispatcher_host.cc:145:
!policy->CanReadRawCookies(child_id)) {
On 2010/08/16 15:06:09, pfeldman wrote:
> How does raw cookies access correlate with raw headers?

Raw headers include cookie headers. I'm adding this as a comment. Also, I
changed this to quietly discard LOAD_REPORT_RAW_HEADERS flag if renderer lacks
the permission, instead of rejecting the request entirely.

http://codereview.chromium.org/3133016/diff/11001/12008
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3133016/diff/11001/12008#newcode2019
chrome/chrome_browser.gypi:2019: 'browser/net/raw_headers_observer.h',
On 2010/08/16 15:06:09, pfeldman wrote:
> Mind the alphabetic order please.

Done.

http://codereview.chromium.org/3133016/diff/11001/12012
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/3133016/diff/11001/12012#newcode155
webkit/glue/resource_loader_bridge.h:155: struct RawHeadersInfo :
base::RefCounted<RawHeadersInfo> {
On 2010/08/16 15:06:09, pfeldman wrote:
> RawHttpHeadersInfo ?

As discussed, renamed to DevToolsInfo.

http://codereview.chromium.org/3133016/diff/11001/12012#newcode212
webkit/glue/resource_loader_bridge.h:212: scoped_refptr<RawHeadersInfo>
raw_headers;
On 2010/08/16 15:06:09, pfeldman wrote:
> Do you envision pointers to the instance co-existing? I am not sure saving the
> copy time here is worth the complexity introduced.

There are several reasons it's a refcounted pointer, rather than a value or a
plain scoped_ptr:
- We want it to be nullable (i.e. have a clear indication when the info is
available or not).
- It's a part of ResponseInfo, which is copyable, so if we want it to be a
pointer, it should be refcounted.
- It's kept in hash_map in the observer (maintaining raw pointers there would be
cumbersome)
- HttpResponseHeaders is already refcounted
- It may be on the order of several KB, so copying it does seem to be a good
idea.

[caseq, 2010-08-17 16:16:51]

Darin & Eric, could you please have a look?

I'm trying to pull actual headers from the network layer through to DevTools.
There are couple of reasons why we need this:

- Get cookies associated with requests/response (we do show cookies associated
with URL, but this is sort of static, i.e. doesn't allow one to see when cookie
is set, what requests were sent with what cookies etc).
- Get other headers that added late by the network layer or cache (e.g.
Accept-Language & co.)

I realize that sending raw headers that include cookies may appear scary, but we
already provide access to raw cookies if the renderer has permissions that are
set by DevToolsManager, so I'm just reusing the same permission. 

Also, I reckon it may appear strange that we're sending raw headers for both
request and response as part of the ResourceResponseHead -- but there doesn't
seem to be a better place for request headers now, given that these are
available rather late. As for response headers, we can conditionally include
cookies with HttpResponseHeaders instead of sending a separate copy of the
response headers, but I wasn't sure of possible implications of that --
suggestions would be very welcome.

[caseq, 2010-08-19 16:57:00]

Just a gentle ping ;)

[eroman, 2010-08-25 08:27:55]

Sorry for slowness on reviewing this.

I don't think that using the NetLog to get the headers is the right approach.

willchan was working on an API for extensions to get at the HTTP
request/response headers. That interface should be suitable for this use-case as
well.

Will, what do you think?

[caseq, 2010-09-29 18:55:08]

Will & Erik,
it looks like there will be some time before HttpNetworkDelegate is suitable for
our needs. Why don't we proceed with current implementation based on
ChromeNetLog and I'll be happy to switch to using HttpNetworkDelegate when it
provides the functionality we need. I rebased the patch, could someone please
review it?

[pfeldman, 2010-09-30 15:58:48]

http://codereview.chromium.org/3133016/diff/44001/45001
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45001#newcode393
chrome/browser/debugger/devtools_manager.cc:393: ChromeThread::IO, FROM_HERE,
FROM_HERE -> next line

http://codereview.chromium.org/3133016/diff/44001/45001#newcode415
chrome/browser/debugger/devtools_manager.cc:415: ChromeThread::IO, FROM_HERE,
ditto

http://codereview.chromium.org/3133016/diff/44001/45002
File chrome/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/3133016/diff/44001/45002#newcode16
chrome/browser/debugger/devtools_manager.h:16: #include
"webkit/glue/resource_loader_bridge.h"
Do you need this?

http://codereview.chromium.org/3133016/diff/44001/45002#newcode22
chrome/browser/debugger/devtools_manager.h:22: class DevToolsNetLogObserver;
ditto

http://codereview.chromium.org/3133016/diff/44001/45002#newcode24
chrome/browser/debugger/devtools_manager.h:24: class IOThread;
ditto

http://codereview.chromium.org/3133016/diff/44001/45002#newcode27
chrome/browser/debugger/devtools_manager.h:27: using
webkit_glue::ResourceLoaderBridge;
ditto

http://codereview.chromium.org/3133016/diff/44001/45003
File chrome/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45003#newcode15
chrome/browser/debugger/devtools_netlog_observer.cc:15: using
webkit_glue::ResourceLoaderBridge;
We don't really do 'using' in chromium.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode48
chrome/browser/debugger/devtools_netlog_observer.cc:48: load_flags();
wrong indent

http://codereview.chromium.org/3133016/diff/44001/45003#newcode65
chrome/browser/debugger/devtools_netlog_observer.cc:65: if (type !=
net::NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST_HEADERS &&
What about SPDY? Can we get SPDY headers as well?

http://codereview.chromium.org/3133016/diff/44001/45003#newcode68
chrome/browser/debugger/devtools_netlog_observer.cc:68:
scoped_refptr<ResourceInfo> info = GetResourceInfo(source.id);
Adding an empty line above this line would ease reading.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode92
chrome/browser/debugger/devtools_netlog_observer.cc:92: }
You might need the default: case here. Did you try sending this to the bots?

http://codereview.chromium.org/3133016/diff/44001/45003#newcode118
chrome/browser/debugger/devtools_netlog_observer.cc:118: void
DevToolsNetLogObserver::PopulateResponseInfo(URLRequest* request,
ResourceResponse* response) {
80 chars

http://codereview.chromium.org/3133016/diff/44001/45003#newcode123
chrome/browser/debugger/devtools_netlog_observer.cc:123: DevToolsNetLogObserver*
dev_tools_net_log_observer = DevToolsNetLogObserver::GetInstance();
80 chars

http://codereview.chromium.org/3133016/diff/44001/45004
File chrome/browser/debugger/devtools_netlog_observer.h (right):

http://codereview.chromium.org/3133016/diff/44001/45004#newcode14
chrome/browser/debugger/devtools_netlog_observer.h:14: using
webkit_glue::ResourceLoaderBridge;
do you really need this using?

http://codereview.chromium.org/3133016/diff/44001/45004#newcode27
chrome/browser/debugger/devtools_netlog_observer.h:27:
scoped_refptr<ResourceInfo> GetResourceInfo(uint32 id);
Do you need this to be public? Also, you might want to return raw pointer here.

http://codereview.chromium.org/3133016/diff/44001/45004#newcode38
chrome/browser/debugger/devtools_netlog_observer.h:38: // Must be called on the
IO thread. May return NULL if no observers
Please add empty line above comments.

http://codereview.chromium.org/3133016/diff/44001/45004#newcode47
chrome/browser/debugger/devtools_netlog_observer.h:47: typedef
base::hash_map<uint32, scoped_refptr<ResourceInfo>>
Please add space between > >.

http://codereview.chromium.org/3133016/diff/44001/45005
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45005#newcode94
chrome/browser/renderer_host/async_resource_handler.cc:94:
LoadTimingObserver::PopulateTimingInfo(request, response);
LoadTiming is technically non-devtools, you might want to inline this. You might
want to support SyncResourceHandler too.

http://codereview.chromium.org/3133016/diff/44001/45008
File chrome/chrome.gyp (right):

http://codereview.chromium.org/3133016/diff/44001/45008#newcode555
chrome/chrome.gyp:555: 'browser/debugger/devtools_netlog_observer.h',
mind order please.

http://codereview.chromium.org/3133016/diff/44001/45011
File net/http/http_net_log_params.h (right):

http://codereview.chromium.org/3133016/diff/44001/45011#newcode44
net/http/http_net_log_params.h:44: const HttpRequestHeaders& GetHeaders() const
{
Do you need these?

http://codereview.chromium.org/3133016/diff/44001/45011#newcode79
net/http/http_net_log_params.h:79: const HttpResponseHeaders& GetHeaders() const
{
ditto

[caseq, 2010-09-30 18:37:27]

http://codereview.chromium.org/3133016/diff/44001/45001
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45001#newcode393
chrome/browser/debugger/devtools_manager.cc:393: ChromeThread::IO, FROM_HERE,
On 2010/09/30 15:58:49, pfeldman wrote:
> FROM_HERE -> next line

Done.

http://codereview.chromium.org/3133016/diff/44001/45001#newcode415
chrome/browser/debugger/devtools_manager.cc:415: ChromeThread::IO, FROM_HERE,
On 2010/09/30 15:58:49, pfeldman wrote:
> ditto

Done.

http://codereview.chromium.org/3133016/diff/44001/45003
File chrome/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45003#newcode15
chrome/browser/debugger/devtools_netlog_observer.cc:15: using
webkit_glue::ResourceLoaderBridge;
On 2010/09/30 15:58:49, pfeldman wrote:
> We don't really do 'using' in chromium.

E:\caseq\gchromium\src\chrome>ufind . -iname \*.cc | xargs grep "^using " | wc
-l
1902

This one is obsolete, though.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode48
chrome/browser/debugger/devtools_netlog_observer.cc:48: load_flags();
On 2010/09/30 15:58:49, pfeldman wrote:
> wrong indent

This is continuation of above line.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode65
chrome/browser/debugger/devtools_netlog_observer.cc:65: if (type !=
net::NetLog::TYPE_HTTP_TRANSACTION_SEND_REQUEST_HEADERS &&
On 2010/09/30 15:58:49, pfeldman wrote:
> What about SPDY? Can we get SPDY headers as well?

Let's do it as a separate change, it requires additional consideration (SPDY
headers are per-session, not per-request, IIANM)

http://codereview.chromium.org/3133016/diff/44001/45003#newcode68
chrome/browser/debugger/devtools_netlog_observer.cc:68:
scoped_refptr<ResourceInfo> info = GetResourceInfo(source.id);
On 2010/09/30 15:58:49, pfeldman wrote:
> Adding an empty line above this line would ease reading.

Done.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode92
chrome/browser/debugger/devtools_netlog_observer.cc:92: }
On 2010/09/30 15:58:49, pfeldman wrote:
> You might need the default: case here. Did you try sending this to the bots?

Done.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode118
chrome/browser/debugger/devtools_netlog_observer.cc:118: void
DevToolsNetLogObserver::PopulateResponseInfo(URLRequest* request,
ResourceResponse* response) {
On 2010/09/30 15:58:49, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/3133016/diff/44001/45003#newcode123
chrome/browser/debugger/devtools_netlog_observer.cc:123: DevToolsNetLogObserver*
dev_tools_net_log_observer = DevToolsNetLogObserver::GetInstance();
On 2010/09/30 15:58:49, pfeldman wrote:
> 80 chars

Done.

http://codereview.chromium.org/3133016/diff/44001/45004
File chrome/browser/debugger/devtools_netlog_observer.h (right):

http://codereview.chromium.org/3133016/diff/44001/45004#newcode14
chrome/browser/debugger/devtools_netlog_observer.h:14: using
webkit_glue::ResourceLoaderBridge;
On 2010/09/30 15:58:49, pfeldman wrote:
> do you really need this using?

Removed (this is obsolete).

http://codereview.chromium.org/3133016/diff/44001/45004#newcode27
chrome/browser/debugger/devtools_netlog_observer.h:27:
scoped_refptr<ResourceInfo> GetResourceInfo(uint32 id);
On 2010/09/30 15:58:49, pfeldman wrote:
> Do you need this to be public? Also, you might want to return raw pointer
here.

You're corect, this does not have to be public anymore -- it used to be before
PopulateResponse became member of DevToolsNetLogObserver.

http://codereview.chromium.org/3133016/diff/44001/45004#newcode38
chrome/browser/debugger/devtools_netlog_observer.h:38: // Must be called on the
IO thread. May return NULL if no observers
On 2010/09/30 15:58:49, pfeldman wrote:
> Please add empty line above comments.

Done.

http://codereview.chromium.org/3133016/diff/44001/45004#newcode47
chrome/browser/debugger/devtools_netlog_observer.h:47: typedef
base::hash_map<uint32, scoped_refptr<ResourceInfo>>
On 2010/09/30 15:58:49, pfeldman wrote:
> Please add space between > >.

Done.

http://codereview.chromium.org/3133016/diff/44001/45005
File chrome/browser/renderer_host/async_resource_handler.cc (right):

http://codereview.chromium.org/3133016/diff/44001/45005#newcode94
chrome/browser/renderer_host/async_resource_handler.cc:94:
LoadTimingObserver::PopulateTimingInfo(request, response);
On 2010/09/30 15:58:49, pfeldman wrote:
> LoadTiming is technically non-devtools, you might want to inline this. You
might
> want to support SyncResourceHandler too.

Done.

http://codereview.chromium.org/3133016/diff/44001/45008
File chrome/chrome.gyp (right):

http://codereview.chromium.org/3133016/diff/44001/45008#newcode555
chrome/chrome.gyp:555: 'browser/debugger/devtools_netlog_observer.h',
On 2010/09/30 15:58:49, pfeldman wrote:
> mind order please.

Done.

http://codereview.chromium.org/3133016/diff/44001/45011
File net/http/http_net_log_params.h (right):

http://codereview.chromium.org/3133016/diff/44001/45011#newcode44
net/http/http_net_log_params.h:44: const HttpRequestHeaders& GetHeaders() const
{
On 2010/09/30 15:58:49, pfeldman wrote:
> Do you need these?

Yes -- headers_ are private and I need to extract them in
DevToolsNetLogObserver::OnAddEntry()

http://codereview.chromium.org/3133016/diff/44001/45011#newcode79
net/http/http_net_log_params.h:79: const HttpResponseHeaders& GetHeaders() const
{
On 2010/09/30 15:58:49, pfeldman wrote:
> ditto

ditto.

[pfeldman, 2010-10-01 08:54:50]

LGTM with nits. Please follow up with the test shortly as we agreed offline.

http://codereview.chromium.org/3133016/diff/56001/57001
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/3133016/diff/56001/57001#newcode16
chrome/browser/debugger/devtools_manager.cc:16: #include
"chrome/browser/debugger/devtools_netlog_observer.h"
Please mind include order.

http://codereview.chromium.org/3133016/diff/56001/57003
File chrome/browser/debugger/devtools_netlog_observer.cc (right):

http://codereview.chromium.org/3133016/diff/56001/57003#newcode19
chrome/browser/debugger/devtools_netlog_observer.cc:19:
DevToolsNetLogObserver::DevToolsNetLogObserver(ChromeNetLog* chrome_net_log)
Note that implementation order should match the one declared in .h.

http://codereview.chromium.org/3133016/diff/56001/57004
File chrome/browser/debugger/devtools_netlog_observer.h (right):

http://codereview.chromium.org/3133016/diff/56001/57004#newcode41
chrome/browser/debugger/devtools_netlog_observer.h:41: ResourceInfo*
GetResourceInfo(uint32 id);
This should go after the constructor.

http://codereview.chromium.org/3133016/diff/56001/57007
File chrome/browser/renderer_host/sync_resource_handler.cc (right):

http://codereview.chromium.org/3133016/diff/56001/57007#newcode62
chrome/browser/renderer_host/sync_resource_handler.cc:62: 
You should populate data here too, do not hesitate :)