Added stats on why blacklisted requests were blacklisted.

net/base/sdch_manager.cc

 // Copyright (c) 2012 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.
 
 #include "net/base/sdch_manager.h"
 
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 214 matching lines @@
 SdchManager::~SdchManager() {
   DCHECK(CalledOnValidThread());
   while (!dictionaries_.empty()) {
     DictionaryMap::iterator it = dictionaries_.begin();
     dictionaries_.erase(it->first);
   }
 }
 
 void SdchManager::ClearData() {
   blacklisted_domains_.clear();
-  exponential_blacklist_count_.clear();
   allow_latency_experiment_.clear();
   if (fetcher_.get())
     fetcher_->Cancel();
 
   // Note that this may result in not having dictionaries we've advertised
   // for incoming responses.  The window is relatively small (as ClearData()
   // is not expected to be called frequently), so we rely on meta-refresh
   // to handle this case.
   dictionaries_.clear();
 }
@@ skipping 11 matching lines @@
 // static
 void SdchManager::EnableSdchSupport(bool enabled) {
   g_sdch_enabled_ = enabled;
 }
 
 // static
 void SdchManager::EnableSecureSchemeSupport(bool enabled) {
   g_secure_scheme_supported_ = enabled;
 }
 
-void SdchManager::BlacklistDomain(const GURL& url) {
+void SdchManager::BlacklistDomain(const GURL& url,
+                                  ProblemCodes blacklist_reason) {
   SetAllowLatencyExperiment(url, false);
 
   std::string domain(StringToLowerASCII(url.host()));
-  int count = blacklisted_domains_[domain];
-  if (count > 0)
+  BlacklistInfo* blacklist_info = &blacklisted_domains_[domain];
+
+  if (blacklist_info->count > 0)
     return;  // Domain is already blacklisted.
 
-  count = 1 + 2 * exponential_blacklist_count_[domain];
+  int count = 1 + 2 * blacklist_info->exponential_count;
   if (count > 0)
-    exponential_blacklist_count_[domain] = count;
+    blacklist_info->exponential_count = count;
   else
-    count = INT_MAX;
+    blacklist_info->exponential_count = INT_MAX;
 
-  blacklisted_domains_[domain] = count;
+  blacklist_info->count = blacklist_info->exponential_count;
+  blacklist_info->reason = blacklist_reason;
 }
 
-void SdchManager::BlacklistDomainForever(const GURL& url) {
+void SdchManager::BlacklistDomainForever(const GURL& url,
+                                         ProblemCodes blacklist_reason) {
   SetAllowLatencyExperiment(url, false);
 
   std::string domain(StringToLowerASCII(url.host()));
-  exponential_blacklist_count_[domain] = INT_MAX;
-  blacklisted_domains_[domain] = INT_MAX;
+  BlacklistInfo* blacklist_info = &blacklisted_domains_[domain];
+  blacklist_info->count = INT_MAX;
+  blacklist_info->exponential_count = INT_MAX;
+  blacklist_info->reason = blacklist_reason;
 }
 
 void SdchManager::ClearBlacklistings() {
   blacklisted_domains_.clear();
-  exponential_blacklist_count_.clear();
 }
 
 void SdchManager::ClearDomainBlacklisting(const std::string& domain) {
-  blacklisted_domains_.erase(StringToLowerASCII(domain));
+  BlacklistInfo* blacklist_info = &blacklisted_domains_[
+      StringToLowerASCII(domain)];
+  blacklist_info->count = 0;
+  blacklist_info->reason = MIN_PROBLEM_CODE;

[jar (doing other things), 2014/08/05 23:33:30]

Why did you change to leaving the entry in the map, rather than erasing it??

[Randy Smith (Not in Mondays), 2014/08/11 20:46:39]

Because the map now contains the exponential backoff information, which is
needed for the next time the domain is blacklisted.

 }
 
 int SdchManager::BlackListDomainCount(const std::string& domain) {
-  if (blacklisted_domains_.end() == blacklisted_domains_.find(domain))
+  std::string domain_lower(StringToLowerASCII(domain));

[jar (doing other things), 2014/08/05 23:33:30]

Good bug fix! Old code assumed it was already lower.

nit: Perhaps you should rename |domain| to |domain_lower| in other methods
before use as an index?

[Randy Smith (Not in Mondays), 2014/08/11 20:46:39]

Done (or got rid of any variable at all), everywhere but in the middle of
AddSdchDictionary, where it's used in a couple of different ways, not just as an
index.

+
+  if (blacklisted_domains_.end() == blacklisted_domains_.find(domain_lower))
     return 0;
-  return blacklisted_domains_[StringToLowerASCII(domain)];
+  return blacklisted_domains_[domain_lower].count;
 }
 
 int SdchManager::BlacklistDomainExponential(const std::string& domain) {
-  if (exponential_blacklist_count_.end() ==
-      exponential_blacklist_count_.find(domain))
+  std::string domain_lower(StringToLowerASCII(domain));
+
+  if (blacklisted_domains_.end() == blacklisted_domains_.find(domain_lower))
     return 0;
-  return exponential_blacklist_count_[StringToLowerASCII(domain)];
+  return blacklisted_domains_[domain_lower].exponential_count;
 }
 
 bool SdchManager::IsInSupportedDomain(const GURL& url) {
   DCHECK(CalledOnValidThread());
   if (!g_sdch_enabled_ )
     return false;
 
   if (!secure_scheme_supported() && url.SchemeIsSecure())
     return false;
 
   if (blacklisted_domains_.empty())
     return true;
 
   std::string domain(StringToLowerASCII(url.host()));
-  DomainCounter::iterator it = blacklisted_domains_.find(domain);
-  if (blacklisted_domains_.end() == it)
+  DomainBlacklistInfo::iterator it = blacklisted_domains_.find(domain);
+  if (blacklisted_domains_.end() == it || it->second.count == 0)
     return true;
 
-  int count = it->second - 1;
-  if (count > 0)
-    blacklisted_domains_[domain] = count;
-  else
-    blacklisted_domains_.erase(domain);
+  int count = it->second.count - 1;
+  if (count > 0) {
+    it->second.count = count;
+  } else {
+    it->second.count = 0;
+    it->second.reason = MIN_PROBLEM_CODE;
+  }
+
+  UMA_HISTOGRAM_ENUMERATION("Sdch3.BlacklistReason", it->second.reason,

[jar (doing other things), 2014/08/05 23:33:30]

Maybe this should go before line 343, since it may otherwise be cleared in line
345.  ...or is this useful in that context?

[Randy Smith (Not in Mondays), 2014/08/11 20:46:39]

Ooops; thanks for the catch.  No, I think we still want the error code on the
last rejection.

+                            MAX_PROBLEM_CODE);
   SdchErrorRecovery(DOMAIN_BLACKLIST_INCLUDES_TARGET);
   return false;
 }
 
 void SdchManager::FetchDictionary(const GURL& request_url,
                                   const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
   if (CanFetchDictionary(request_url, dictionary_url) && fetcher_.get())
     fetcher_->Schedule(dictionary_url);
 }
@@ skipping 226 matching lines @@
       case '/':
         (*output)[i] = '_';
         continue;
       default:
         continue;
     }
   }
 }
 
 }  // namespace net