Added HTTP response code field to ResourceRequestDetails.

Added HTTP response code field to ResourceRequestDetails.

BUG=361585
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262987

[Charlie Reis, 2014-04-07 16:17:44]

I'll defer to Darin, but it seems like the response code is something that
should be associated with ResourceResponse, not ResourceRequest.

[Charlie Reis, 2014-04-07 16:20:38]

Also, please try to include some information in the CL for why you need this. 
In general, we include a BUG=NNN line and a test in CLs to help reviewers see
why the change is needed and to prevent regressions.

[pdamek, 2014-04-08 11:33:47]

Well, this change is used by some Opera's client code, so actually I can't point
to any BUG.

We observe WebContents using WebContentsObserver implementing the following
method:

// This method is invoked when a response has been received for a resource
// request.
virtual void DidGetResourceResponseStart(
    const ResourceRequestDetails& details) {}

The ResourceRequest you mentioned seems to be quite far away. Actually
ResourceRequestDetails already contains data connected with a response (or lack
of it), like net::URLRequestStatus status, which contains an error code. So I
think this is just some naming mismatch...

[Charlie Reis, 2014-04-08 19:58:05]

Darin's out this week, so I'll add mmenke for his thoughts.

On 2014/04/08 11:33:47, pdamek wrote:
> Well, this change is used by some Opera's client code, so actually I can't
point
> to any BUG.

Ah.  I would still recommend filing a bug for it.  That's really helpful for
people who look at this change later and wonder what it's for.  Just mention the
use case you're trying to support in the bug.

> 
> We observe WebContents using WebContentsObserver implementing the following
> method:
> 
> // This method is invoked when a response has been received for a resource
> // request.
> virtual void DidGetResourceResponseStart(
>     const ResourceRequestDetails& details) {}
> 
> The ResourceRequest you mentioned seems to be quite far away. Actually
> ResourceRequestDetails already contains data connected with a response (or
lack
> of it), like net::URLRequestStatus status, which contains an error code. So I
> think this is just some naming mismatch...

Matt, maybe you can weigh in on whether there's a better way to support this?

[mmenke, 2014-04-08 20:44:59]

On 2014/04/08 19:58:05, Charlie Reis wrote:
> Darin's out this week, so I'll add mmenke for his thoughts.
> 
> On 2014/04/08 11:33:47, pdamek wrote:
> > Well, this change is used by some Opera's client code, so actually I can't
> point
> > to any BUG.
> 
> Ah.  I would still recommend filing a bug for it.  That's really helpful for
> people who look at this change later and wonder what it's for.  Just mention
the
> use case you're trying to support in the bug.
> 
> > 
> > We observe WebContents using WebContentsObserver implementing the following
> > method:
> > 
> > // This method is invoked when a response has been received for a resource
> > // request.
> > virtual void DidGetResourceResponseStart(
> >     const ResourceRequestDetails& details) {}
> > 
> > The ResourceRequest you mentioned seems to be quite far away. Actually
> > ResourceRequestDetails already contains data connected with a response (or
> lack
> > of it), like net::URLRequestStatus status, which contains an error code. So
I
> > think this is just some naming mismatch...
> 
> Matt, maybe you can weigh in on whether there's a better way to support this?

I'm not aware of one.  I don't believe we currently have http status codes
anywhere on the UI thread, either for frames or for all resources.  There aren't
a whole lot of cases where we pass information on subresources to the UI thread,
so if that's what's needed, this is probably the simplest way to go (Or could
add another argument to DidGetResourceResponseStart, but I don't think that
really gets us much).  Could rename the structure to something more accurate,
though I'm not sure [Resource]ResponseStartedDetails is a whole lot clearer.

If the information is just needed for frames, could pass this information along
from the renderer process on commit, instead, I suppose, like we do with net
error codes.

The only other options I can think of, without more context of what it's being
used for, would be to hook up ResourceThrottle to grab the information, or grab
it on the ChromeNetworkDelegate.

[Charlie Reis, 2014-04-08 23:22:29]

Huh, ok.  I don't know this API well enough to say, so I'll defer to mmenke's
comment.  Sounds like this could be a useful addition then.

A few style nits below, and then we can proceed once it has a bug number.

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
File content/public/browser/resource_request_details.cc (right):

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
content/public/browser/resource_request_details.cc:30:
request->response_info().headers ?
Seems like we want a .get() here to be clear.

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
content/public/browser/resource_request_details.cc:32: : -1;
nit: The colon belongs on the previous line.  (It's reasonable to put the -1 on
the previous line as well.)

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
File content/public/browser/resource_request_details.h (right):

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
content/public/browser/resource_request_details.h:45: // -1 if there are no
response headers yet for some reason.
nit: Remove "for some reason."

https://codereview.chromium.org/226183015/diff/1/content/public/browser/resou...
content/public/browser/resource_request_details.h:46: // Can be also 0 if the
response code text seems to exist but could not be
Most of this comment seems to be taken from http_response_headers.h.  Rather
than copying and reordering it here, let's just say "See
HttpResponseHeaders::response_code()," plus your line about -1 (since that's new
behavior here).

[mmenke, 2014-04-09 14:52:56]

Ahh...I missed that LoadCommittedDetails already includes http status code for
frames.

pdamek:  I assume you need the error code for all resources.

[pdamek, 2014-04-09 16:11:33]

On 2014/04/09 14:52:56, mmenke wrote:
> Ahh...I missed that LoadCommittedDetails already includes http status code for
> frames.

I suppose the methods of WebContentsObserver that use LoadCommittedDetails are
called in different time. From what I investigated DidGetResourceResponseStart
is called really early after receiving the first bytes of the response. And that
is crucial for us.

> 
> pdamek:  I assume you need the error code for all resources.

Right. I mostly pointed it out to show similarity, because http code is also a
possible error code from higher protocol.

[mmenke, 2014-04-09 16:17:14]

On 2014/04/09 16:11:33, pdamek wrote:
> On 2014/04/09 14:52:56, mmenke wrote:
> > Ahh...I missed that LoadCommittedDetails already includes http status code
for
> > frames.
> 
> I suppose the methods of WebContentsObserver that use LoadCommittedDetails are
> called in different time. From what I investigated DidGetResourceResponseStart
> is called really early after receiving the first bytes of the response. And
that
> is crucial for us.

Yea, LoadCommittedDetails are round-tripped through the renderer, and there are
some intervening events before commit on cross-process navigation as well. 
DidGetResourceResponseStart comes straight from the network objects living on
the IO thread, bypassing the renderer entirely.  (You may know all this already,
just thought I'd give a little more context if not).

> > 
> > pdamek:  I assume you need the error code for all resources.
> 
> Right. I mostly pointed it out to show similarity, because http code is also a
> possible error code from higher protocol.

[pdamek, 2014-04-09 16:31:12]

> On 2014/04/09 14:52:56, mmenke wrote:
> > Ahh...I missed that LoadCommittedDetails already includes http status code
for
> > frames.
> 

So the issue I opened for this patch -
http://code.google.com/p/chromium/issues/detail?id=361585 might need name change
in order to be more precise. I can't do it unfortunatelly...

[Charlie Reis, 2014-04-09 20:05:26]

Thanks.  LGTM.

[pdamek, 2014-04-10 12:29:50]

The CQ bit was checked by pdamek@opera.com

[commit-bot: I haz the power, 2014-04-10 12:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdamek@opera.com/226183015/20001

[commit-bot: I haz the power, 2014-04-10 14:39:51]

Change committed as 262987