[Cronet] Set up HttpServerPropertiesManager

net/url_request/url_request_context_builder.h

Index: net/url_request/url_request_context_builder.h
diff --git a/net/url_request/url_request_context_builder.h b/net/url_request/url_request_context_builder.h
index a104c4386550a0d43363b6d9f159c48c4866ffb6..d21e53b1f2a6de561e47e3a35add340560b893d6 100644
--- a/net/url_request/url_request_context_builder.h
+++ b/net/url_request/url_request_context_builder.h
@@ -22,6 +22,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
 #include "build/build_config.h"
 #include "net/base/net_export.h"
 #include "net/base/network_delegate.h"
@@ -42,6 +43,7 @@ class CookieStore;
 class FtpTransactionFactory;
 class HostMappingRules;
 class HttpAuthHandlerFactory;
+class HttpServerProperties;
 class ProxyConfigService;
 class URLRequestContext;
 class URLRequestInterceptor;
@@ -207,6 +209,11 @@ class NET_EXPORT URLRequestContextBuilder {
   // SdchOwner in net/sdch/sdch_owner.h is a simple policy object.
   void set_sdch_enabled(bool enable) { sdch_enabled_ = enable; }
 
+  void set_http_server_properties(
+      const base::WeakPtr<HttpServerProperties>& http_server_properties) {

[pauljensen, 2015/06/15 15:57:42]

I'm torn as to whether this should be a scoped_ptr or a WeakPtr. 
URLRequestContext takes a WeakPtr but URLRequestContextStorage takes a
scoped_ptr.  Using a WeakPtr promotes sharing the HttpServerProperties (which is
good for performance) but it forces callers to take ownership (which is added
complexity that a lot of URLRequestContextBuilder clients probably don't want to
entertain).  I'm tempted to go with a scoped_ptr and have
URLRequestContextBuilder take ownership, as it does for set_host_resolver()
which is another preferably share resource.  I guess this depends on how
PrefService is supposed to be used (i.e. one versus man instantiations).

On a side note, this class desperately needs a useful comment, especially
because we encourage its use.  It's frankly terrifying that so many of its
setters take pointers...but take ownership.  Then Build() transfers ownership to
the URLRequestContext, so calling Build() twice will have different results
(i.e. the first generated context get's the owned item, but the second gets
none/default).

[xunjieli, 2015/06/15 18:41:36]

I also prefer that storage takes ownership of the HttpServerPropertiesManager,
but
HttpServerPropertiesManager has this requirement that it needs to be explicitly
shut down on the pref thread
(See HttpServerPropertiesManager::ShutdownOnPrefThread, which the interface
class does not have). And In Cronet, there isn't a pref thread, we just use the
network thread as the pref thread for scheduling updates. This forces me to use
WeakPtr instead to manage the initialization and shutdown of the
HttpServerPropertiesManager instance explicitly. Not sure if there's a work
around.
Agreed.

[xunjieli, 2015/06/15 18:49:22]

I guess if we make storage to take ownership of the HttpServerPropertiesManager.
During shutdown, we can get HttpServerProperties from the URLRequestContext and
do a cast to see if it is an instance of HttpServerPropertiesManager? If yes, we
will invoke ShutDownOnPrefThread?

+    http_server_properties_ = http_server_properties;
+  }
+
   URLRequestContext* Build();
 
  private:
@@ -248,6 +255,7 @@ class NET_EXPORT URLRequestContextBuilder {
   scoped_ptr<FtpTransactionFactory> ftp_transaction_factory_;
   std::vector<SchemeFactory> extra_http_auth_handlers_;
   ScopedVector<URLRequestInterceptor> url_request_interceptors_;
+  base::WeakPtr<HttpServerProperties> http_server_properties_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestContextBuilder);
 };