Allows deferral of a URLRequest just before talking to the network, at

Allows deferral of a URLRequest just before talking to the network, at
DoCreateStream time in the HttpNetworkTransaction. This hook allows
for experimentation with the ResourceScheduler so that it can defer
after first checking the cache, gathering the cookies, and starting
the WebRequest but before going to the network.

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

[mattm, 2014-01-03 23:55:54]

redirecting

[mmenke, 2014-01-06 21:55:00]

On 2014/01/03 23:55:54, mattm wrote:
> redirecting

jkarlin:  I think I'll hold off on review until you make the behavioral change
discussed in the doc, unless you really want me to review it as-is (Which I'm
happy to do, just busy).

[mmenke, 2014-01-06 21:56:00]

On 2014/01/06 21:55:00, mmenke wrote:
> On 2014/01/03 23:55:54, mattm wrote:
> > redirecting
> 
> jkarlin:  I think I'll hold off on review until you make the behavioral change
> discussed in the doc, unless you really want me to review it as-is (Which I'm
> happy to do, just busy).

Oh...oops...looks like you already did that.  I'll try to get to this tomorrow.

[jkarlin, 2014-01-06 22:43:51]

On 2014/01/06 21:56:00, mmenke wrote:
> On 2014/01/06 21:55:00, mmenke wrote:
> > On 2014/01/03 23:55:54, mattm wrote:
> > > redirecting
> > 
> > jkarlin:  I think I'll hold off on review until you make the behavioral
change
> > discussed in the doc, unless you really want me to review it as-is (Which
I'm
> > happy to do, just busy).
> 
> Oh...oops...looks like you already did that.  I'll try to get to this
tomorrow.

Yeah, I just need to do some renaming from OnNetworkCreateStream to something
like OnNetworkFirstUse.  I'll do that in the morning before you're in so it's
all set for you.  Thanks!

[jkarlin, 2014-01-07 14:33:23]

On 2014/01/06 22:43:51, jkarlin wrote:
> On 2014/01/06 21:56:00, mmenke wrote:
> > On 2014/01/06 21:55:00, mmenke wrote:
> > > On 2014/01/03 23:55:54, mattm wrote:
> > > > redirecting
> > > 
> > > jkarlin:  I think I'll hold off on review until you make the behavioral
> change
> > > discussed in the doc, unless you really want me to review it as-is (Which
> I'm
> > > happy to do, just busy).
> > 
> > Oh...oops...looks like you already did that.  I'll try to get to this
> tomorrow.
> 
> Yeah, I just need to do some renaming from OnNetworkCreateStream to something
> like OnNetworkFirstUse.  I'll do that in the morning before you're in so it's
> all set for you.  Thanks!

All set, review away!

[mmenke, 2014-01-07 15:26:23]

This looks really good to me - I'll get back with more comments later today, and
these are all pretty minor, so feel free to either hold off on responding to my
comments or not.

Some of these were made when I was reviewing set 4 yesterday, so if some
comments look outdated, they may well be.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:135: const base::Callback<void(bool*)>&
callback) OVERRIDE;
Should make the callback a typedef with a comment on what the bool* is.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:137: private:
nit:  Blank line before private.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction.cc:707: return DoLoop(OK);
This should just return OK, since we're already in a call to DoLoop, and can
just let the parent call continue along.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:409:
BeforeNetworkStartHandler(bool defer)
explicit

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:423: bool
defer_on_before_network_start_;
const?

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:424: bool
observed_before_network_start_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1457:
base::MessageLoop::current()->RunUntilIdle();
Are these needed?  TearDown() already does its fair share of this.  I'm not
really that opposed, just wondering what's going on.

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

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:203: *defer = false;
"OnReceivedRedirect" assumes defer is initialized to false.  I think we should
be consistent - and having it auto-initialized seems safer to me.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:838: if (!(*defer))
optional nit:  I think !*defer is more common in net, though it's not exactly
common.  If you think this is clearer, I don't feel particularly strongly about
it.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:844: DCHECK(job_);
Maybe DCHECK(notified_network_create_stream_);?

I'd also suggest setting notified_network_create_stream_ to true after the call
into the delegate, to make the DCHECK trigger on re-entrancy.  I don't believe
the ResumeNetworkCreateStream DCHECK will trigger in that case, and I'd like to
have some protection against it, at least in debug builds.

We could try and handle that case, but I don't think there's a reason to.

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

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request_http_job.h:264: URLRequest* request_;
Not needed.

URLRequestJob::request_ is protected, so can just use that.  Also, you don't
seem to add any new uses of this, anyways.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:4158: entries, log_position + 1);
nit:  Use braces when body of if takes up more than one line.

