Plumb ConnectionAttempts from HttpNetworkTransaction to URLRequest.

Plumb ConnectionAttempts from HttpNetworkTransaction to URLRequest.

BUG=480565
Committed: https://crrev.com/3ae06923eff86f2c91144ac4f41231018e2477af
Cr-Commit-Position: refs/heads/master@{#329270}

[Deprecated (see juliatuttle), 2015-04-24 02:34:13]

ttuttle@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Deprecated (see juliatuttle), 2015-04-24 02:34:13]

PTAL when you get a chance; note dependency in CL description.

[Deprecated (see juliatuttle), 2015-04-27 22:24:47]

Patchset #2 (id:20001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:24:53]

Patchset #2 (id:40001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:25:03]

Patchset #1 (id:1) has been deleted

[Randy Smith (Not in Mondays), 2015-05-01 17:03:44]

In general, I think of CL dependency information as being better in CL comments
than in the CL description.  What should go in the CL description is information
that'll be interesting after commit/six months down the road/to someone looking
at the CL who's got no clue about the context after it's landed.  Could you
remove the dependency link from the CL description?  (Can put it in a comment or
not as you want, since that CL's already landed.)

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.cc:1189: void
URLRequest::GetConnectionAttempts(ConnectionAttempts* out) const {
Actually, you might be able to get away without having the #include here either;
you're not using any structure from the ConnectionAttempts definition.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.h:31: #include "net/socket/connection_attempts.h"
Unneeded; just forward declare ConnectionAttempts and do the inclusion in the
.cc file.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.h:60: void
GetConnectionAttempts(ConnectionAttempts* out) const override;
IWYU: I think you should forward declare ConnectionAttempts in this file even
though it's clearly not causing compile problems.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.h:60: void
GetConnectionAttempts(ConnectionAttempts* out) const override;
It looks to me as if priority() isn't an override of URLRequestJob, which means
this should be above it with the other overrides from URLRequestJob.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.h:22: #include
"net/socket/connection_attempts.h"
Replace with forward decl.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.h:230: virtual void
GetConnectionAttempts(ConnectionAttempts* out) const;
I think commenting of method functionality is more important in base classes
than usual.  Maybe a note about what this is doing (though that's actually
pretty obvious from the signature) along with when during the job lifetime it's
appropriate to call it or something like that?  (I don't have strong feelings
about what should be in the comment, I just don't want people going "Yuck,
another random base class method I've got no clue about" when they see it.)

[Randy Smith (Not in Mondays), 2015-05-01 17:08:20]

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.cc:1189: void
URLRequest::GetConnectionAttempts(ConnectionAttempts* out) const {
On 2015/05/01 17:03:43, rdsmith wrote:
> Actually, you might be able to get away without having the #include here
either;
> you're not using any structure from the ConnectionAttempts definition.

Actually (:-}), please ignore this comment; out->clear() uses ConnectionAttempts
structure.

[Deprecated (see juliatuttle), 2015-05-04 19:53:37]

PTAL.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.cc:1189: void
URLRequest::GetConnectionAttempts(ConnectionAttempts* out) const {
On 2015/05/01 17:08:20, rdsmith wrote:
> On 2015/05/01 17:03:43, rdsmith wrote:
> > Actually, you might be able to get away without having the #include here
> either;
> > you're not using any structure from the ConnectionAttempts definition.
> 
> Actually (:-}), please ignore this comment; out->clear() uses
ConnectionAttempts
> structure.

Acknowledged.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.cc:1189: void
URLRequest::GetConnectionAttempts(ConnectionAttempts* out) const {
On 2015/05/01 17:03:43, rdsmith wrote:
> Actually, you might be able to get away without having the #include here
either;
> you're not using any structure from the ConnectionAttempts definition.

Acknowledged.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request.h:31: #include "net/socket/connection_attempts.h"
On 2015/05/01 17:03:44, rdsmith wrote:
> Unneeded; just forward declare ConnectionAttempts and do the inclusion in the
> .cc file.

Acknowledged.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.h:60: void
GetConnectionAttempts(ConnectionAttempts* out) const override;
On 2015/05/01 17:03:44, rdsmith wrote:
> IWYU: I think you should forward declare ConnectionAttempts in this file even
> though it's clearly not causing compile problems.

Done.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_http_job.h:60: void
GetConnectionAttempts(ConnectionAttempts* out) const override;
On 2015/05/01 17:03:44, rdsmith wrote:
> It looks to me as if priority() isn't an override of URLRequestJob, which
means
> this should be above it with the other overrides from URLRequestJob.

Done.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.h:22: #include
"net/socket/connection_attempts.h"
On 2015/05/01 17:03:44, rdsmith wrote:
> Replace with forward decl.

Can't, it's a typedef.

https://codereview.chromium.org/1097303003/diff/80001/net/url_request/url_req...
net/url_request/url_request_job.h:230: virtual void
GetConnectionAttempts(ConnectionAttempts* out) const;
On 2015/05/01 17:03:44, rdsmith wrote:
> I think commenting of method functionality is more important in base classes
> than usual.  Maybe a note about what this is doing (though that's actually
> pretty obvious from the signature) along with when during the job lifetime
it's
> appropriate to call it or something like that?  (I don't have strong feelings
> about what should be in the comment, I just don't want people going "Yuck,
> another random base class method I've got no clue about" when they see it.)

Done.

[Deprecated (see juliatuttle), 2015-05-08 23:54:05]

Ping?

[Randy Smith (Not in Mondays), 2015-05-11 20:59:31]

LGTM with comment nit in url_request_job.h below (other comment nit up to you).

https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:612: void
GetConnectionAttempts(ConnectionAttempts* out) const;
Should this have a comment as to when during the URLRequest lifetime this will
be valid and what it will include?  Or is that overkill?  (Up to you.)

https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
net/url_request/url_request_job.h:232: // the response has been sent or after
the connection has failed.
"response has been sent".  Do you mean "response has been received" or "request
has been sent"?  On the client side, I'm not sure what "response has been sent"
means.

[Randy Smith (Not in Mondays), 2015-05-11 21:02:35]

On 2015/05/11 20:59:31, rdsmith wrote:
> LGTM with comment nit in url_request_job.h below (other comment nit up to
you).
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> File net/url_request/url_request.h (right):
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> net/url_request/url_request.h:612: void
> GetConnectionAttempts(ConnectionAttempts* out) const;
> Should this have a comment as to when during the URLRequest lifetime this will
> be valid and what it will include?  Or is that overkill?  (Up to you.)
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> File net/url_request/url_request_job.h (right):
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> net/url_request/url_request_job.h:232: // the response has been sent or after
> the connection has failed.
> "response has been sent".  Do you mean "response has been received" or
"request
> has been sent"?  On the client side, I'm not sure what "response has been
sent"
> means.

Whoops, also remove the dependency info from the CL (LGTM also contingent on
that).

[Deprecated (see juliatuttle), 2015-05-11 21:07:54]

On 2015/05/11 20:59:31, rdsmith wrote:
> LGTM with comment nit in url_request_job.h below (other comment nit up to
you).
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> File net/url_request/url_request.h (right):
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> net/url_request/url_request.h:612: void
> GetConnectionAttempts(ConnectionAttempts* out) const;
> Should this have a comment as to when during the URLRequest lifetime this will
> be valid and what it will include?  Or is that overkill?  (Up to you.)
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> File net/url_request/url_request_job.h (right):
> 
>
https://codereview.chromium.org/1097303003/diff/140001/net/url_request/url_re...
> net/url_request/url_request_job.h:232: // the response has been sent or after
> the connection has failed.
> "response has been sent".  Do you mean "response has been received" or
"request
> has been sent"?  On the client side, I'm not sure what "response has been
sent"
> means.

Both fixed, and removed the dependency comment in the description. Off to the CQ
we go...

[Deprecated (see juliatuttle), 2015-05-11 22:03:28]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-05-11 22:03:28]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1097303003/#ps160001
(title: "Tweak a couple of comments")

[commit-bot: I haz the power, 2015-05-11 22:04:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097303003/160001

[commit-bot: I haz the power, 2015-05-11 23:42:02]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-05-11 23:42:59]

Patchset 6 (id:??) landed as
https://crrev.com/3ae06923eff86f2c91144ac4f41231018e2477af
Cr-Commit-Position: refs/heads/master@{#329270}