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

net/base/sdch_manager.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Provides global database of differential decompression dictionaries for the
 // SDCH filter (processes sdch enconded content).
 
 // Exactly one instance of SdchManager is built, and all references are made
 // into that collection.
 //
 // The SdchManager maintains a collection of memory resident dictionaries. It
 // can find a dictionary (based on a server specification of a hash), store a
 // dictionary, and make judgements about what URLs can use, set, etc. a
 // dictionary.
 
 // These dictionaries are acquired over the net, and include a header
 // (containing metadata) as well as a VCDIFF dictionary (for use by a VCDIFF
 // module) to decompress data.
 
 #ifndef NET_BASE_SDCH_MANAGER_H_
 #define NET_BASE_SDCH_MANAGER_H_
 
 #include <map>
 #include <set>
 #include <string>
 
 #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"
 #include "url/gurl.h"
 
 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.
   enum ProblemCodes {
     MIN_PROBLEM_CODE,
 
     // Content-encoding correction problems.
     ADDED_CONTENT_ENCODING = 1,
     FIXED_CONTENT_ENCODING = 2,
     FIXED_CONTENT_ENCODINGS = 3,
@@ skipping 166 matching lines @@
 
   SdchManager();
   virtual ~SdchManager();
 
   // Clear data (for browser data removal).
   void ClearData();
 
   // 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);
 
   static bool sdch_enabled() { return g_sdch_enabled_; }
 
   // Enables or disables SDCH compression over secure connection.
   static void EnableSecureSchemeSupport(bool enabled);
 
   static bool secure_scheme_supported() { return g_secure_scheme_supported_; }
 
@@ skipping 22 matching lines @@
 
   // Unit test only: Indicate what current blacklist increment is for a domain.
   int BlacklistDomainExponential(const std::string& domain);
 
   // Check to see if SDCH is enabled (globally), and the given URL is in a
   // supported domain (i.e., not blacklisted, and either the specific supported
   // domain, or all domains were assumed supported). If it is blacklist, reduce
   // 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|
   // to use to decompreses data that arrived as SDCH encoded content. Check to
   // be sure the returned |dictionary| can be used for decoding content supplied
   // in response to a request for |referring_url|.
   // Return null in |dictionary| if there is no matching legal dictionary.
   void GetVcdiffDictionary(const std::string& server_hash,
                            const GURL& referring_url,
                            scoped_refptr<Dictionary>* dictionary);
@@ skipping 10 matching lines @@
                            std::string* client_hash, std::string* server_hash);
 
   // For Latency testing only, we need to know if we've succeeded in doing a
   // round trip before starting our comparative tests. If ever we encounter
   // problems with SDCH, we opt-out of the test unless/until we perform a
   // complete SDCH decoding.
   bool AllowLatencyExperiment(const GURL& url) const;
 
   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 {
     BlacklistInfo()
         : count(0),
           exponential_count(0),
           reason(MIN_PROBLEM_CODE) {}
 
     int count;                   // # of times to refuse SDCH advertisement.
     int exponential_count;       // Current exponential backoff ratchet.
     ProblemCodes reason;         // Why domain was blacklisted.
 
   };
   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;
 
   // Support SDCH compression, by advertising in headers.
   static bool g_sdch_enabled_;
 
   // Support SDCH compression for HTTPS requests and responses. When supported,
   // HTTPS applicable dictionaries MUST have been acquired securely via HTTPS.
   static bool g_secure_scheme_supported_;
 
   // A simple implementation of a RFC 3548 "URL safe" base64 encoder.
   static void UrlSafeBase64Encode(const std::string& input,
                                   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_;
 
   // List of hostnames for which a latency experiment is allowed (because a
   // 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);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_SDCH_MANAGER_H_