[jkarlin, 2014-01-07 16:19:31]

Awesome, ready for the next round.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:135: const base::Callback<void(bool*)>&
callback) OVERRIDE;
On 2014/01/07 15:26:23, mmenke wrote:
> Should make the callback a typedef with a comment on what the bool* is.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:137: private:
On 2014/01/07 15:26:23, mmenke wrote:
> nit:  Blank line before private.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction.cc:707: return DoLoop(OK);
On 2014/01/07 15:26:23, mmenke wrote:
> This should just return OK, since we're already in a call to DoLoop, and can
> just let the parent call continue along.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:409:
BeforeNetworkStartHandler(bool defer)
On 2014/01/07 15:26:23, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:423: bool
defer_on_before_network_start_;
On 2014/01/07 15:26:23, mmenke wrote:
> const?

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:424: bool
observed_before_network_start_;
On 2014/01/07 15:26:23, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/122453002/diff/460001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1457:
base::MessageLoop::current()->RunUntilIdle();
On 2014/01/07 15:26:23, mmenke wrote:
> Are these needed?  TearDown() already does its fair share of this.  I'm not
> really that opposed, just wondering what's going on.

Fair enough. Removed.

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

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:203: *defer = false;
On 2014/01/07 15:26:23, mmenke wrote:
> "OnReceivedRedirect" assumes defer is initialized to false.  I think we should
> be consistent - and having it auto-initialized seems safer to me.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:838: if (!(*defer))
On 2014/01/07 15:26:23, mmenke wrote:
> optional nit:  I think !*defer is more common in net, though it's not exactly
> common.  If you think this is clearer, I don't feel particularly strongly
about
> it.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request.cc:844: DCHECK(job_);
On 2014/01/07 15:26:23, mmenke wrote:
> Maybe DCHECK(notified_network_create_stream_);?
> 
> I'd also suggest setting notified_network_create_stream_ to true after the
call
> into the delegate, to make the DCHECK trigger on re-entrancy.  I don't believe
> the ResumeNetworkCreateStream DCHECK will trigger in that case, and I'd like
to
> have some protection against it, at least in debug builds.
> 
> We could try and handle that case, but I don't think there's a reason to.

Done.

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

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request_http_job.h:264: URLRequest* request_;
On 2014/01/07 15:26:23, mmenke wrote:
> Not needed.
> 
> URLRequestJob::request_ is protected, so can just use that.  Also, you don't
> seem to add any new uses of this, anyways.

Done.

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/460001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:4158: entries, log_position + 1);
On 2014/01/07 15:26:23, mmenke wrote:
> nit:  Use braces when body of if takes up more than one line.

Done.

[mmenke, 2014-01-07 19:44:05]

https://codereview.chromium.org/122453002/diff/840018/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_cache_tra...
net/http/http_cache_transaction.h:444: base::Callback<void(bool*)>
before_network_start_callback_;
base::Callback<void(bool*)> -> BeforeNetworkStartCallback

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction.h:340: base::Callback<void(bool*)>
before_network_start_callback_;
base::Callback<void(bool*)> -> BeforeNetworkStartCallback

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1389: MockRead(SYNCHRONOUS, 0), };
optional nit (x2):  I don't think I've seen this done much - whenever there's an
extra comma at the end, the close brace is generally put on the next line.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1415:
EXPECT_EQ(ERR_CONTENT_LENGTH_MISMATCH, rv);
Any reason not to use a success case instead?  (OR as well?)

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1419: EXPECT_EQ(0,
GetIdleSocketCountInTransportSocketPool(session.get()));
Why are we checking this?

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1445:
session_deps_.socket_factory->AddSocketDataProvider(&data);
We don't need all this.  In fact, it's better not to have it.  That way, if the
test tries to make a socket, things fail, badly.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
net/http/http_transaction.h:31: // The bool* is used to inform the caller if it
needs to defer or not.
I think this could be clearer.

Maybe something like:

