Simplify HttpCache/HttpNetworkLayer/HttpNetworkSession interaction.

net/url_request/url_request_test_util.cc

Index: net/url_request/url_request_test_util.cc
diff --git a/net/url_request/url_request_test_util.cc b/net/url_request/url_request_test_util.cc
index 099810adbfeb7739a18d99ac1e7639efa37d4a0a..5053f35ec7fcc498f4e307829195d602d704c346 100644
--- a/net/url_request/url_request_test_util.cc
+++ b/net/url_request/url_request_test_util.cc
@@ -7,6 +7,9 @@
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/threading/thread.h"
+#include "net/http/http_network_session.h"
+#include "net/socket/client_socket_factory.h"
+#include "net/spdy/spdy_session_pool.h"

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

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?

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

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]

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!

 
 TestCookiePolicy::TestCookiePolicy(int options_bit_mask)
     : ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
@@ -110,17 +113,19 @@ void TestURLRequestContext::Init() {
   http_auth_handler_factory_ = net::HttpAuthHandlerFactory::CreateDefault(
       host_resolver_);
   http_transaction_factory_ = new net::HttpCache(
-      net::HttpNetworkLayer::CreateFactory(host_resolver_,
-                                           cert_verifier_,
-                                           NULL /* dnsrr_resolver */,
-                                           NULL /* dns_cert_checker */,
-                                           NULL /* ssl_host_info_factory */,
-                                           proxy_service_,
-                                           ssl_config_service_,
-                                           http_auth_handler_factory_,
-                                           network_delegate_,
-                                           NULL),
-      NULL /* net_log */,
+      new net::HttpNetworkSession(
+          host_resolver_,
+          cert_verifier_,
+          NULL /* dnsrr_resolver */,
+          NULL /* dns_cert_checker */,
+          NULL /* ssl_host_info_factory */,
+          proxy_service_,
+          net::ClientSocketFactory::GetDefaultFactory(),
+          ssl_config_service_,
+          new net::SpdySessionPool(ssl_config_service_),
+          http_auth_handler_factory_,
+          network_delegate_,
+          NULL),

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

Nit: add a /* net_log * comment after this NULL?

       net::HttpCache::DefaultBackend::InMemory(0));
   // In-memory cookie store.
   cookie_store_ = new net::CookieMonster(NULL, NULL);