Move full hash caching logic to v4_get_hash_protocol_manager

components/safe_browsing_db/database_manager.cc

Index: components/safe_browsing_db/database_manager.cc
diff --git a/components/safe_browsing_db/database_manager.cc b/components/safe_browsing_db/database_manager.cc
index b3bb2a6d24aa846bb91c2df250021a5991e60edd..d8f8512ed4f1b337d2928d63753d3af8cdcc8f1f 100644
--- a/components/safe_browsing_db/database_manager.cc
+++ b/components/safe_browsing_db/database_manager.cc
@@ -14,23 +14,6 @@ using content::BrowserThread;
 
 namespace {
 
-// Enumerate full hash cache hits/misses for histogramming purposes.
-// DO NOT CHANGE THE ORDERING OF THESE VALUES.
-enum V4FullHashCacheResultType {
-  // Full hashes for which there is no cache hit.
-  FULL_HASH_CACHE_MISS = 0,
-
-  // Full hashes with a cache hit.
-  FULL_HASH_CACHE_HIT = 1,
-
-  // Full hashes with a negative cache hit.
-  FULL_HASH_NEGATIVE_CACHE_HIT = 2,
-
-  // Memory space for histograms is determined by the max. ALWAYS
-  // ADD NEW VALUES BEFORE THIS ONE.
-  FULL_HASH_CACHE_RESULT_MAX
-};
-
 // Enumerate GetHash hits/misses for histogramming purposes. DO NOT CHANGE THE
 // ORDERING OF THESE VALUES.
 enum V4GetHashCheckResultType {
@@ -48,13 +31,6 @@ enum V4GetHashCheckResultType {
   GET_HASH_CHECK_RESULT_MAX
 };
 
-// Record a full hash cache hit result.
-void RecordV4FullHashCacheResult(
-    V4FullHashCacheResultType result_type) {
-  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4FullHashCacheResult", result_type,
-                            FULL_HASH_CACHE_RESULT_MAX);
-}
-
 // Record a GetHash hit result.
 void RecordV4GetHashCheckResult(
     V4GetHashCheckResultType result_type) {
@@ -154,7 +130,7 @@ bool SafeBrowsingDatabaseManager::CheckApiBlacklistUrl(const GURL& url,
 
   std::vector<SBFullHashResult> cached_results;
   std::vector<SBPrefix> prefixes_needing_reqs;
-  GetFullHashCachedResults(SB_THREAT_TYPE_API_ABUSE,
+  v4_get_hash_protocol_manager_->GetFullHashCachedResults(SB_THREAT_TYPE_API_ABUSE,

[Nathan Parker, 2016/08/15 19:54:47]

How about moving all of this code into v4_get_hash_protocol_manager?  That is,
so the caller doesn't even know there is a cache.  Ideally it'd be just an
optimization, and shouldn't affect the PM API.

       full_hashes, now, &prefixes_needing_reqs, &cached_results);
 
   if (prefixes_needing_reqs.empty() && cached_results.empty())
@@ -182,107 +158,6 @@ bool SafeBrowsingDatabaseManager::CheckApiBlacklistUrl(const GURL& url,
   return false;
 }
 
-void SafeBrowsingDatabaseManager::GetFullHashCachedResults(
-    const SBThreatType& threat_type,
-    const std::vector<SBFullHash>& full_hashes,
-    base::Time now,
-    std::vector<SBPrefix>* prefixes_needing_reqs,
-    std::vector<SBFullHashResult>* cached_results) {
-  DCHECK(prefixes_needing_reqs);
-  prefixes_needing_reqs->clear();
-  DCHECK(cached_results);
-  cached_results->clear();
-
-  // Caching behavior is documented here:
-  // https://developers.google.com/safe-browsing/v4/caching#about-caching
-  //
-  // The cache operates as follows:
-  // Lookup:
-  //     Case 1: The prefix is in the cache.
-  //         Case a: The full hash is in the cache.
-  //             Case i : The positive full hash result has not expired.
-  //                      The result is unsafe and we do not need to send a new
-  //                      request.
-  //             Case ii: The positive full hash result has expired.
-  //                      We need to send a request for full hashes.
-  //         Case b: The full hash is not in the cache.
-  //             Case i : The negative cache entry has not expired.
-  //                      The result is still safe and we do not need to send a
-  //                      new request.
-  //             Case ii: The negative cache entry has expired.
-  //                      We need to send a request for full hashes.
-  //     Case 2: The prefix is not in the cache.
-  //             We need to send a request for full hashes.
-  //
-  // Eviction:
-  //   SBCachedFullHashResult entries can be removed from the cache only when
-  //   the negative cache expire time and the cache expire time of all full
-  //   hash results for that prefix have expired.
-  //   Individual full hash results can be removed from the prefix's
-  //   cache entry if they expire AND their expire time is after the negative
-  //   cache expire time.
-  for (const SBFullHash& full_hash : full_hashes) {
-    auto entry = v4_full_hash_cache_[threat_type].find(full_hash.prefix);
-    if (entry != v4_full_hash_cache_[threat_type].end()) {
-      // Case 1.
-      SBCachedFullHashResult& cache_result = entry->second;
-
-      const SBFullHashResult* found_full_hash = nullptr;
-      size_t matched_idx = 0;
-      for (const SBFullHashResult& hash_result : cache_result.full_hashes) {
-        if (SBFullHashEqual(full_hash, hash_result.hash)) {
-          found_full_hash = &hash_result;
-          break;
-        }
-        ++matched_idx;
-      }
-
-      if (found_full_hash) {
-        // Case a.
-        if (found_full_hash->cache_expire_after > now) {
-          // Case i.
-          cached_results->push_back(*found_full_hash);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_HIT);
-        } else {
-          // Case ii.
-          prefixes_needing_reqs->push_back(full_hash.prefix);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-          // If the negative cache expire time has passed, evict this full hash
-          // result from the cache.
-          if (cache_result.expire_after <= now) {
-            cache_result.full_hashes.erase(
-                cache_result.full_hashes.begin() + matched_idx);
-            // If there are no more full hashes, we can evict the entire entry.
-            if (cache_result.full_hashes.empty()) {
-              v4_full_hash_cache_[threat_type].erase(entry);
-            }
-          }
-        }
-      } else {
-        // Case b.
-        if (cache_result.expire_after > now) {
-          // Case i.
-          RecordV4FullHashCacheResult(FULL_HASH_NEGATIVE_CACHE_HIT);
-        } else {
-          // Case ii.
-          prefixes_needing_reqs->push_back(full_hash.prefix);
-          RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-        }
-      }
-    } else {
-      // Case 2.
-      prefixes_needing_reqs->push_back(full_hash.prefix);
-      RecordV4FullHashCacheResult(FULL_HASH_CACHE_MISS);
-    }
-  }
-
-  // Multiple full hashes could share a prefix, remove duplicates.
-  // TODO(kcarattini): Make |prefixes_needing_reqs| a set.
-  std::sort(prefixes_needing_reqs->begin(), prefixes_needing_reqs->end());
-  prefixes_needing_reqs->erase(std::unique(prefixes_needing_reqs->begin(),
-      prefixes_needing_reqs->end()), prefixes_needing_reqs->end());
-}
-
 void SafeBrowsingDatabaseManager::HandleGetHashesWithApisResults(
     SafeBrowsingApiCheck* check,
     const std::vector<SBFullHashResult>& full_hash_results,
@@ -296,22 +171,6 @@ void SafeBrowsingDatabaseManager::HandleGetHashesWithApisResults(
                              base::TimeTicks::Now() - check->start_time());
   }
 
-  // If the time is uninitialized, don't cache the results.
-  if (!negative_cache_expire.is_null()) {
-    // Cache the results.
-    // Create or reset all cached results for this prefix.
-    for (const SBPrefix& prefix : check->prefixes()) {
-      v4_full_hash_cache_[SB_THREAT_TYPE_API_ABUSE][prefix] =
-          SBCachedFullHashResult(negative_cache_expire);
-    }
-    // Insert any full hash hits. Note that there may be one, multiple, or no
-    // full hashes for any given prefix.
-    for (const SBFullHashResult& result : full_hash_results) {
-      v4_full_hash_cache_[SB_THREAT_TYPE_API_ABUSE][result.hash.prefix].
-          full_hashes.push_back(result);
-    }
-  }
-
   // If the check is not in |api_checks_| then the request was cancelled by the
   // client.
   ApiCheckSet::iterator it = api_checks_.find(check);