Sdch view for net-internals

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 6f2ea5af6de0a637e70a395164fe5f80fdd09af3..9cf30e9bd5a18d70640684e37de548bc1b068d47 100644
--- a/net/base/sdch_manager.h
+++ b/net/base/sdch_manager.h
@@ -23,6 +23,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
@@ -30,8 +31,14 @@
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
+#include "net/base/net_log.h"
 #include "url/gurl.h"
 
+namespace base {
+class Value;
+class DictionaryValue;
+}
+
 namespace net {
 
 //------------------------------------------------------------------------------
@@ -65,98 +72,11 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   // 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,
-
-    // Content decoding errors.
-    DECODE_HEADER_ERROR = 4,
-    DECODE_BODY_ERROR = 5,
-
-    // More content-encoding correction problems.
-    OPTIONAL_GUNZIP_ENCODING_ADDED = 6,
-
-    // Content encoding correction when we're not even tagged as HTML!?!
-    BINARY_ADDED_CONTENT_ENCODING = 7,
-    BINARY_FIXED_CONTENT_ENCODING = 8,
-    BINARY_FIXED_CONTENT_ENCODINGS = 9,
-
-    // Dictionary selection for use problems.
-    DICTIONARY_FOUND_HAS_WRONG_DOMAIN = 10,
-    DICTIONARY_FOUND_HAS_WRONG_PORT_LIST = 11,
-    DICTIONARY_FOUND_HAS_WRONG_PATH = 12,
-    DICTIONARY_FOUND_HAS_WRONG_SCHEME = 13,
-    DICTIONARY_HASH_NOT_FOUND = 14,
-    DICTIONARY_HASH_MALFORMED = 15,
-
-    // Dictionary saving problems.
-    DICTIONARY_HAS_NO_HEADER = 20,
-    DICTIONARY_HEADER_LINE_MISSING_COLON = 21,
-    DICTIONARY_MISSING_DOMAIN_SPECIFIER = 22,
-    DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN = 23,
-    DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL = 24,
-    DICTIONARY_PORT_NOT_MATCHING_SOURCE_URL = 25,
-    DICTIONARY_HAS_NO_TEXT = 26,
-    DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX = 27,
-
-    // Dictionary loading problems.
-    DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST = 30,
-    DICTIONARY_SELECTED_FOR_SSL = 31,
-    DICTIONARY_ALREADY_LOADED = 32,
-    DICTIONARY_SELECTED_FROM_NON_HTTP = 33,
-    DICTIONARY_IS_TOO_LARGE= 34,
-    DICTIONARY_COUNT_EXCEEDED = 35,
-    DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD = 36,
-    DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD = 37,
-
-    // Failsafe hack.
-    ATTEMPT_TO_DECODE_NON_HTTP_DATA = 40,
-
-
-    // Content-Encoding problems detected, with no action taken.
-    MULTIENCODING_FOR_NON_SDCH_REQUEST = 50,
-    SDCH_CONTENT_ENCODE_FOR_NON_SDCH_REQUEST = 51,
-
-    // Dictionary manager issues.
-    DOMAIN_BLACKLIST_INCLUDES_TARGET = 61,
-
-    // Problematic decode recovery methods.
-    META_REFRESH_RECOVERY = 70,            // Dictionary not found.
-    // defunct =  71, // Almost the same as META_REFRESH_UNSUPPORTED.
-    // defunct = 72,  // Almost the same as CACHED_META_REFRESH_UNSUPPORTED.
-    // defunct = 73,  // PASSING_THROUGH_NON_SDCH plus DISCARD_TENTATIVE_SDCH.
-    META_REFRESH_UNSUPPORTED = 74,         // Unrecoverable error.
-    CACHED_META_REFRESH_UNSUPPORTED = 75,  // As above, but pulled from cache.
-    PASSING_THROUGH_NON_SDCH = 76,  // Tagged sdch but missing dictionary-hash.
-    INCOMPLETE_SDCH_CONTENT = 77,   // Last window was not completely decoded.
-    PASS_THROUGH_404_CODE = 78,     // URL not found message passing through.
-
-    // This next report is very common, and not really an error scenario, but
-    // it exercises the error recovery logic.
-    PASS_THROUGH_OLD_CACHED = 79,   // Back button got pre-SDCH cached content.
-
-    // Common decoded recovery methods.
-    META_REFRESH_CACHED_RECOVERY = 80,  // Probably startup tab loading.
-    DISCARD_TENTATIVE_SDCH = 81,        // Server decided not to use sdch.
-
-    // Non SDCH problems, only accounted for to make stat counting complete
-    // (i.e., be able to be sure all dictionary advertisements are accounted
-    // for).
-
-    UNFLUSHED_CONTENT = 90,    // Possible error in filter chaining.
-    // defunct = 91,           // MISSING_TIME_STATS (Should never happen.)
-    CACHE_DECODED = 92,        // No timing stats recorded.
-    // defunct = 93,           // OVER_10_MINUTES (No timing stats recorded.)
-    UNINITIALIZED = 94,        // Filter never even got initialized.
-    PRIOR_TO_DICTIONARY = 95,  // We hadn't even parsed a dictionary selector.
-    DECODE_ERROR = 96,         // Something went wrong during decode.
-
-    // Problem during the latency test.
-    LATENCY_TEST_DISALLOWED = 100,  // SDCH now failing, but it worked before!
+    PROBLEM_CODE_OK = 0,
 
+#define SDCH_PROBLEM_CODE(label, value) label = value,
+#include "net/base/sdch_problem_code_list.h"
+#undef SDCH_PROBLEM_CODE
     MAX_PROBLEM_CODE  // Used to bound histogram.
   };
 
