PVer4: Re-use the PVer3 implementation to get full hashes

components/safe_browsing_db/v4_get_hash_protocol_manager.cc

 // Copyright 2016 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 "components/safe_browsing_db/v4_get_hash_protocol_manager.h"
 
 #include <utility>
 
 #include "base/base64url.h"
 #include "base/macros.h"
@@ skipping 280 matching lines @@
       RecordParseGetHashResult(UNEXPECTED_THREAT_TYPE_ERROR);
       return false;
     }
 
     full_hashes->push_back(result);
   }
   return true;
 }
 
 void V4GetHashProtocolManager::GetFullHashes(
+    const base::hash_set<HashPrefix>& prefixes,
+    const PlatformTypeSet& platforms,
+    const ThreatEntryTypeSet& threat_entry_types,
+    const ThreatTypeSet& threat_types,
+    FullHashCallback callback) {

[Scott Hess - ex-Googler, 2016/08/23 20:25:46]

I think these are in the opposite order from the header?

I think having the function overloaded has strong potential for confusion. 
IMHO, you should probably just rename the V3 version to GetFullHashesV3 or
something, which will make it clear which call sites need conversion.  Yeah, it
means you'll touch more files than you might like, but *shrug*, you'll have to
touch them eventually.

+  DCHECK(CalledOnValidThread());
+  DCHECK(!prefixes.empty());
+  DCHECK(!platforms.empty());
+  DCHECK(!threat_entry_types.empty());
+  DCHECK(!threat_types.empty());
+
+  // TODO(vakh): The following code currently relies on the PVer3
+  // implementation but should not. This will be fixed after
+  // http://crbug.com/616647 is fixed.
+  std::vector<SBPrefix> pver3_prefixes;
+  for (const auto& prefix : prefixes) {
+    // This code assumes 4-byte hash prefixes but PVer4 prefixes can be longer.
+    SBPrefix pver3_prefix =
+        prefix[0] | prefix[1] << 8 | prefix[2] << 16 | prefix[3] << 24;
+    pver3_prefixes.push_back(pver3_prefix);
+  }
+  std::vector<PlatformType> platforms_list(platforms.begin(), platforms.end());
+
+  GetFullHashes(pver3_prefixes, platforms_list, *(threat_types.begin()),
+                callback);

[Scott Hess - ex-Googler, 2016/08/23 20:25:46]

I'm a tiny bit nervous about DCHECK(!thread_types.empty()) plus
*(thread_types.begin()).  If someone breaks the contract in production, then
that becomes an undefined something.  Upgrading to a CHECK() would make sense to
me (though in the long term DCHECK makes more sense).

+}
+
+void V4GetHashProtocolManager::GetFullHashes(
     const std::vector<SBPrefix>& prefixes,
     const std::vector<PlatformType>& platforms,
     ThreatType threat_type,
     FullHashCallback callback) {
   DCHECK(CalledOnValidThread());
   // We need to wait the minimum waiting duration, and if we are in backoff,
   // we need to check if we're past the next allowed time. If we are, we can
   // proceed with the request. If not, we are required to return empty results
   // (i.e. treat the page as safe).
   if (clock_->Now() <= next_gethash_time_) {
@@ skipping 98 matching lines @@
 
 void V4GetHashProtocolManager::GetHashUrlAndHeaders(
     const std::string& req_base64,
     GURL* gurl,
     net::HttpRequestHeaders* headers) const {
   V4ProtocolManagerUtil::GetRequestUrlAndHeaders(req_base64, "fullHashes:find",
                                                  config_, gurl, headers);
 }
 
 }  // namespace safe_browsing