Restructure SDCH layering to allow more separation (observer/1->[0,n]

net/base/sdch_manager.h

Index: net/base/sdch_manager.h
diff --git a/net/base/sdch_manager.h b/net/base/sdch_manager.h
index 7d2c103defa1af1832492ba53f0fb33c4d88141b..54f34cec4730310cb1869b8305416305ca1cea1a 100644
--- a/net/base/sdch_manager.h
+++ b/net/base/sdch_manager.h
@@ -27,6 +27,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
@@ -34,46 +35,13 @@
 
 namespace net {
 
-//------------------------------------------------------------------------------
-// Create a public interface to help us load SDCH dictionaries.
-// The SdchManager class allows registration to support this interface.
-// A browser may register a fetcher that is used by the dictionary managers to
-// get data from a specified URL. This allows us to use very high level browser
-// functionality in this base (when the functionality can be provided).
-class NET_EXPORT SdchFetcher {
- public:
-  class NET_EXPORT Delegate {
-   public:
-    virtual ~Delegate() {}
-
-    // Called whenever the SdchFetcher has successfully retrieved a
-    // dictionary.  |dictionary_text| contains the body of the dictionary
-    // retrieved from |dictionary_url|.
-    virtual void AddSdchDictionary(const std::string& dictionary_text,
-                                   const GURL& dictionary_url) = 0;
-  };
-
-  SdchFetcher() {}
-  virtual ~SdchFetcher() {}
-
-  // The Schedule() method is called when there is a need to get a dictionary
-  // from a server. The callee is responsible for getting that dictionary_text,
-  // and then calling back to AddSdchDictionary() in the Delegate instance.
-  virtual void Schedule(const GURL& dictionary_url) = 0;
-
-  // The Cancel() method is called to cancel all pending dictionary fetches.
-  // This is used for implementation of ClearData() below.
-  virtual void Cancel() = 0;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SdchFetcher);
-};
+class SdchObserver;
+class URLRequest;
 
 //------------------------------------------------------------------------------
 
 class NET_EXPORT SdchManager
-    : public SdchFetcher::Delegate,
-      public NON_EXPORTED_BASE(base::NonThreadSafe) {
+    : public NON_EXPORTED_BASE(base::NonThreadSafe) {

[Ryan Sleevi, 2014/10/20 22:42:04]

Any chance we can remove this inheritance as part of the cleanup? :D

[Randy Smith (Not in Mondays), 2014/10/21 19:05:36]

I'm happy to, but I want to understand the rationale.   I would think assertions
that we're operating on a single thread would be good, and we can't use
content::BrowserThread::CurrentlyOn() in net/?

[Ryan Sleevi, 2014/10/21 23:09:29]

Because generally trait-inheritance requires concrete inheritence, which becomes
a code smell.

This is where having a ThreadChecker member over exposing NonThreadSafe is
useful here.

(Having classes inheriting from NonThreadSafe caused issues with RefCounted
cleanups, because of how NonThreadSafe exposed its dtors. Multiple inheritance
messiness)

[Randy Smith (Not in Mondays), 2014/10/23 20:57:07]

Done.

  public:
   // A list of errors that appeared and were either resolved, or used to turn
   // off sdch encoding.
@@ -260,9 +228,6 @@ class NET_EXPORT SdchManager
   // Record stats on various errors.
   static void SdchErrorRecovery(ProblemCodes problem);
 
-  // Register a fetcher that this class can use to obtain dictionaries.
-  void set_sdch_fetcher(scoped_ptr<SdchFetcher> fetcher);
-
   // Enables or disables SDCH compression.
   static void EnableSdchSupport(bool enabled);
 
@@ -305,16 +270,9 @@ class NET_EXPORT SdchManager
   // by 1 the number of times it will be reported as blacklisted.
   bool IsInSupportedDomain(const GURL& url);
 
-  // Schedule the URL fetching to load a dictionary. This will always return
-  // before the dictionary is actually loaded and added.
-  // After the implied task does completes, the dictionary will have been
-  // cached in memory.
-  void FetchDictionary(const GURL& request_url, const GURL& dictionary_url);
-
-  // Security test function used before initiating a FetchDictionary.
-  // Return true if fetch is legal.
-  bool CanFetchDictionary(const GURL& referring_url,
-                          const GURL& dictionary_url) const;
+  // Send out appropriate events notifying observers that a Get-Dictionary
+  // header has been seen.
+  void OnGetDictionary(const GURL& request_url, const GURL& dictionary_url);
 
   // Find the vcdiff dictionary (the body of the sdch dictionary that appears
   // after the meta-data headers like Domain:...) with the given |server_hash|
@@ -345,18 +303,16 @@ class NET_EXPORT SdchManager
 
   void SetAllowLatencyExperiment(const GURL& url, bool enable);
 
-  int GetFetchesCountForTesting() const {
-    return fetches_count_for_testing_;
-  }
-
-  // Implementation of SdchFetcher::Delegate.
+  // Register for events generated by the SDCH subsystem.
+  virtual void AddObserver(SdchObserver* observer);
+  virtual void RemoveObserver(SdchObserver* observer);
 
   // Add an SDCH dictionary to our list of availible
   // dictionaries. This addition will fail if addition is illegal
   // (data in the dictionary is not acceptable from the
   // dictionary_url; dictionary already added, etc.).
   virtual void AddSdchDictionary(const std::string& dictionary_text,
-                                 const GURL& dictionary_url) OVERRIDE;
+                                 const GURL& dictionary_url);
 
  private:
   struct BlacklistInfo {
@@ -373,6 +329,11 @@ class NET_EXPORT SdchManager
   typedef std::map<std::string, BlacklistInfo> DomainBlacklistInfo;
   typedef std::set<std::string> ExperimentSet;
 
+  // Security test function used before initiating a FetchDictionary.
+  // Return true if fetch is legal.

[Ryan Sleevi, 2014/10/20 22:42:04]

Documentation needs more. I have trouble understanding what this is doing just
from reading the header.

[Randy Smith (Not in Mondays), 2014/10/21 19:05:36]

Done, but note that the change in this CL is making this function private when
before it was public.  So I might argue that no comment is needed, as it's no
longer part of the interface.

+  bool CanFetchDictionary(const GURL& referring_url,
+                          const GURL& dictionary_url) const;
+
   // A map of dictionaries info indexed by the hash that the server provides.
   typedef std::map<std::string, scoped_refptr<Dictionary> > DictionaryMap;
 
@@ -388,9 +349,6 @@ class NET_EXPORT SdchManager
                                   std::string* output);
   DictionaryMap dictionaries_;
 
-  // An instance that can fetch a dictionary given a URL.
-  scoped_ptr<SdchFetcher> fetcher_;
-
   // List domains where decode failures have required disabling sdch.
   DomainBlacklistInfo blacklisted_domains_;
 
@@ -398,7 +356,11 @@ class NET_EXPORT SdchManager
   // round trip test has recently passed).
   ExperimentSet allow_latency_experiment_;
 
-  int fetches_count_for_testing_;
+  // Observers that want to be notified of SDCH events.
+  // Assert list is empty on destruction since if there is an observer
+  // that hasn't removed itself from the list, that observer probably
+  // has a reference to the SdchManager.
+  ObserverList<SdchObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(SdchManager);
 };