@@ -165,6 +85,16 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   static const size_t kMaxDictionarySize;
   static const size_t kMaxDictionaryCount;
 
+  // Result if adding the downloaded dictionary. |added| will be true, if the
+  // dictionary was valid and successfully added. Otherwise, |added| will be
+  // false and |problem| will contain the error. |added| my be false even when
+  // no error occurs (SDCH or SDCH-over-https may be disabled).

[Randy Smith (Not in Mondays), 2014/08/05 17:50:34]

I would think we'd want to add errors in this case.  The reason for returning an
AddResult structure is that we get context for why a failure occurred, so there
being cases where we don't get that context is unfortunate.

This opens a (relatively small) can of worms in that ProblemCodes is a list of
values that go into a histogram, and if you change that it's messy (you need to
rev the histograms that use it).  Possibly we should create a new enum for the
possible return values from this routine.  

[baranovich, 2014/08/12 20:54:50]

Done.

+  struct AddResult {

[Randy Smith (Not in Mondays), 2014/08/05 17:50:34]

I don't think there's any need to make this a structure, at least if you go with
my comment above--just return the problem code and make it ..._OK if the
dictionary was added.  If you don't end up going that route, I think it's more
in line with chromium coding conventions to return a boolean and take a pointer
to a ProblemCode to give more detail.

[baranovich, 2014/08/12 20:54:50]

Done. Decided to go with bool + out param

+    AddResult();
+    bool added;
+    ProblemCodes problem;
+  };
+
   // There is one instance of |Dictionary| for each memory-cached SDCH
   // dictionary.
   class NET_EXPORT_PRIVATE Dictionary : public base::RefCounted<Dictionary> {
@@ -195,16 +125,18 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
 
     // Security method to check if we can advertise this dictionary for use
     // if the |target_url| returns SDCH compressed data.
-    bool CanAdvertise(const GURL& target_url);
+    bool CanAdvertise(const GURL& target_url) const;
 
     // Security methods to check if we can establish a new dictionary with the
     // given data, that arrived in response to get of dictionary_url.
-    static bool CanSet(const std::string& domain, const std::string& path,
-                       const std::set<int>& ports, const GURL& dictionary_url);
+    static ProblemCodes CanSet(const std::string& domain,
+                               const std::string& path,
+                               const std::set<int>& ports,
+                               const GURL& dictionary_url);
 
     // Security method to check if we can use a dictionary to decompress a
     // target that arrived with a reference to this dictionary.
-    bool CanUse(const GURL& referring_url);
+    ProblemCodes CanUse(const GURL& referring_url) const;
 
     // Compare paths to see if they "match" for dictionary use.
     static bool PathMatch(const std::string& path,
@@ -213,6 +145,7 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
     // Compare domains to see if the "match" for dictionary use.
     static bool DomainMatch(const GURL& url, const std::string& restriction);
 
+    base::DictionaryValue* DictionaryInfoToValue() const;

[Randy Smith (Not in Mondays), 2014/08/05 17:50:34]

I think it's reasonable to make all of the information needed by this function
part of Dictionary's public interface, and I'd rather have it there than make
Dictionary construct a json value purely for net-internals (see my note below to
Matt about SdchInfoToValue()).

[baranovich, 2014/08/12 20:54:50]

Done. Moved this to SdchInfoToValue()

 
     // The actual text of the dictionary.
     std::string text_;
@@ -294,18 +227,20 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   // 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);
+  ProblemCodes 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;
+  // Return PROBLEM_CODE_OK if fetch is legal.
+  ProblemCodes CanFetchDictionary(const GURL& referring_url,
+                                  const GURL& dictionary_url) const;
 
   // Add an SDCH dictionary to our list of availible dictionaries. This addition
-  // will fail (return false) if addition is illegal (data in the dictionary is
-  // not acceptable from the dictionary_url; dictionary already added, etc.).
-  bool AddSdchDictionary(const std::string& dictionary_text,
-                         const GURL& dictionary_url);
+  // will fail (AddResult::added will be false) if addition is illegal (data in
+  // the dictionary is not acceptable from the dictionary_url; dictionary
+  // already added, etc.).
+  AddResult AddSdchDictionary(const std::string& dictionary_text,
+                              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|
@@ -313,9 +248,12 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
   // 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);
+  // Returns PROBLEM_CODE_OK if dictionary is not found, SDCH(-over-https) is
+  // disabled, or if matching legal dictionary exists. Otherwise returns the
+  // corresponding problem code.
+  ProblemCodes GetVcdiffDictionary(const std::string& server_hash,
+                                   const GURL& referring_url,
+                                   scoped_refptr<Dictionary>* dictionary);
 
   // Get list of available (pre-cached) dictionaries that we have already loaded
   // into memory.  The list is a comma separated list of (client) hashes per
@@ -336,7 +274,13 @@ class NET_EXPORT SdchManager : public NON_EXPORTED_BASE(base::NonThreadSafe) {
 
   void SetAllowLatencyExperiment(const GURL& url, bool enable);
 
+  base::Value* SdchInfoToValue() const;

[Randy Smith (Not in Mondays), 2014/08/05 17:50:34]

I keep wincing at the existence of this function; to my mind, the SdchManager
shouldn't know about consing up its internal state for the net-internals
debugging functions, and the interface shouldn't have methods purely for that
purpose.  I'd rather have the net internals system reach in for the data and
construct the json itself.  

When I look at what data isn't public, I get:
* The dictionary list.  I'd be good with GetAvailDictionaryList returning a list
of dictionary pointers (with strong constraints on how long they're valid for).
* blacklisted domains.  I don't see a way around breaking the interface for
this, so I'd suggest putting an interface to get the blacklisted domains, with
details (maybe with a ForDebugging suffix to make it clear this isn't intended
for ordinary use) into the SdchManager interface.

[mmenke, 2014/08/05 18:55:17]

This is how things are often done, currently, when you want to log non-public
data.  I'm a bit reluctant to make more data public, since that encourages
tighter coupling, API bloat, and misuse/abuse of functions really intended only
for logging.  Also, why should the caller know or care about the structure of
the internal state of the SdchManager?  Of course, net-internals still needs
dependencies on the data in the dictionary.

+
  private:
+  bool IsInBlacklistedDomain(const GURL& url);
+
+  static bool IsSdchEnabledForUrl(const GURL& url);
+
   typedef std::map<std::string, int> DomainCounter;
   typedef std::set<std::string> ExperimentSet;