Simplify HttpCache/HttpNetworkLayer/HttpNetworkSession interaction.

Simplify HttpCache/HttpNetworkLayer/HttpNetworkSession interaction.

Eliminate lazy initialization of HttpNetworkSession in HttpNetworkLayer.
  * This eliminates the need to update parameters for HttpNetworkLayer, it just takes a HttpNetworkSession.
  * It is OK to eliminate lazy initialization since these variables are cheap.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72931

[willchan no longer on Chromium, 2011-01-27 04:31:04]

wtc: This is step one. I'll probably introduce a HttpNetworkSession::Params
struct after this.

[wtc, 2011-01-28 01:51:23]

LGTM.

Please note the two comments marked with "IMPORTANT" below.

All the other comments are just nits, some of which you may
ignore at your discretion.

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:494: net::HttpNetworkSession*
network_session = main_cache->GetSession();
IMPORTANT: Why do we not need the null check for 'main_cache'
in the original code?

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester.cc:85: NULL));
Nit: add a /* net_log */ comment after this NULL?

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:134: NULL));
Add /* network_delegate */ and /* net_log */ after these NULLs?

http://codereview.chromium.org/6402002/diff/11001/chrome/service/net/service_...
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/service/net/service_...
chrome/service/net/service_url_request_context.cc:142: NULL));
Nit: add /* network_delegate */ and /* net_log */ after these
NULLs?

http://codereview.chromium.org/6402002/diff/11001/chrome/test/plugin/plugin_t...
File chrome/test/plugin/plugin_test.cpp (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/test/plugin/plugin_t...
chrome/test/plugin/plugin_test.cpp:301: NULL));
Nit: add /* net_log */ after this NULL?

http://codereview.chromium.org/6402002/diff/11001/chrome_frame/metrics_servic...
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome_frame/metrics_servic...
chrome_frame/metrics_service.cc:196: NULL);
Nit: add /* net_log */ after this NULL?

http://codereview.chromium.org/6402002/diff/11001/chrome_frame/test/test_serv...
File chrome_frame/test/test_server_test.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome_frame/test/test_serv...
chrome_frame/test/test_server_test.cc:88: NULL),
Nit: add /* network_delegate */ and /* net_log */ after these
NULLs?

http://codereview.chromium.org/6402002/diff/11001/net/proxy/proxy_script_fetc...
File net/proxy/proxy_script_fetcher_impl_unittest.cc (right):

http://codereview.chromium.org/6402002/diff/11001/net/proxy/proxy_script_fetc...
net/proxy/proxy_script_fetcher_impl_unittest.cc:76: };
Should we put lines 31-76 in an unnamed namespace, nested
inside the 'net' namespace?

http://codereview.chromium.org/6402002/diff/11001/net/spdy/spdy_test_util.cc
File net/spdy/spdy_test_util.cc (right):

http://codereview.chromium.org/6402002/diff/11001/net/spdy/spdy_test_util.cc#...
net/spdy/spdy_test_util.cc:13: #include "net/http/http_network_session.h"
Nit: list this header before "net/http/http_network_transaction.h".

http://codereview.chromium.org/6402002/diff/11001/net/spdy/spdy_test_util.cc#...
net/spdy/spdy_test_util.cc:941: NULL));
Nit: add a /* net_log */ comment after this NULL?

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
net/url_request/url_request_test_util.cc:12: #include
"net/spdy/spdy_session_pool.h"
IMPORTANT: if we consider this file as sample code of using
URLRequest, then the need to include these three headers
shows that the implementation details of the network stack
are exposed.  This is bad.  Did I misunderstand this?

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
net/url_request/url_request_test_util.cc:128: NULL),
Nit: add a /* net_log * comment after this NULL?

[willchan no longer on Chromium, 2011-01-28 06:12:07]

I've addressed your two main points here. Since you gave the LGTM, I'm going to
go ahead and commit it tonight after I fix the nits. If there's anything you
disagree with still, I can discuss it with you in MTV tomorrow and I'll address
any potential issues in my followup changelist where I will replace most of
HttpNetworkSession's parameters with a Params struct.

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6402002/diff/11001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:494: net::HttpNetworkSession*
network_session = main_cache->GetSession();
On 2011/01/28 01:51:23, wtc wrote:
> IMPORTANT: Why do we not need the null check for 'main_cache'
> in the original code?

I will doublecheck with eroman tomorrow to make sure, but I'm pretty sure that
the main ChromeURLRequestContext is *ALWAYS* a HttpCache, so GetCache() will
always return it. I think the old code was probably back from a time when it
wasn't always guaranteed to be an HttpCache. In any case, all tests pass with
this, so I *think* it works. If we find it doesn't, I'll add it back and add a
test for it :)

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
net/url_request/url_request_test_util.cc:12: #include
"net/spdy/spdy_session_pool.h"
On 2011/01/28 01:51:23, wtc wrote:
> IMPORTANT: if we consider this file as sample code of using
> URLRequest, then the need to include these three headers
> shows that the implementation details of the network stack
> are exposed.  This is bad.  Did I misunderstand this?

That's a very interesting point! I *think* it's OK. I think we actually do need
to expose this information during construction in the future, because we'll want
to expose more and more ways to configure portions of the network stack. For
example, we'll want to pass in ways to configure the number of connections per
host for the socket pools.

I think exposing this during construction of HttpCache is OK. It's pretty
standard for dependency injection techniques to do this. What's *important* is
that we don't let the majority of the code access the internals, by restricting
the accessors/setters of URLRequestContext and HttpTransactionFactory. If you
look at HttpTransactionFactory
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/net/h...),
it's actually being evil by exposing the HttpCache and HttpNetworkSession at
runtime. I plan to get rid of those accessors. I think that using this
technique, I will be able to reduce the number of members in URLRequestContext,
because they're actually only used in HttpNetworkSession. Therefore, I hope to
hide more implementation details in the future.

[wtc, 2011-01-28 18:19:48]

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/6402002/diff/11001/net/url_request/url_request...
net/url_request/url_request_test_util.cc:12: #include
"net/spdy/spdy_session_pool.h"
When I reviewed this CL, I first reviewed the "net" files,
and then the "chrome" files.  So this was the first file where
I saw these three headers, but later I saw these three headers
added to some of the "chrome" files, too.

What stands out the most is spdy_session_pool.h, followed
by client_socket_factory.h.  I am complaining about not
only exposing implementation details, but also imposing
a burden on network stack users.  A user of the network stack
should not need to write the same "boilerplate" code to use
the default client socket factory or create a SPDY session
pool.

I look forward to more cleanup patches from you.  Thank you
for working on this!