Move http_server_properties_manager from chrome/browser/net to net/http.

net/http/http_server_properties_manager.h

Index: net/http/http_server_properties_manager.h
diff --git a/chrome/browser/net/http_server_properties_manager.h b/net/http/http_server_properties_manager.h
similarity index 62%
rename from chrome/browser/net/http_server_properties_manager.h
rename to net/http/http_server_properties_manager.h
index 71de395dc514cce6c8fa2789d795c25cef890449..6d1ffcd80e8c63d90405c9bd27430c4321cd859e 100644
--- a/chrome/browser/net/http_server_properties_manager.h
+++ b/net/http/http_server_properties_manager.h
@@ -20,147 +20,142 @@
 
 class PrefService;
 
-namespace user_prefs {
-class PrefRegistrySyncable;
+namespace base {
+class SingleThreadTaskRunner;

[Ryan Sleevi, 2014/07/08 18:19:26]

Do you really need an STTR?

A SequencedTaskRunner seems sufficient, in that you aren't binding to any
thread-local storage. The actual need to bind to a particular thread should be
minimal.

[mef, 2014/07/09 12:12:04]

Done.

 }
 
-namespace chrome_browser_net {
+namespace net {
 
 ////////////////////////////////////////////////////////////////////////////////
-//  HttpServerPropertiesManager
+// HttpServerPropertiesManager
 
 // The manager for creating and updating an HttpServerProperties (for example it
 // tracks if a server supports SPDY or not).
 //
-// This class interacts with both the UI thread, where notifications of pref
-// changes are received from, and the IO thread, which owns it (in the
+// This class interacts with both the pref thread, where notifications of pref
+// changes are received from, and the IO thread, which owns it (e.g. in the

[Ryan Sleevi, 2014/07/08 18:19:26]

nit: usually called network task runner.

 // ProfileIOData) and it persists the changes from network stack that if a

[Ryan Sleevi, 2014/07/08 18:19:26]

ProfileIOData = abstraction leak

[mef, 2014/07/09 12:12:04]

Done.

 // server supports SPDY or not.
 //
-// It must be constructed on the UI thread, to set up |ui_method_factory_| and
+// It must be constructed on the pref thread, to set up |pref_task_runner_| and
 // the prefs listeners.
 //
-// ShutdownOnUIThread must be called from UI before destruction, to release
-// the prefs listeners on the UI thread. This is done from ProfileIOData.
-//
-// Update tasks from the UI thread can post safely to the IO thread, since the
-// destruction order of Profile and ProfileIOData guarantees that if this
-// exists in UI, then a potential destruction on IO will come after any task
-// posted to IO from that method on UI. This is used to go through IO before
-// the actual update starts, and grab a WeakPtr.
-class HttpServerPropertiesManager
-    : public net::HttpServerProperties {
+// ShutdownOnPrefThread must be called from pref thread before destruction, to
+// release the prefs listeners on the pref thread.
+
+// Class expects that update tasks from the Pref thread can post safely to the
+// IO thread, since the destruction order guarantees that if this
+// exists in pref, then a potential destruction on IO will come after any task
+// posted to IO from that method on pref thread. This is used to go through IO
+// before  the actual update starts, and grab a WeakPtr.

[Ryan Sleevi, 2014/07/08 18:19:26]

1) extra space in "before  the"

2) This destruction order is no longer guaranteed in //net. It needs to be
spelled out as a requirement, not as an assumption

[mef, 2014/07/09 12:12:04]

Done.

+class NET_EXPORT HttpServerPropertiesManager : public HttpServerProperties {
  public:
   // Create an instance of the HttpServerPropertiesManager. The lifetime of the
   // PrefService objects must be longer than that of the
-  // HttpServerPropertiesManager object. Must be constructed on the UI thread.
-  explicit HttpServerPropertiesManager(PrefService* pref_service);
+  // HttpServerPropertiesManager object. Must be constructed on the Pref thread.
+  HttpServerPropertiesManager(
+      PrefService* pref_service,
+      const char* pref_path,
+      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
   virtual ~HttpServerPropertiesManager();
 
-  // Initialize |http_server_properties_impl_| and |io_method_factory_| on IO
-  // thread. It also posts a task to UI thread to get SPDY Server preferences
+  // Initialize |http_server_properties_impl_| and |io_task_runner_| on IO
+  // thread. It also posts a task to Pref thread to get SPDY Server preferences
   // from |pref_service_|.

[Ryan Sleevi, 2014/07/08 18:19:26]

This last sentence seems to unnecessarily describe implementation detail.

[mef, 2014/07/09 12:12:04]

Done.

   void InitializeOnIOThread();
 
-  // Prepare for shutdown. Must be called on the UI thread, before destruction.
-  void ShutdownOnUIThread();
-
-  // Register |prefs| for properties managed here.
-  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+  // Prepare for shutdown. Must be called on the Pref thread before destruction.
+  void ShutdownOnPrefThread();

[Ryan Sleevi, 2014/07/08 18:19:26]

And what happens if they don't?

[Bernhard Bauer, 2014/07/08 21:47:21]

A DCHECK will trigger when this object (with its UI-thread-living members) is
destroyed on another thread. Is there a better way to describe such a contract
of a class in a comment?

[Ryan Sleevi, 2014/07/08 22:54:35]

Well, this is more of a "long term design" issue, and not a refactoring that I
think is essential to moving this (but one I do think is essential for code
health), but an 'ideal' world is that the HSPM lives on a single thread - the
Pref thread - and that there's no need for an explicit Shutdown() state, as C++
provides that (via the DTOR).

Since HSP has to live on the IO thread, it would be a separate class, and the
HSPM and the HSP(Prefs) could communicate via internal, implementation-detail
methods.

rch@ has a much better idea of this vision.

My goal here with this comment is to do our best, in the time until things are
fixed, to make sure there's no security issues and we catch developers as
quickly as possible. DCHECK() is probably fine, or any other sanity/safety
things. a CHECK might even be more appropriate, depending - I'm not familiar
enough to know.

[mef, 2014/07/09 12:12:04]

Ack. I'm fine with abandoning this CL in favor or rch's changes if they go to
net.

 
   // Helper function for unit tests to set the version in the dictionary.
   static void SetVersion(base::DictionaryValue* http_server_properties_dict,
                          int version_number);
 
   // Deletes all data. Works asynchronously, but if a |completion| callback is
-  // provided, it will be fired on the UI thread when everything is done.
+  // provided, it will be fired on the pref thread when everything is done.
   void Clear(const base::Closure& completion);
 
   // ----------------------------------
-  // net::HttpServerProperties methods:
+  // HttpServerProperties methods:
   // ----------------------------------
 
   // Gets a weak pointer for this object.
-  virtual base::WeakPtr<net::HttpServerProperties> GetWeakPtr() OVERRIDE;
+  virtual base::WeakPtr<HttpServerProperties> GetWeakPtr() OVERRIDE;

[Ryan Sleevi, 2014/07/08 18:19:26]

Hrm. Long-term, I think we will want to remove this method as a WeakPtr smell,
but I'm not going to fight for it now.

[mef, 2014/07/09 12:12:04]

Roger that.

 
   // Deletes all data. Works asynchronously.
   virtual void Clear() OVERRIDE;
 
   // Returns true if |server| supports SPDY. Should only be called from IO
   // thread.
-  virtual bool SupportsSpdy(const net::HostPortPair& server) OVERRIDE;
+  virtual bool SupportsSpdy(const HostPortPair& server) OVERRIDE;
 
   // Add |server| as the SPDY server which supports SPDY protocol into the
   // persisitent store. Should only be called from IO thread.
-  virtual void SetSupportsSpdy(const net::HostPortPair& server,
+  virtual void SetSupportsSpdy(const HostPortPair& server,
                                bool support_spdy) OVERRIDE;
 
   // Returns true if |server| has an Alternate-Protocol header.
-  virtual bool HasAlternateProtocol(const net::HostPortPair& server) OVERRIDE;
+  virtual bool HasAlternateProtocol(const HostPortPair& server) OVERRIDE;
 
   // Returns the Alternate-Protocol and port for |server|.
   // HasAlternateProtocol(server) must be true.
-  virtual net::PortAlternateProtocolPair GetAlternateProtocol(
-      const net::HostPortPair& server) OVERRIDE;
+  virtual PortAlternateProtocolPair GetAlternateProtocol(
+      const HostPortPair& server) OVERRIDE;
 
   // Sets the Alternate-Protocol for |server|.
   virtual void SetAlternateProtocol(
-      const net::HostPortPair& server,
+      const HostPortPair& server,
       uint16 alternate_port,
-      net::AlternateProtocol alternate_protocol) OVERRIDE;
+      AlternateProtocol alternate_protocol) OVERRIDE;
 
   // Sets the Alternate-Protocol for |server| to be BROKEN.
-  virtual void SetBrokenAlternateProtocol(
-      const net::HostPortPair& server) OVERRIDE;
+  virtual void SetBrokenAlternateProtocol(const HostPortPair& server) OVERRIDE;
 
   // Returns true if Alternate-Protocol for |server| was recently BROKEN.
   virtual bool WasAlternateProtocolRecentlyBroken(
-      const net::HostPortPair& server) OVERRIDE;
+      const HostPortPair& server) OVERRIDE;
 
   // Confirms that Alternate-Protocol for |server| is working.
-  virtual void ConfirmAlternateProtocol(
-      const net::HostPortPair& server) OVERRIDE;
+  virtual void ConfirmAlternateProtocol(const HostPortPair& server) OVERRIDE;
 
   // Clears the Alternate-Protocol for |server|.
-  virtual void ClearAlternateProtocol(const net::HostPortPair& server) OVERRIDE;
+  virtual void ClearAlternateProtocol(const HostPortPair& server) OVERRIDE;
 
   // Returns all Alternate-Protocol mappings.
-  virtual const net::AlternateProtocolMap&
-      alternate_protocol_map() const OVERRIDE;
+  virtual const AlternateProtocolMap& alternate_protocol_map() const OVERRIDE;
 
   virtual void SetAlternateProtocolExperiment(
-      net::AlternateProtocolExperiment experiment) OVERRIDE;
+      AlternateProtocolExperiment experiment) OVERRIDE;
 
-  virtual net::AlternateProtocolExperiment GetAlternateProtocolExperiment()
+  virtual AlternateProtocolExperiment GetAlternateProtocolExperiment()
       const OVERRIDE;
 
   // Gets a reference to the SettingsMap stored for a host.
   // If no settings are stored, returns an empty SettingsMap.
-  virtual const net::SettingsMap& GetSpdySettings(
-      const net::HostPortPair& host_port_pair) OVERRIDE;
+  virtual const SettingsMap& GetSpdySettings(
+      const HostPortPair& host_port_pair) OVERRIDE;
 
   // Saves an individual SPDY setting for a host. Returns true if SPDY setting
   // is to be persisted.
-  virtual bool SetSpdySetting(const net::HostPortPair& host_port_pair,
-                              net::SpdySettingsIds id,
-                              net::SpdySettingsFlags flags,
+  virtual bool SetSpdySetting(const HostPortPair& host_port_pair,
+                              SpdySettingsIds id,
+                              SpdySettingsFlags flags,
                               uint32 value) OVERRIDE;
 
   // Clears all SPDY settings for a host.
-  virtual void ClearSpdySettings(
-      const net::HostPortPair& host_port_pair) OVERRIDE;
+  virtual void ClearSpdySettings(const HostPortPair& host_port_pair) OVERRIDE;
 
   // Clears all SPDY settings for all hosts.
   virtual void ClearAllSpdySettings() OVERRIDE;
 
   // Returns all SPDY persistent settings.
-  virtual const net::SpdySettingsMap& spdy_settings_map() const OVERRIDE;
+  virtual const SpdySettingsMap& spdy_settings_map() const OVERRIDE;
 
-  virtual void SetServerNetworkStats(const net::HostPortPair& host_port_pair,
+  virtual void SetServerNetworkStats(const HostPortPair& host_port_pair,
                                      NetworkStats stats) OVERRIDE;
 
   virtual const NetworkStats* GetServerNetworkStats(
-      const net::HostPortPair& host_port_pair) const OVERRIDE;
+      const HostPortPair& host_port_pair) const OVERRIDE;
 
  protected:
   // --------------------
@@ -169,24 +164,24 @@ class HttpServerPropertiesManager
   // These are used to delay updating of the cached data in
   // |http_server_properties_impl_| while the preferences are changing, and
   // execute only one update per simultaneous prefs changes.
-  void ScheduleUpdateCacheOnUI();
+  void ScheduleUpdateCacheOnPref();
 
   // Starts the timers to update the cached prefs. This are overridden in tests
   // to prevent the delay.
-  virtual void StartCacheUpdateTimerOnUI(base::TimeDelta delay);
+  virtual void StartCacheUpdateTimerOnPref(base::TimeDelta delay);
 
   // Update cached prefs in |http_server_properties_impl_| with data from
   // preferences. It gets the data on UI thread and calls
   // UpdateSpdyServersFromPrefsOnIO() to perform the update on IO thread.
-  virtual void UpdateCacheFromPrefsOnUI();
+  virtual void UpdateCacheFromPrefsOnPref();
 
   // Starts the update of cached prefs in |http_server_properties_impl_| on the
   // IO thread. Protected for testing.
   void UpdateCacheFromPrefsOnIO(
       std::vector<std::string>* spdy_servers,
-      net::SpdySettingsMap* spdy_settings_map,
-      net::AlternateProtocolMap* alternate_protocol_map,
-      net::AlternateProtocolExperiment alternate_protocol_experiment,
+      SpdySettingsMap* spdy_settings_map,
+      AlternateProtocolMap* alternate_protocol_map,
+      AlternateProtocolExperiment alternate_protocol_experiment,
       bool detected_corrupted_prefs);
 
   // These are used to delay updating the preferences when cached data in
@@ -207,13 +202,12 @@ class HttpServerPropertiesManager
   // when finished. Virtual for testing.
   virtual void UpdatePrefsFromCacheOnIO(const base::Closure& completion);
 
-  // Update prefs::kHttpServerProperties preferences on UI thread. Executes an
+  // Update prefs::kHttpServerProperties preferences on pref thread. Executes an
   // optional |completion| callback when finished. Protected for testing.
-  void UpdatePrefsOnUI(
-      base::ListValue* spdy_server_list,
-      net::SpdySettingsMap* spdy_settings_map,
-      net::AlternateProtocolMap* alternate_protocol_map,
-      const base::Closure& completion);
+  void UpdatePrefsOnPref(base::ListValue* spdy_server_list,

[Ryan Sleevi, 2014/07/08 18:19:26]

nit: naming could be clearer (OnPref -> OnPrefThread)

[mef, 2014/07/09 12:12:04]

Done.

+                         SpdySettingsMap* spdy_settings_map,
+                         AlternateProtocolMap* alternate_protocol_map,
+                         const base::Closure& completion);
 
  private:
   void OnHttpServerPropertiesChanged();
@@ -222,25 +216,30 @@ class HttpServerPropertiesManager
   // UI thread

[Ryan Sleevi, 2014/07/08 18:19:26]

out of date comment

[mef, 2014/07/09 12:12:04]

Done.

   // ---------
 
+  const scoped_refptr<base::SingleThreadTaskRunner> pref_task_runner_;

[Ryan Sleevi, 2014/07/08 18:19:26]

STR seems fine here. Only requirement is it's the same task runner that
notifications for prefs come in. No special threading requirements here, just
sequencing.

[mef, 2014/07/09 12:12:04]

Done.

+
   // Used to get |weak_ptr_| to self on the UI thread.
   scoped_ptr<base::WeakPtrFactory<HttpServerPropertiesManager> >
-      ui_weak_ptr_factory_;
+      pref_weak_ptr_factory_;
 
-  base::WeakPtr<HttpServerPropertiesManager> ui_weak_ptr_;
+  base::WeakPtr<HttpServerPropertiesManager> pref_weak_ptr_;
 
   // Used to post cache update tasks.
   scoped_ptr<base::OneShotTimer<HttpServerPropertiesManager> >
-      ui_cache_update_timer_;
+      pref_cache_update_timer_;
 
   // Used to track the spdy servers changes.
   PrefChangeRegistrar pref_change_registrar_;
   PrefService* pref_service_;  // Weak.
   bool setting_prefs_;
+  const char* path_;
 
   // ---------
   // IO thread
   // ---------
 
+  const scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+
   // Used to get |weak_ptr_| to self on the IO thread.
   scoped_ptr<base::WeakPtrFactory<HttpServerPropertiesManager> >
       io_weak_ptr_factory_;
@@ -249,7 +248,7 @@ class HttpServerPropertiesManager
   scoped_ptr<base::OneShotTimer<HttpServerPropertiesManager> >
       io_prefs_update_timer_;
 
-  scoped_ptr<net::HttpServerPropertiesImpl> http_server_properties_impl_;
+  scoped_ptr<HttpServerPropertiesImpl> http_server_properties_impl_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpServerPropertiesManager);
 };