Sdch view for net-internals

net/base/sdch_manager.cc

Index: net/base/sdch_manager.cc
diff --git a/net/base/sdch_manager.cc b/net/base/sdch_manager.cc
index 59589fb544723d386fa37019dd30a574cd8432d0..bd68cede3f522e7d622b06e323212856ed5d39e3 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -9,8 +9,10 @@
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/values.h"
 #include "crypto/sha2.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/base/sdch_net_log_params.h"
 #include "net/url_request/url_request_http_job.h"
 
 namespace net {
@@ -56,7 +58,8 @@ SdchManager::Dictionary::Dictionary(const std::string& dictionary_text,
 SdchManager::Dictionary::~Dictionary() {
 }
 
-bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) {
+SdchManager::ProblemCodes SdchManager::Dictionary::CanAdvertise(
+    const GURL& target_url) const {
   /* The specific rules of when a dictionary should be advertised in an
      Avail-Dictionary header are modeled after the rules for cookie scoping. The
      terms "domain-match" and "pathmatch" are defined in RFC 2965 [6]. A
@@ -73,28 +76,29 @@ bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) {
      url scheme.
     */
   if (!DomainMatch(target_url, domain_))
-    return false;
+    return DICTIONARY_FOUND_HAS_WRONG_DOMAIN;
   if (!ports_.empty() && 0 == ports_.count(target_url.EffectiveIntPort()))
-    return false;
+    return DICTIONARY_FOUND_HAS_WRONG_PORT_LIST;
   if (path_.size() && !PathMatch(target_url.path(), path_))
-    return false;
+    return DICTIONARY_FOUND_HAS_WRONG_PATH;
   if (!SdchManager::secure_scheme_supported() && target_url.SchemeIsSecure())
-    return false;
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
   if (target_url.SchemeIsSecure() != url_.SchemeIsSecure())
-    return false;
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
   if (base::Time::Now() > expiration_)
-    return false;
-  return true;
+    return DICTIONARY_FOUND_EXPIRED;
+  return PROBLEM_CODE_OK;
 }
 
 //------------------------------------------------------------------------------
 // Security functions restricting loads and use of dictionaries.
 
 // static
-bool SdchManager::Dictionary::CanSet(const std::string& domain,
-                                     const std::string& path,
-                                     const std::set<int>& ports,
-                                     const GURL& dictionary_url) {
+SdchManager::ProblemCodes SdchManager::Dictionary::CanSet(
+    const std::string& domain,
+    const std::string& path,
+    const std::set<int>& ports,
+    const GURL& dictionary_url) {
   /*
   A dictionary is invalid and must not be stored if any of the following are
   true:
@@ -113,20 +117,16 @@ bool SdchManager::Dictionary::CanSet(const std::string& domain,
   // and hence the conservative approach is to not allow any redirects (if there
   // were any... then don't allow the dictionary to be set).
 
-  if (domain.empty()) {
-    SdchErrorRecovery(DICTIONARY_MISSING_DOMAIN_SPECIFIER);
-    return false;  // Domain is required.
-  }
+  if (domain.empty())
+    return DICTIONARY_MISSING_DOMAIN_SPECIFIER;  // Domain is required.
+
   if (registry_controlled_domains::GetDomainAndRegistry(
-        domain,
-        registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES).empty()) {
-    SdchErrorRecovery(DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN);
-    return false;  // domain was a TLD.
-  }
-  if (!Dictionary::DomainMatch(dictionary_url, domain)) {
-    SdchErrorRecovery(DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL);
-    return false;
-  }
+          domain, registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)
+          .empty())
+    return DICTIONARY_SPECIFIES_TOP_LEVEL_DOMAIN;  // domain was a TLD.
+
+  if (!Dictionary::DomainMatch(dictionary_url, domain))
+    return DICTIONARY_DOMAIN_NOT_MATCHING_SOURCE_URL;
 
   std::string referrer_url_host = dictionary_url.host();
   size_t postfix_domain_index = referrer_url_host.rfind(domain);
@@ -134,23 +134,19 @@ bool SdchManager::Dictionary::CanSet(const std::string& domain,
   if (referrer_url_host.size() == postfix_domain_index + domain.size()) {
     // It is a postfix... so check to see if there's a dot in the prefix.
     size_t end_of_host_index = referrer_url_host.find_first_of('.');
-    if (referrer_url_host.npos != end_of_host_index  &&
-        end_of_host_index < postfix_domain_index) {
-      SdchErrorRecovery(DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX);
-      return false;
-    }
+    if (referrer_url_host.npos != end_of_host_index &&
+        end_of_host_index < postfix_domain_index)
+      return DICTIONARY_REFERER_URL_HAS_DOT_IN_PREFIX;
   }
 
-  if (!ports.empty()
-      && 0 == ports.count(dictionary_url.EffectiveIntPort())) {
-    SdchErrorRecovery(DICTIONARY_PORT_NOT_MATCHING_SOURCE_URL);
-    return false;
-  }
-  return true;
+  if (!ports.empty() && 0 == ports.count(dictionary_url.EffectiveIntPort()))
+    return DICTIONARY_PORT_NOT_MATCHING_SOURCE_URL;
+
+  return PROBLEM_CODE_OK;
 }
 
-// static
-bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
+SdchManager::ProblemCodes SdchManager::Dictionary::CanUse(
+    const GURL& referring_url) const {
   /*
     1. The request URL's host name domain-matches the Domain attribute of the
       dictionary.
@@ -162,39 +158,30 @@ bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
     HTTPS support AND the dictionary acquisition scheme matches the target
      url scheme.
   */
-  if (!DomainMatch(referring_url, domain_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_DOMAIN);
-    return false;
-  }
-  if (!ports_.empty()
-      && 0 == ports_.count(referring_url.EffectiveIntPort())) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PORT_LIST);
-    return false;
-  }
-  if (path_.size() && !PathMatch(referring_url.path(), path_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PATH);
-    return false;
-  }
-  if (!SdchManager::secure_scheme_supported() &&
-      referring_url.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
-  if (referring_url.SchemeIsSecure() != url_.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
+  if (!DomainMatch(referring_url, domain_))
+    return DICTIONARY_FOUND_HAS_WRONG_DOMAIN;
+
+  if (!ports_.empty() && 0 == ports_.count(referring_url.EffectiveIntPort()))
+    return DICTIONARY_FOUND_HAS_WRONG_PORT_LIST;
+
+  if (path_.size() && !PathMatch(referring_url.path(), path_))
+    return DICTIONARY_FOUND_HAS_WRONG_PATH;
+
+  if (!SdchManager::secure_scheme_supported() && referring_url.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
+
+  if (referring_url.SchemeIsSecure() != url_.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
 
   // TODO(jar): Remove overly restrictive failsafe test (added per security
   // review) when we have a need to be more general.
-  if (!referring_url.SchemeIsHTTPOrHTTPS()) {
-    SdchErrorRecovery(ATTEMPT_TO_DECODE_NON_HTTP_DATA);
-    return false;
-  }
+  if (!referring_url.SchemeIsHTTPOrHTTPS())
+    return ATTEMPT_TO_DECODE_NON_HTTP_DATA;
 
-  return true;
+  return PROBLEM_CODE_OK;
 }
 
+// static
 bool SdchManager::Dictionary::PathMatch(const std::string& path,
                                         const std::string& restriction) {
   /*  Must be either:
@@ -306,7 +293,7 @@ void SdchManager::ClearDomainBlacklisting(const std::string& domain) {
   BlacklistInfo* blacklist_info = &blacklisted_domains_[
       base::StringToLowerASCII(domain)];
   blacklist_info->count = 0;
-  blacklist_info->reason = MIN_PROBLEM_CODE;
+  blacklist_info->reason = PROBLEM_CODE_OK;
 }
 
 int SdchManager::BlackListDomainCount(const std::string& domain) {
@@ -325,48 +312,66 @@ int SdchManager::BlacklistDomainExponential(const std::string& domain) {
   return blacklisted_domains_[domain_lower].exponential_count;
 }
 
-bool SdchManager::IsInSupportedDomain(const GURL& url) {
+SdchManager::ProblemCodes SdchManager::IsInSupportedDomain(const GURL& url) {
   DCHECK(CalledOnValidThread());
   if (!g_sdch_enabled_ )
-    return false;
+    return SDCH_DISABLED;
 
   if (!secure_scheme_supported() && url.SchemeIsSecure())
-    return false;
+    return SECURE_SCHEME_NOT_SUPPORTED;
 
   if (blacklisted_domains_.empty())
-    return true;
+    return PROBLEM_CODE_OK;
 
   DomainBlacklistInfo::iterator it =
       blacklisted_domains_.find(base::StringToLowerASCII(url.host()));
   if (blacklisted_domains_.end() == it || it->second.count == 0)
-    return true;
+    return PROBLEM_CODE_OK;
 
   UMA_HISTOGRAM_ENUMERATION("Sdch3.BlacklistReason", it->second.reason,
                             MAX_PROBLEM_CODE);
-  SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);

[Randy Smith (Not in Mondays), 2014/09/18 20:55:51]

Just to summarize what I think the refactor of this routine did in terms of
calls to SdchManager::SdchErrorRecovery(), in case I missed something: It looks
like you removed this call here, return problem codes but don't log them in the
callers, *except* that in
URLRequestHttpJob::{NotifyHeadersComplete,AddExtraHeaders} you log only
DOMAIN_BLACKLIST_INCLUDES_TARGET.

So I'm struggling with what I'd like you to do here.  The existing code is a
mess, for several different reasons.  However, I feel as if, if we change the
places in the code that file UMA for DOMAIN_BLACKLIST_INCLUDES_TARGET, we need
to rev the histogram with all the hassle that involves (or, to be fair,
deprecate this constant and create a new one that means something different).  I
think this change (and the changes in the callers) does indeed change the
frequency and context in which this UMA is logged (since it's no longer logged
in the GetVcdiffDictionary/GetAvailDictionary/AddSdchDictionary cases.

I don't want to fix the histogram or deprecate the constant until I'm clear on
what the right thing to do is, and I don't want to do it multiple times.  And
I'd also like to keep a 1-1 correspondence between a logging of
BLACKLIST_INCLUDES_TARGET and an entry in Sdch3.BlacklistReason.  

So what I'd like to suggest is that you put the SdchErrorRecovery() call back
here, and remove the UMA logging from URLRequestHttpJob::* (no change needed to
the net log logging; you're creating that, so there's no consistency issues). 
My I get my proverbial round tuit, I'll kill BLACKLIST_INCLUDES_TARGET with
fire, but until then we'll keep it in its current form.

Please let me know if you see any problems with that plan.  My apologies if I've
asked for different behavior in the past; I skimmed previous comments to see if
I could figure out what guidance I've previously given and didn't find any, but
given the history of this CL, I might have missed a discussion.

[baranovich, 2014/09/19 12:42:44]

Got your point, although GetVcdiffDictionary/AddSdchDictionary logs
DOMAIN_BLACKLIST_INCLUDES_TARGET (for GetVcdiffDictionary SdchFilter does this,
AddSdchDictionary logs everything inside it). but there are error in how they do
it now (should filter out SDCH_DISABLED and SECURE_SCHEME_NOT_SUPPORTED from
histograms). So what I've done is moving logging out of this function.
That implicitly removes SdchErrorRecovery() call from GetAvailDictionary. But
you were saying that this call is badly broken and histogram is a mess because
of it, since it was intended to have a single DOMAIN_BLACKLIST_INCLUDES_TARGET
(from the call to IsInSupportedDomain, I guess) record when URLRequestHttpJob
tries to get the dicts instead of zillions of them when GetAvailDictionary lists
all the dicts. So I decided to skip this, because the code looks much simpler
that way.

But I could make all the histograms be reported as-is with just a little more
hacks:) Introducing another histogram later is indeed a better dicision.

On 2014/09/18 20:55:51, rdsmith wrote:

[baranovich, 2014/09/30 13:16:51]

Done. I've restored the frequency if the blacklist histogram. Now it is recorded
outside by the caller.

 
   int count = it->second.count - 1;
   if (count > 0) {
     it->second.count = count;
   } else {
     it->second.count = 0;
-    it->second.reason = MIN_PROBLEM_CODE;
+    it->second.reason = PROBLEM_CODE_OK;
   }
 
-  return false;
+  return DOMAIN_BLACKLIST_INCLUDES_TARGET;
 }
 
-void SdchManager::FetchDictionary(const GURL& request_url,
-                                  const GURL& dictionary_url) {
+SdchManager::ProblemCodes SdchManager::FetchDictionary(
+    const GURL& request_url,
+    const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
-  if (CanFetchDictionary(request_url, dictionary_url) && fetcher_.get()) {
+  ProblemCodes rv = CanFetchDictionary(request_url, dictionary_url);
+  if (rv != PROBLEM_CODE_OK)
+    return rv;
+
+  if (fetcher_.get()) {
     ++fetches_count_for_testing_;
-    fetcher_->Schedule(dictionary_url);
+    SdchFetcher::ScheduleResult result = fetcher_->Schedule(dictionary_url);
+    switch (result) {
+      case SdchFetcher::ALREADY_SCHEDULED:
+        rv = DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD;
+        break;
+      case SdchFetcher::ALREADY_TRIED:
+        rv = DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD;
+        break;
+      default:
+        DCHECK_EQ(SdchFetcher::SCHEDULE_OK, result);
+        break;
+    }
   }
+
+  return rv;
 }
 
-bool SdchManager::CanFetchDictionary(const GURL& referring_url,
-                                     const GURL& dictionary_url) const {
+SdchManager::ProblemCodes SdchManager::CanFetchDictionary(
+    const GURL& referring_url,
+    const GURL& dictionary_url) const {
   DCHECK(CalledOnValidThread());
   /* The user agent may retrieve a dictionary from the dictionary URL if all of
      the following are true:
@@ -381,41 +386,40 @@ bool SdchManager::CanFetchDictionary(const GURL& referring_url,
   // Item (1) above implies item (2).  Spec should be updated.
   // I take "host name match" to be "is identical to"
   if (referring_url.host() != dictionary_url.host() ||
-      referring_url.scheme() != dictionary_url.scheme()) {
-    SdchErrorRecovery(DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST);
-    return false;
-  }
-  if (!secure_scheme_supported() && referring_url.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_SELECTED_FOR_SSL);
-    return false;
-  }
+      referring_url.scheme() != dictionary_url.scheme())
+    return DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST;
+
+  if (!secure_scheme_supported() && referring_url.SchemeIsSecure())
+    return DICTIONARY_SELECTED_FOR_SSL;
 
   // TODO(jar): Remove this failsafe conservative hack which is more restrictive
   // than current SDCH spec when needed, and justified by security audit.
-  if (!referring_url.SchemeIsHTTPOrHTTPS()) {
-    SdchErrorRecovery(DICTIONARY_SELECTED_FROM_NON_HTTP);
-    return false;
-  }
+  if (!referring_url.SchemeIsHTTPOrHTTPS())
+    return DICTIONARY_SELECTED_FROM_NON_HTTP;
 
-  return true;
+  return PROBLEM_CODE_OK;
 }
 
-void SdchManager::GetVcdiffDictionary(
+SdchManager::ProblemCodes SdchManager::GetVcdiffDictionary(
     const std::string& server_hash,
     const GURL& referring_url,
     scoped_refptr<Dictionary>* dictionary) {
   DCHECK(CalledOnValidThread());
   *dictionary = NULL;
   DictionaryMap::iterator it = dictionaries_.find(server_hash);
-  if (it == dictionaries_.end()) {
-    return;
-  }
+  if (it == dictionaries_.end())
+    return PROBLEM_CODE_OK;  // not a problem. Just no dictionary.
+
   scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
-    return;
-  if (!matching_dictionary->CanUse(referring_url))
-    return;
-  *dictionary = matching_dictionary;
+
+  ProblemCodes rv = IsInSupportedDomain(referring_url);
+  if (rv != PROBLEM_CODE_OK)
+    return rv;
+
+  rv = matching_dictionary->CanUse(referring_url);
+  if (rv == PROBLEM_CODE_OK)
+    *dictionary = matching_dictionary;
+  return rv;
 }
 
 // TODO(jar): If we have evictions from the dictionaries_, then we need to
@@ -427,9 +431,9 @@ void SdchManager::GetAvailDictionaryList(const GURL& target_url,
   int count = 0;
   for (DictionaryMap::iterator it = dictionaries_.begin();
        it != dictionaries_.end(); ++it) {
-    if (!IsInSupportedDomain(target_url))
+    if (IsInSupportedDomain(target_url) != PROBLEM_CODE_OK)
       continue;
-    if (!it->second->CanAdvertise(target_url))
+    if (it->second->CanAdvertise(target_url) != PROBLEM_CODE_OK)
       continue;
     ++count;
     if (!list->empty())
@@ -479,13 +483,15 @@ void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
 }
 
 void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
-    const GURL& dictionary_url) {
+                                    const GURL& dictionary_url,
+                                    const BoundNetLog& net_log) {
   DCHECK(CalledOnValidThread());
   std::string client_hash;
   std::string server_hash;
   GenerateHash(dictionary_text, &client_hash, &server_hash);
   if (dictionaries_.find(server_hash) != dictionaries_.end()) {
-    SdchErrorRecovery(DICTIONARY_ALREADY_LOADED);
+    LogDictionaryError(
+        DICTIONARY_ALREADY_LOADED, dictionary_url, true, net_log);
     return;                             // Already loaded.
   }
 
@@ -494,13 +500,13 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   base::Time expiration(base::Time::Now() + base::TimeDelta::FromDays(30));
 
   if (dictionary_text.empty()) {
-    SdchErrorRecovery(DICTIONARY_HAS_NO_TEXT);
+    LogDictionaryError(DICTIONARY_HAS_NO_TEXT, dictionary_url, true, net_log);
     return;                             // Missing header.
   }
 
   size_t header_end = dictionary_text.find("\n\n");
   if (std::string::npos == header_end) {
-    SdchErrorRecovery(DICTIONARY_HAS_NO_HEADER);
+    LogDictionaryError(DICTIONARY_HAS_NO_HEADER, dictionary_url, true, net_log);
     return;                             // Missing header.
   }
   size_t line_start = 0;  // Start of line being parsed.
@@ -511,7 +517,8 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
 
     size_t colon_index = dictionary_text.find(':', line_start);
     if (std::string::npos == colon_index) {
-      SdchErrorRecovery(DICTIONARY_HEADER_LINE_MISSING_COLON);
+      LogDictionaryError(
+          DICTIONARY_HEADER_LINE_MISSING_COLON, dictionary_url, true, net_log);
       return;                         // Illegal line missing a colon.
     }
 
@@ -532,6 +539,8 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
         path = value;
       } else if (name == "format-version") {
         if (value != "1.0")
+          LogDictionaryError(
+              UNSUPPORTED_VERSION, dictionary_url, true, net_log);
           return;
       } else if (name == "max-age") {
         int64 seconds;
@@ -550,22 +559,29 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     line_start = line_end + 1;
   }
 
-  if (!IsInSupportedDomain(dictionary_url))
+  ProblemCodes rv = IsInSupportedDomain(dictionary_url);
+  if (rv != PROBLEM_CODE_OK) {
+    LogDictionaryError(rv, dictionary_url, true, net_log);
     return;
+  }
 
-  if (!Dictionary::CanSet(domain, path, ports, dictionary_url))
+  rv = Dictionary::CanSet(domain, path, ports, dictionary_url);
+  if (rv != PROBLEM_CODE_OK) {
+    LogDictionaryError(rv, dictionary_url, true, net_log);
     return;
+  }
 
   // TODO(jar): Remove these hacks to preclude a DOS attack involving piles of
   // useless dictionaries.  We should probably have a cache eviction plan,
   // instead of just blocking additions.  For now, with the spec in flux, it
   // is probably not worth doing eviction handling.
   if (kMaxDictionarySize < dictionary_text.size()) {
-    SdchErrorRecovery(DICTIONARY_IS_TOO_LARGE);
+    LogDictionaryError(DICTIONARY_IS_TOO_LARGE, dictionary_url, true, net_log);
     return;
   }
   if (kMaxDictionaryCount <= dictionaries_.size()) {
-    SdchErrorRecovery(DICTIONARY_COUNT_EXCEEDED);
+    LogDictionaryError(
+        DICTIONARY_COUNT_EXCEEDED, dictionary_url, true, net_log);
     return;
   }
 
@@ -589,4 +605,61 @@ void SdchManager::UrlSafeBase64Encode(const std::string& input,
   std::replace(output->begin(), output->end(), '/', '_');
 }
 
+base::Value* SdchManager::SdchInfoToValue() const {
+  base::DictionaryValue* value = new base::DictionaryValue();
+
+  value->SetBoolean("sdch_enabled", sdch_enabled());
+  value->SetBoolean("secure_scheme_support", secure_scheme_supported());
+
+  base::ListValue* entry_list = new base::ListValue();
+  for (DictionaryMap::const_iterator it = dictionaries_.begin();
+       it != dictionaries_.end();
+       ++it) {
+    base::DictionaryValue* entry_dict = new base::DictionaryValue();
+    entry_dict->SetString("url", it->second->url().spec());
+    entry_dict->SetString("client_hash", it->second->client_hash());
+    entry_dict->SetString("domain", it->second->domain());
+    entry_dict->SetString("path", it->second->path());
+    base::ListValue* port_list = new base::ListValue();
+    for (std::set<int>::const_iterator port_it = it->second->ports().begin();
+         port_it != it->second->ports().end();
+         ++port_it) {
+      port_list->AppendInteger(*port_it);
+    }
+    entry_dict->Set("ports", port_list);
+    entry_dict->SetString("server_hash", it->first);
+    entry_list->Append(entry_dict);
+  }
+  value->Set("dictionaries", entry_list);
+
+  entry_list = new base::ListValue();
+  for (DomainBlacklistInfo::const_iterator it = blacklisted_domains_.begin();
+       it != blacklisted_domains_.end();
+       ++it) {
+    if (it->second.count == 0)
+      continue;
+    base::DictionaryValue* entry_dict = new base::DictionaryValue();
+    entry_dict->SetString("domain", it->first);
+    if (it->second.count != INT_MAX)
+      entry_dict->SetInteger("tries", it->second.count);
+    entry_dict->SetInteger("reason", it->second.reason);
+    entry_list->Append(entry_dict);
+  }
+  value->Set("blacklisted", entry_list);
+
+  return value;
+}
+
+// static

[Randy Smith (Not in Mondays), 2014/09/18 20:55:51]

Why not make this a file static or put it in an anonymous namespace in this
file?  It's not called from outside this file, so making the visibility as
narrow as possible seems a good thing.

+void SdchManager::LogDictionaryError(ProblemCodes error,
+                                     const GURL& url,
+                                     bool is_error,
+                                     const BoundNetLog& net_log) {
+  SdchErrorRecovery(error);
+  net_log.AddEvent(
+      NetLog::TYPE_SDCH_DICTIONARY_FETCH_ERROR,
+      base::Bind(
+          &NetLogSdchDictionaryFetchProblemCallback, error, &url, is_error));
+}
+
 }  // namespace net