// If |*defer| is set to true, the transaction will wait until
// ResumeNetworkStart is called before establishing a connection.
typedef base::Callback<void(bool* defer)> BeforeNetworkStartCallback;

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
File net/http/http_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
net/http/http_transaction_unittest.cc:420: // should not get here
nit:  Per style guide, comments should generally start with a capital and end
with a period.

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

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request.h:712: friend class URLRequestHttpJob;
I don't think this is needed.

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3830: static size_t
ExpectBeforeNetworkStartBeginAndEnd(
I find this name hard to parse.

Maybe just ExpectBeforeNetworkEvents?

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3830: static size_t
ExpectBeforeNetworkStartBeginAndEnd(
Also, should have some sort of comment.

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:5265: URLRequest
req(test_server_.GetURL("server-redirect?simple.html"),
Why do we use simple.html here and echo in the other two?  Is there even a
simple.html file?

[jkarlin, 2014-01-07 20:10:03]

https://codereview.chromium.org/122453002/diff/840018/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_cache_tra...
net/http/http_cache_transaction.h:444: base::Callback<void(bool*)>
before_network_start_callback_;
On 2014/01/07 19:44:05, mmenke wrote:
> base::Callback<void(bool*)> -> BeforeNetworkStartCallback

Done. And three other places I missed before.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction.h:340: base::Callback<void(bool*)>
before_network_start_callback_;
On 2014/01/07 19:44:05, mmenke wrote:
> base::Callback<void(bool*)> -> BeforeNetworkStartCallback

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1389: MockRead(SYNCHRONOUS, 0), };
On 2014/01/07 19:44:05, mmenke wrote:
> optional nit (x2):  I don't think I've seen this done much - whenever there's
an
> extra comma at the end, the close brace is generally put on the next line.

Huh, I wonder what's up with clang-format.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1415:
EXPECT_EQ(ERR_CONTENT_LENGTH_MISMATCH, rv);
On 2014/01/07 19:44:05, mmenke wrote:
> Any reason not to use a success case instead?  (OR as well?)

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1419: EXPECT_EQ(0,
GetIdleSocketCountInTransportSocketPool(session.get()));
On 2014/01/07 19:44:05, mmenke wrote:
> Why are we checking this?

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1419: EXPECT_EQ(0,
GetIdleSocketCountInTransportSocketPool(session.get()));
On 2014/01/07 19:44:05, mmenke wrote:
> Why are we checking this?

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1445:
session_deps_.socket_factory->AddSocketDataProvider(&data);
On 2014/01/07 19:44:05, mmenke wrote:
> We don't need all this.  In fact, it's better not to have it.  That way, if
the
> test tries to make a socket, things fail, badly.

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
net/http/http_transaction.h:31: // The bool* is used to inform the caller if it
needs to defer or not.
On 2014/01/07 19:44:05, mmenke wrote:
> I think this could be clearer.
> 
> Maybe something like:
> 
> // If |*defer| is set to true, the transaction will wait until
> // ResumeNetworkStart is called before establishing a connection.
> typedef base::Callback<void(bool* defer)> BeforeNetworkStartCallback;

Done.

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
File net/http/http_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/http/http_transacti...
net/http/http_transaction_unittest.cc:420: // should not get here
On 2014/01/07 19:44:05, mmenke wrote:
> nit:  Per style guide, comments should generally start with a capital and end
> with a period.

Done.

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

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request.h:712: friend class URLRequestHttpJob;
On 2014/01/07 19:44:05, mmenke wrote:
> I don't think this is needed.

Done.

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3830: static size_t
ExpectBeforeNetworkStartBeginAndEnd(
On 2014/01/07 19:44:05, mmenke wrote:
> I find this name hard to parse.
> 
> Maybe just ExpectBeforeNetworkEvents?

Done.

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:3830: static size_t
ExpectBeforeNetworkStartBeginAndEnd(
On 2014/01/07 19:44:05, mmenke wrote:
> Also, should have some sort of comment.

Done.

https://codereview.chromium.org/122453002/diff/840018/net/url_request/url_req...
net/url_request/url_request_unittest.cc:5265: URLRequest
req(test_server_.GetURL("server-redirect?simple.html"),
On 2014/01/07 19:44:05, mmenke wrote:
> Why do we use simple.html here and echo in the other two?  Is there even a
> simple.html file?

Done.

[mmenke, 2014-01-07 20:31:38]

LGTM

https://codereview.chromium.org/122453002/diff/990001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/990001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1388: MockRead("Content-Length:
100\r\n\r\n"),
Could you just change this to content-length: 5.  Or even just remove the
content-length and the keep-alive.  Doesn't seem like those add anything to the
test, and seems weird as-is.

[jkarlin, 2014-01-08 12:15:09]

https://codereview.chromium.org/122453002/diff/990001/net/http/http_network_t...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/122453002/diff/990001/net/http/http_network_t...
net/http/http_network_transaction_unittest.cc:1388: MockRead("Content-Length:
100\r\n\r\n"),
On 2014/01/07 20:31:38, mmenke wrote:
> Could you just change this to content-length: 5.  Or even just remove the
> content-length and the keep-alive.  Doesn't seem like those add anything to
the
> test, and seems weird as-is.

Done.

[commit-bot: I haz the power, 2014-01-08 13:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/122453002/1260001

[commit-bot: I haz the power, 2014-01-08 15:40:49]

Change committed as 243568