Move full hash caching logic to v4_get_hash_protocol_manager

components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc

Index: components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
diff --git a/components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc b/components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
index edf130ac9dc24a607fddf176e6e960b100cdfe42..6923ee0db0393b5de1ebbdbbdb3bef88690c9983 100644
--- a/components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
+++ b/components/safe_browsing_db/v4_get_hash_protocol_manager_unittest.cc
@@ -9,6 +9,7 @@
 
 #include "base/base64.h"
 #include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/simple_test_clock.h"
 #include "base/time/time.h"
@@ -19,7 +20,7 @@
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/test_url_fetcher_factory.h"
-#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
 
 using base::Time;
 using base::TimeDelta;
@@ -34,37 +35,77 @@ const char kKeyParam[] = "test_key_param";
 
 namespace safe_browsing {
 
-class SafeBrowsingV4GetHashProtocolManagerTest : public testing::Test {
- protected:
+namespace {
+
+struct KeyValue {
+  std::string key;
+  std::string value;
+
+  explicit KeyValue(const std::string key, const std::string value)
+      : key(key), value(value){};
+  explicit KeyValue(const KeyValue& other) = default;
+
+ private:
+  KeyValue();
+};
+
+struct ResponseInfo {
+  FullHash full_hash;
+  UpdateListIdentifier list_id;
+  std::vector<KeyValue> key_values;
+
+  explicit ResponseInfo(FullHash full_hash, UpdateListIdentifier list_id)
+      : full_hash(full_hash), list_id(list_id){};
+  explicit ResponseInfo(const ResponseInfo& other)
+      : full_hash(other.full_hash),
+        list_id(other.list_id),
+        key_values(other.key_values){};
+
+ private:
+  ResponseInfo();
+};
+
+}  // namespace
+
+class V4GetHashProtocolManagerTest : public PlatformTest {
+ public:
+  void SetUp() override {
+    PlatformTest::SetUp();
+    callback_called_ = false;
+  }
+
   std::unique_ptr<V4GetHashProtocolManager> CreateProtocolManager() {
     V4ProtocolConfig config;
     config.client_name = kClient;
     config.version = kAppVer;
     config.key_param = kKeyParam;
-    return std::unique_ptr<V4GetHashProtocolManager>(
-        V4GetHashProtocolManager::Create(NULL, config));
+    base::hash_set<UpdateListIdentifier> stores_to_look(
+        {GetUrlMalwareId(), GetChromeUrlApiId()});
+    return V4GetHashProtocolManager::Create(NULL, stores_to_look, config);
   }
 
-  std::string GetStockV4HashResponse() {
-    FindFullHashesResponse res;
-    res.mutable_negative_cache_duration()->set_seconds(600);
-    ThreatMatch* m = res.add_matches();
-    m->set_threat_type(API_ABUSE);
-    m->set_platform_type(CHROME_PLATFORM);
-    m->set_threat_entry_type(URL);
-    m->mutable_cache_duration()->set_seconds(300);
-    m->mutable_threat()->set_hash(
-        SBFullHashToString(SBFullHashForString("Everything's shiny, Cap'n.")));
-    ThreatEntryMetadata::MetadataEntry* e =
-        m->mutable_threat_entry_metadata()->add_entries();
-    e->set_key("permission");
-    e->set_value("NOTIFICATIONS");
+  static void SetupFetcherToReturnOKResponse(
+      const net::TestURLFetcherFactory& factory,
+      const std::vector<ResponseInfo>& infos) {
+    net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
+    DCHECK(fetcher);
+    fetcher->set_status(net::URLRequestStatus());
+    fetcher->set_response_code(200);
+    fetcher->SetResponseString(GetV4HashResponse(infos));
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+  }
 
-    // Serialize.
-    std::string res_data;
-    res.SerializeToString(&res_data);
+  static std::vector<ResponseInfo> GetStockV4HashResponseInfos() {
+    ResponseInfo info(FullHash("Everything's shiny, Cap'n."),
+                      GetChromeUrlApiId());
+    info.key_values.emplace_back("permission", "NOTIFICATIONS");
+    std::vector<ResponseInfo> infos;
+    infos.push_back(info);
+    return infos;
+  }
 
-    return res_data;
+  static std::string GetStockV4HashResponse() {
+    return GetV4HashResponse(GetStockV4HashResponseInfos());
   }
 
   void SetTestClock(base::Time now, V4GetHashProtocolManager* pm) {
@@ -72,37 +113,68 @@ class SafeBrowsingV4GetHashProtocolManagerTest : public testing::Test {
     clock->SetNow(now);
     pm->SetClockForTests(base::WrapUnique(clock));
   }
-};
 
-void ValidateGetV4HashResults(
-    const std::vector<SBFullHashResult>& expected_full_hashes,
-    const base::Time& expected_cache_expire,
-    const std::vector<SBFullHashResult>& full_hashes,
-    const base::Time& cache_expire) {
-  EXPECT_EQ(expected_cache_expire, cache_expire);
-  ASSERT_EQ(expected_full_hashes.size(), full_hashes.size());
-
-  for (unsigned int i = 0; i < expected_full_hashes.size(); ++i) {
-    const SBFullHashResult& expected = expected_full_hashes[i];
-    const SBFullHashResult& actual = full_hashes[i];
-    EXPECT_TRUE(SBFullHashEqual(expected.hash, actual.hash));
-    EXPECT_EQ(expected.metadata, actual.metadata);
-    EXPECT_EQ(expected.cache_expire_after, actual.cache_expire_after);
+  void ValidateGetV4ApiResults(const ThreatMetadata& expected_md,
+                               const ThreatMetadata& actual_md) {
+    EXPECT_EQ(expected_md, actual_md);
+    callback_called_ = true;
   }
-}
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
-       TestGetHashErrorHandlingNetwork) {
+  void ValidateGetV4HashResults(
+      const std::vector<FullHashInfo>& expected_results,
+      const std::vector<FullHashInfo>& actual_results) {
+    EXPECT_EQ(expected_results.size(), actual_results.size());
+    for (size_t i = 0; i < actual_results.size(); i++) {
+      EXPECT_TRUE(expected_results[i] == actual_results[i]);
+    }
+    callback_called_ = true;
+  }
+
+  bool callback_called() { return callback_called_; }
+
+ private:
+  static std::string GetV4HashResponse(
+      std::vector<ResponseInfo> response_infos) {
+    FindFullHashesResponse res;
+    res.mutable_negative_cache_duration()->set_seconds(600);
+    for (const ResponseInfo& info : response_infos) {
+      ThreatMatch* m = res.add_matches();
+      m->set_platform_type(info.list_id.platform_type);
+      m->set_threat_entry_type(info.list_id.threat_entry_type);
+      m->set_threat_type(info.list_id.threat_type);
+      m->mutable_cache_duration()->set_seconds(300);
+      m->mutable_threat()->set_hash(info.full_hash);
+
+      for (const KeyValue& key_value : info.key_values) {
+        ThreatEntryMetadata::MetadataEntry* e =
+            m->mutable_threat_entry_metadata()->add_entries();
+        e->set_key(key_value.key);
+        e->set_value(key_value.value);
+      }
+    }
+
+    // Serialize.
+    std::string res_data;
+    res.SerializeToString(&res_data);
+
+    return res_data;
+  }
+
+  bool callback_called_;
+};
+
+TEST_F(V4GetHashProtocolManagerTest, TestGetHashErrorHandlingNetwork) {
   net::TestURLFetcherFactory factory;
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
-  std::vector<SBPrefix> prefixes;
-  std::vector<SBFullHashResult> expected_full_hashes;
-  base::Time expected_cache_expire;
-
-  pm->GetFullHashesWithApis(
-      prefixes, base::Bind(&ValidateGetV4HashResults, expected_full_hashes,
-                           expected_cache_expire));
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;

[Nathan Parker, 2016/09/09 21:26:22]

nit: This is a long type name and a long var name.  Could you make the var name
shorter?  It gets hard to read.  Here and below

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

If you don't mind it too much, I actually prefer the type names to be a bit
wordy if they represent something that's otherwise difficult to convey using a
shorter name.
Changed the var name.

+  full_hash_to_store_and_hash_prefixes[FullHash("AFullHash")].push_back(
+      StoreAndHashPrefix(GetUrlSocEngId(), HashPrefix("AHashPrefix")));

[Nathan Parker, 2016/09/09 21:26:22]

Is it ok to have the prefix not match the full hash that in the map key?

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

In general not. But in this case it is irrelevant because the response from the
server is that of error. However, in the spirit of keeping it consistent, I've
changed it.

+  std::vector<FullHashInfo> expected_results;
+  pm->GetFullHashes(
+      full_hash_to_store_and_hash_prefixes,
+      base::Bind(&V4GetHashProtocolManagerTest::ValidateGetV4HashResults,
+                 base::Unretained(this), expected_results));
 
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   DCHECK(fetcher);
@@ -116,20 +188,21 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   // Should have recorded one error, but back off multiplier is unchanged.
   EXPECT_EQ(1ul, pm->gethash_error_count_);
   EXPECT_EQ(1ul, pm->gethash_back_off_mult_);
+  EXPECT_TRUE(callback_called());
 }
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
-       TestGetHashErrorHandlingResponseCode) {
+TEST_F(V4GetHashProtocolManagerTest, TestGetHashErrorHandlingResponseCode) {
   net::TestURLFetcherFactory factory;
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
-  std::vector<SBPrefix> prefixes;
-  std::vector<SBFullHashResult> expected_full_hashes;
-  base::Time expected_cache_expire;
-
-  pm->GetFullHashesWithApis(
-      prefixes, base::Bind(&ValidateGetV4HashResults, expected_full_hashes,
-                           expected_cache_expire));
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
+  full_hash_to_store_and_hash_prefixes[FullHash("AFullHash")].push_back(
+      StoreAndHashPrefix(GetUrlSocEngId(), HashPrefix("AHashPrefix")));
+  std::vector<FullHashInfo> expected_results;
+  pm->GetFullHashes(
+      full_hash_to_store_and_hash_prefixes,
+      base::Bind(&V4GetHashProtocolManagerTest::ValidateGetV4HashResults,
+                 base::Unretained(this), expected_results));
 
   net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
   DCHECK(fetcher);
@@ -142,81 +215,106 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   // Should have recorded one error, but back off multiplier is unchanged.
   EXPECT_EQ(1ul, pm->gethash_error_count_);
   EXPECT_EQ(1ul, pm->gethash_back_off_mult_);
+  EXPECT_TRUE(callback_called());
 }
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest, TestGetHashErrorHandlingOK) {
+TEST_F(V4GetHashProtocolManagerTest, TestGetHashErrorHandlingOK) {
   net::TestURLFetcherFactory factory;
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
   base::Time now = base::Time::UnixEpoch();
   SetTestClock(now, pm.get());
 
-  std::vector<SBPrefix> prefixes;
-  std::vector<SBFullHashResult> expected_full_hashes;
-  SBFullHashResult hash_result;
-  hash_result.hash = SBFullHashForString("Everything's shiny, Cap'n.");
-  hash_result.metadata.api_permissions.insert("NOTIFICATIONS");
-  hash_result.cache_expire_after = now + base::TimeDelta::FromSeconds(300);
-  expected_full_hashes.push_back(hash_result);
-  base::Time expected_cache_expire = now + base::TimeDelta::FromSeconds(600);
+  HashPrefix prefix("Everything");
+  FullHash full_hash("Everything's shiny, Cap'n.");
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
+  full_hash_to_store_and_hash_prefixes[full_hash].push_back(
+      StoreAndHashPrefix(GetChromeUrlApiId(), prefix));
+  std::vector<FullHashInfo> expected_results;
+  FullHashInfo fhi(full_hash, GetChromeUrlApiId(),
+                   now + base::TimeDelta::FromSeconds(300));
+  fhi.metadata.api_permissions.insert("NOTIFICATIONS");
+  expected_results.push_back(fhi);
 
-  pm->GetFullHashesWithApis(
-      prefixes, base::Bind(&ValidateGetV4HashResults, expected_full_hashes,
-                           expected_cache_expire));
+  pm->GetFullHashes(
+      full_hash_to_store_and_hash_prefixes,
+      base::Bind(&V4GetHashProtocolManagerTest::ValidateGetV4HashResults,
+                 base::Unretained(this), expected_results));
 
-  net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
-  DCHECK(fetcher);
-  fetcher->set_status(net::URLRequestStatus());
-  fetcher->set_response_code(200);
-  fetcher->SetResponseString(GetStockV4HashResponse());
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  SetupFetcherToReturnOKResponse(factory, GetStockV4HashResponseInfos());
 
   // No error, back off multiplier is unchanged.
   EXPECT_EQ(0ul, pm->gethash_error_count_);
   EXPECT_EQ(1ul, pm->gethash_back_off_mult_);
+
+  // Verify the state of the cache.
+  const FullHashCache* cache = pm->full_hash_cache_for_tests();
+  // Check the cache.
+  ASSERT_EQ(1u, cache->size());
+  EXPECT_EQ(1u, cache->count(prefix));
+  const CachedHashPrefixInfo& cached_result = cache->at(prefix);
+  EXPECT_EQ(cached_result.negative_ttl,
+            now + base::TimeDelta::FromSeconds(600));
+  ASSERT_EQ(1u, cached_result.full_hash_infos.size());
+  EXPECT_EQ(FullHash("Everything's shiny, Cap'n."),
+            cached_result.full_hash_infos[0].full_hash);
+  EXPECT_TRUE(callback_called());
 }
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest, TestGetHashRequest) {
+TEST_F(V4GetHashProtocolManagerTest,
+       TestResultsNotCachedForNegativeCacheDuration) {
+  net::TestURLFetcherFactory factory;
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
+  HashPrefix prefix("Everything");
+  std::vector<HashPrefix> prefixes_requested({prefix});
+  base::Time negative_cache_expire;
+  FullHash full_hash("Everything's shiny, Cap'n.");
+  std::vector<FullHashInfo> fhis;
+  fhis.emplace_back(full_hash, GetChromeUrlApiId(), base::Time::UnixEpoch());
+
+  pm->UpdateCache(prefixes_requested, fhis, negative_cache_expire);
+
+  // Verify the state of the cache.
+  const FullHashCache* cache = pm->full_hash_cache_for_tests();
+  // Check the cache.
+  EXPECT_EQ(0u, cache->size());
+}
+
+TEST_F(V4GetHashProtocolManagerTest, TestGetHashRequest) {
+  HashPrefix one = "hashone";
+  HashPrefix two = "hashtwo";
+  std::vector<HashPrefix> prefixes_to_request = {one, two};
+
   FindFullHashesRequest req;
   ThreatInfo* info = req.mutable_threat_info();
+  info->add_threat_types(MALWARE_THREAT);
   info->add_threat_types(API_ABUSE);
+
+  info->add_platform_types(GetCurrentPlatformType());
   info->add_platform_types(CHROME_PLATFORM);
+
   info->add_threat_entry_types(URL);
 
-  SBPrefix one = 1u;
-  SBPrefix two = 2u;
-  SBPrefix three = 3u;
-  std::string hash(reinterpret_cast<const char*>(&one), sizeof(SBPrefix));
-  info->add_threat_entries()->set_hash(hash);
-  hash.clear();
-  hash.append(reinterpret_cast<const char*>(&two), sizeof(SBPrefix));
-  info->add_threat_entries()->set_hash(hash);
-  hash.clear();
-  hash.append(reinterpret_cast<const char*>(&three), sizeof(SBPrefix));
-  info->add_threat_entries()->set_hash(hash);
+  info->add_threat_entries()->set_hash(one);
+  info->add_threat_entries()->set_hash(two);
 
   // Serialize and Base64 encode.
   std::string req_data, req_base64;
   req.SerializeToString(&req_data);
   base::Base64Encode(req_data, &req_base64);
 
-  std::vector<PlatformType> platform;
-  platform.push_back(CHROME_PLATFORM);
-  std::vector<SBPrefix> prefixes;
-  prefixes.push_back(one);
-  prefixes.push_back(two);
-  prefixes.push_back(three);
-  EXPECT_EQ(req_base64, pm->GetHashRequest(prefixes, platform, API_ABUSE));
+  std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
+  EXPECT_EQ(req_base64, pm->GetHashRequest(prefixes_to_request));
 }
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest, TestParseHashResponse) {
+TEST_F(V4GetHashProtocolManagerTest, TestParseHashResponse) {
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
   base::Time now = base::Time::UnixEpoch();
   SetTestClock(now, pm.get());
 
+  FullHash full_hash("Everything's shiny, Cap'n.");
   FindFullHashesResponse res;
   res.mutable_negative_cache_duration()->set_seconds(600);
   res.mutable_minimum_wait_duration()->set_seconds(400);
@@ -225,8 +323,7 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest, TestParseHashResponse) {
   m->set_platform_type(CHROME_PLATFORM);
   m->set_threat_entry_type(URL);
   m->mutable_cache_duration()->set_seconds(300);
-  m->mutable_threat()->set_hash(
-      SBFullHashToString(SBFullHashForString("Everything's shiny, Cap'n.")));
+  m->mutable_threat()->set_hash(full_hash);
   ThreatEntryMetadata::MetadataEntry* e =
       m->mutable_threat_entry_metadata()->add_entries();
   e->set_key("permission");
@@ -236,24 +333,23 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest, TestParseHashResponse) {
   std::string res_data;
   res.SerializeToString(&res_data);
 
-  std::vector<SBFullHashResult> full_hashes;
+  std::vector<FullHashInfo> full_hash_infos;
   base::Time cache_expire;
-  EXPECT_TRUE(pm->ParseHashResponse(res_data, &full_hashes, &cache_expire));
+  EXPECT_TRUE(pm->ParseHashResponse(res_data, &full_hash_infos, &cache_expire));
 
   EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
-  EXPECT_EQ(1ul, full_hashes.size());
-  EXPECT_TRUE(SBFullHashEqual(SBFullHashForString("Everything's shiny, Cap'n."),
-                              full_hashes[0].hash));
-  EXPECT_EQ(1ul, full_hashes[0].metadata.api_permissions.size());
-  EXPECT_EQ(1ul,
-            full_hashes[0].metadata.api_permissions.count("NOTIFICATIONS"));
-  EXPECT_EQ(now +
-      base::TimeDelta::FromSeconds(300), full_hashes[0].cache_expire_after);
+  ASSERT_EQ(1ul, full_hash_infos.size());
+  const FullHashInfo& fhi = full_hash_infos[0];
+  EXPECT_EQ(full_hash, fhi.full_hash);
+  EXPECT_EQ(GetChromeUrlApiId(), fhi.list_id);
+  EXPECT_EQ(1ul, fhi.metadata.api_permissions.size());
+  EXPECT_EQ(1ul, fhi.metadata.api_permissions.count("NOTIFICATIONS"));
+  EXPECT_EQ(now + base::TimeDelta::FromSeconds(300), fhi.positive_ttl);
   EXPECT_EQ(now + base::TimeDelta::FromSeconds(400), pm->next_gethash_time_);
 }
 
 // Adds an entry with an ignored ThreatEntryType.
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
+TEST_F(V4GetHashProtocolManagerTest,
        TestParseHashResponseWrongThreatEntryType) {
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
@@ -268,96 +364,117 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   std::string res_data;
   res.SerializeToString(&res_data);
 
-  std::vector<SBFullHashResult> full_hashes;
+  std::vector<FullHashInfo> full_hash_infos;
   base::Time cache_expire;
-  EXPECT_FALSE(pm->ParseHashResponse(res_data, &full_hashes, &cache_expire));
+  EXPECT_FALSE(
+      pm->ParseHashResponse(res_data, &full_hash_infos, &cache_expire));
 
   EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
   // There should be no hash results.
-  EXPECT_EQ(0ul, full_hashes.size());
+  EXPECT_EQ(0ul, full_hash_infos.size());
 }
 
 // Adds entries with a ThreatPatternType metadata.
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
-       TestParseHashThreatPatternType) {
+TEST_F(V4GetHashProtocolManagerTest, TestParseHashThreatPatternType) {
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
   base::Time now = base::Time::UnixEpoch();
   SetTestClock(now, pm.get());
 
-  // Test social engineering pattern type.
-  FindFullHashesResponse se_res;
-  se_res.mutable_negative_cache_duration()->set_seconds(600);
-  ThreatMatch* se = se_res.add_matches();
-  se->set_threat_type(SOCIAL_ENGINEERING_PUBLIC);
-  se->set_platform_type(CHROME_PLATFORM);
-  se->set_threat_entry_type(URL);
-  SBFullHash hash_string = SBFullHashForString("Everything's shiny, Cap'n.");
-  se->mutable_threat()->set_hash(SBFullHashToString(hash_string));
-  ThreatEntryMetadata::MetadataEntry* se_meta =
-      se->mutable_threat_entry_metadata()->add_entries();
-  se_meta->set_key("se_pattern_type");
-  se_meta->set_value("SOCIAL_ENGINEERING_LANDING");
-
-  std::string se_data;
-  se_res.SerializeToString(&se_data);
-
-  std::vector<SBFullHashResult> full_hashes;
-  base::Time cache_expire;
-  EXPECT_TRUE(pm->ParseHashResponse(se_data, &full_hashes, &cache_expire));
+  {

[Nathan Parker, 2016/09/09 21:26:22]

I'm glad you scoped these separately, and I think you could go a step further
and make them separate tests since they aren't dependent on each other.

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

I'm usually in favor of keeping tests small and focused.

In this case, since they are doing a related thing, it might be easier for a
future developer to get the big picture of what they are testing. I think
there's some value in keeping these together.

+    // Test social engineering pattern type.
+    FindFullHashesResponse se_res;
+    se_res.mutable_negative_cache_duration()->set_seconds(600);
+    ThreatMatch* se = se_res.add_matches();
+    se->set_threat_type(SOCIAL_ENGINEERING_PUBLIC);
+    se->set_platform_type(CHROME_PLATFORM);
+    se->set_threat_entry_type(URL);
+    FullHash full_hash("Everything's shiny, Cap'n.");
+    se->mutable_threat()->set_hash(full_hash);
+    ThreatEntryMetadata::MetadataEntry* se_meta =
+        se->mutable_threat_entry_metadata()->add_entries();
+    se_meta->set_key("se_pattern_type");
+    se_meta->set_value("SOCIAL_ENGINEERING_LANDING");
+
+    std::string se_data;
+    se_res.SerializeToString(&se_data);
+
+    std::vector<FullHashInfo> full_hash_infos;
+    base::Time cache_expire;
+    EXPECT_TRUE(
+        pm->ParseHashResponse(se_data, &full_hash_infos, &cache_expire));
+    EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
+
+    ASSERT_EQ(1ul, full_hash_infos.size());
+    const FullHashInfo& fhi = full_hash_infos[0];
+    EXPECT_EQ(full_hash, fhi.full_hash);
+    const UpdateListIdentifier list_id(CHROME_PLATFORM, URL,
+                                       SOCIAL_ENGINEERING_PUBLIC);
+    EXPECT_EQ(list_id, fhi.list_id);
+    EXPECT_EQ(ThreatPatternType::SOCIAL_ENGINEERING_LANDING,
+              fhi.metadata.threat_pattern_type);
+  }
 
-  EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
-  EXPECT_EQ(1ul, full_hashes.size());
-  EXPECT_TRUE(SBFullHashEqual(hash_string, full_hashes[0].hash));
-  EXPECT_EQ(ThreatPatternType::SOCIAL_ENGINEERING_LANDING,
-            full_hashes[0].metadata.threat_pattern_type);
-
-  // Test potentially harmful application pattern type.
-  FindFullHashesResponse pha_res;
-  pha_res.mutable_negative_cache_duration()->set_seconds(600);
-  ThreatMatch* pha = pha_res.add_matches();
-  pha->set_threat_type(POTENTIALLY_HARMFUL_APPLICATION);
-  pha->set_threat_entry_type(URL);
-  pha->set_platform_type(CHROME_PLATFORM);
-  hash_string = SBFullHashForString("Not to fret.");
-  pha->mutable_threat()->set_hash(SBFullHashToString(hash_string));
-  ThreatEntryMetadata::MetadataEntry* pha_meta =
-      pha->mutable_threat_entry_metadata()->add_entries();
-  pha_meta->set_key("pha_pattern_type");
-  pha_meta->set_value("LANDING");
-
-  std::string pha_data;
-  pha_res.SerializeToString(&pha_data);
-  full_hashes.clear();
-  EXPECT_TRUE(pm->ParseHashResponse(pha_data, &full_hashes, &cache_expire));
-  EXPECT_EQ(1ul, full_hashes.size());
-  EXPECT_TRUE(SBFullHashEqual(hash_string, full_hashes[0].hash));
-  EXPECT_EQ(ThreatPatternType::MALWARE_LANDING,
-            full_hashes[0].metadata.threat_pattern_type);
-
-  // Test invalid pattern type.
-  FindFullHashesResponse invalid_res;
-  invalid_res.mutable_negative_cache_duration()->set_seconds(600);
-  ThreatMatch* invalid = invalid_res.add_matches();
-  invalid->set_threat_type(POTENTIALLY_HARMFUL_APPLICATION);
-  invalid->set_threat_entry_type(URL);
-  invalid->set_platform_type(CHROME_PLATFORM);
-  invalid->mutable_threat()->set_hash(SBFullHashToString(hash_string));
-  ThreatEntryMetadata::MetadataEntry* invalid_meta =
-      invalid->mutable_threat_entry_metadata()->add_entries();
-  invalid_meta->set_key("pha_pattern_type");
-  invalid_meta->set_value("INVALIDE_VALUE");
-
-  std::string invalid_data;
-  invalid_res.SerializeToString(&invalid_data);
-  full_hashes.clear();
-  EXPECT_FALSE(
-      pm->ParseHashResponse(invalid_data, &full_hashes, &cache_expire));
-  EXPECT_EQ(0ul, full_hashes.size());
+  {
+    // Test potentially harmful application pattern type.
+    FindFullHashesResponse pha_res;
+    pha_res.mutable_negative_cache_duration()->set_seconds(600);
+    ThreatMatch* pha = pha_res.add_matches();
+    pha->set_threat_type(POTENTIALLY_HARMFUL_APPLICATION);
+    pha->set_threat_entry_type(URL);
+    pha->set_platform_type(CHROME_PLATFORM);
+    FullHash full_hash("Not to fret.");
+    pha->mutable_threat()->set_hash(full_hash);
+    ThreatEntryMetadata::MetadataEntry* pha_meta =
+        pha->mutable_threat_entry_metadata()->add_entries();
+    pha_meta->set_key("pha_pattern_type");
+    pha_meta->set_value("LANDING");
+
+    std::string pha_data;
+    pha_res.SerializeToString(&pha_data);
+    std::vector<FullHashInfo> full_hash_infos;
+    base::Time cache_expire;
+    EXPECT_TRUE(
+        pm->ParseHashResponse(pha_data, &full_hash_infos, &cache_expire));
+    EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
+
+    ASSERT_EQ(1ul, full_hash_infos.size());
+    const FullHashInfo& fhi = full_hash_infos[0];
+    EXPECT_EQ(full_hash, fhi.full_hash);
+    const UpdateListIdentifier list_id(CHROME_PLATFORM, URL,
+                                       POTENTIALLY_HARMFUL_APPLICATION);
+    EXPECT_EQ(list_id, fhi.list_id);
+    EXPECT_EQ(ThreatPatternType::MALWARE_LANDING,
+              fhi.metadata.threat_pattern_type);
+  }
+
+  {
+    // Test invalid pattern type.
+    FullHash full_hash("Not to fret.");
+    FindFullHashesResponse invalid_res;
+    invalid_res.mutable_negative_cache_duration()->set_seconds(600);
+    ThreatMatch* invalid = invalid_res.add_matches();
+    invalid->set_threat_type(POTENTIALLY_HARMFUL_APPLICATION);
+    invalid->set_threat_entry_type(URL);
+    invalid->set_platform_type(CHROME_PLATFORM);
+    invalid->mutable_threat()->set_hash(full_hash);
+    ThreatEntryMetadata::MetadataEntry* invalid_meta =
+        invalid->mutable_threat_entry_metadata()->add_entries();
+    invalid_meta->set_key("pha_pattern_type");
+    invalid_meta->set_value("INVALIDE_VALUE");
+
+    std::string invalid_data;
+    invalid_res.SerializeToString(&invalid_data);
+    std::vector<FullHashInfo> full_hash_infos;
+    base::Time cache_expire;
+    EXPECT_FALSE(
+        pm->ParseHashResponse(invalid_data, &full_hash_infos, &cache_expire));
+    EXPECT_EQ(0ul, full_hash_infos.size());
+  }
 }
 
 // Adds metadata with a key value that is not "permission".
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
+TEST_F(V4GetHashProtocolManagerTest,
        TestParseHashResponseNonPermissionMetadata) {
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
@@ -370,8 +487,7 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   m->set_threat_type(API_ABUSE);
   m->set_platform_type(CHROME_PLATFORM);
   m->set_threat_entry_type(URL);
-  m->mutable_threat()->set_hash(
-      SBFullHashToString(SBFullHashForString("Not to fret.")));
+  m->mutable_threat()->set_hash(FullHash("Not to fret."));
   ThreatEntryMetadata::MetadataEntry* e =
       m->mutable_threat_entry_metadata()->add_entries();
   e->set_key("notpermission");
@@ -381,15 +497,16 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   std::string res_data;
   res.SerializeToString(&res_data);
 
-  std::vector<SBFullHashResult> full_hashes;
+  std::vector<FullHashInfo> full_hash_infos;
   base::Time cache_expire;
-  EXPECT_FALSE(pm->ParseHashResponse(res_data, &full_hashes, &cache_expire));
+  EXPECT_FALSE(
+      pm->ParseHashResponse(res_data, &full_hash_infos, &cache_expire));
 
   EXPECT_EQ(now + base::TimeDelta::FromSeconds(600), cache_expire);
-  EXPECT_EQ(0ul, full_hashes.size());
+  EXPECT_EQ(0ul, full_hash_infos.size());
 }
 
-TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
+TEST_F(V4GetHashProtocolManagerTest,
        TestParseHashResponseInconsistentThreatTypes) {
   std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
 
@@ -399,22 +516,194 @@ TEST_F(SafeBrowsingV4GetHashProtocolManagerTest,
   m1->set_threat_type(API_ABUSE);
   m1->set_platform_type(CHROME_PLATFORM);
   m1->set_threat_entry_type(URL);
-  m1->mutable_threat()->set_hash(
-      SBFullHashToString(SBFullHashForString("Everything's shiny, Cap'n.")));
+  m1->mutable_threat()->set_hash(FullHash("Everything's shiny, Cap'n."));
   m1->mutable_threat_entry_metadata()->add_entries();
   ThreatMatch* m2 = res.add_matches();
   m2->set_threat_type(MALWARE_THREAT);
   m2->set_threat_entry_type(URL);
-  m2->mutable_threat()->set_hash(
-      SBFullHashToString(SBFullHashForString("Not to fret.")));
+  m2->mutable_threat()->set_hash(FullHash("Not to fret."));
 
   // Serialize.
   std::string res_data;
   res.SerializeToString(&res_data);
 
-  std::vector<SBFullHashResult> full_hashes;
+  std::vector<FullHashInfo> full_hash_infos;
   base::Time cache_expire;
-  EXPECT_FALSE(pm->ParseHashResponse(res_data, &full_hashes, &cache_expire));
+  EXPECT_FALSE(
+      pm->ParseHashResponse(res_data, &full_hash_infos, &cache_expire));
+}
+
+// Checks that results are looked up correctly in the cache.
+TEST_F(V4GetHashProtocolManagerTest, GetCachedResults) {

[Nathan Parker, 2016/09/09 21:26:22]

An overall comment: I think it would be less fragile to use methods on *pm to
store things in the cache rather than modifying its internal state.  That would
also verify that when things get cached, their TTL/etc are written correctly.

I'm torn on if this is important enough to rewrite these tests -- I'll let you
decide.

+  base::Time now = base::Time::UnixEpoch();
+  FullHash full_hash("example.com/");

[Nathan Parker, 2016/09/09 21:26:22]

nit: A url-as-hash is sortof confusing

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  HashPrefix prefix("exam");
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
+  std::vector<HashPrefix> prefixes_to_request;
+  std::vector<FullHashInfo> cached_full_hash_infos;
+  full_hash_to_store_and_hash_prefixes[full_hash].push_back(
+      StoreAndHashPrefix(GetUrlMalwareId(), prefix));
+  std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
+  FullHashCache* cache = pm->full_hash_cache_for_tests();
+
+  // Test with an empty cache. (Case: 2)
+  pm->GetFullHashCachedResults(full_hash_to_store_and_hash_prefixes, now,
+                               &prefixes_to_request, &cached_full_hash_infos);
+  EXPECT_TRUE(cache->empty());
+  ASSERT_EQ(1ul, prefixes_to_request.size());
+  EXPECT_EQ(prefix, prefixes_to_request[0]);
+  EXPECT_TRUE(cached_full_hash_infos.empty());
+
+  // Prefix has a cache entry but full hash is not there. (Case: 1-b-i)
+  CachedHashPrefixInfo* entry = &(*cache)[prefix];

[Nathan Parker, 2016/09/09 21:26:21]

Do all these cases need to be in one test? Could you split them into separate
tests, all calling a common setup function?   It's pretty hard to tell how they
interact, and what the state of the cache is at each step (since it's the
cumulative affect of all preceding lines).

A possible middleground: Clear out *entry at the start of each sub-test.

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Scoped each of the cases, and clear the cache before each one. These tests are
doing related stuff so it is good to be able to keep the close to each other to
spot the differences easily.

+  entry->negative_ttl = now + base::TimeDelta::FromMinutes(5);
+  prefixes_to_request.clear();
+  cached_full_hash_infos.clear();
+  pm->GetFullHashCachedResults(full_hash_to_store_and_hash_prefixes, now,
+                               &prefixes_to_request, &cached_full_hash_infos);
+  EXPECT_TRUE(prefixes_to_request.empty());
+  EXPECT_TRUE(cached_full_hash_infos.empty());
+
+  // Expired negative cache entry. (Case: 1-b-ii)
+  entry->negative_ttl = now - base::TimeDelta::FromMinutes(5);
+  prefixes_to_request.clear();
+  cached_full_hash_infos.clear();
+  pm->GetFullHashCachedResults(full_hash_to_store_and_hash_prefixes, now,
+                               &prefixes_to_request, &cached_full_hash_infos);
+  ASSERT_EQ(1ul, prefixes_to_request.size());
+  EXPECT_EQ(prefix, prefixes_to_request[0]);
+  EXPECT_TRUE(cached_full_hash_infos.empty());
+
+  // Now put unexpired full hash in the cache. (Case: 1-a-i)
+  FullHashInfo fhi(full_hash, GetUrlMalwareId(),
+                   now + base::TimeDelta::FromMinutes(3));
+  entry->full_hash_infos.push_back(fhi);
+  prefixes_to_request.clear();
+  cached_full_hash_infos.clear();
+  pm->GetFullHashCachedResults(full_hash_to_store_and_hash_prefixes, now,
+                               &prefixes_to_request, &cached_full_hash_infos);
+  EXPECT_TRUE(prefixes_to_request.empty());
+  ASSERT_EQ(1ul, cached_full_hash_infos.size());
+  EXPECT_EQ(full_hash, cached_full_hash_infos[0].full_hash);
+
+  // Expire the full hash in the cache. (Case: 1-a-ii)
+  entry->full_hash_infos[0].positive_ttl =
+      now - base::TimeDelta::FromMinutes(3);
+  prefixes_to_request.clear();
+  cached_full_hash_infos.clear();
+  pm->GetFullHashCachedResults(full_hash_to_store_and_hash_prefixes, now,
+                               &prefixes_to_request, &cached_full_hash_infos);
+  ASSERT_EQ(1ul, prefixes_to_request.size());
+  EXPECT_EQ(prefix, prefixes_to_request[0]);
+  EXPECT_TRUE(cached_full_hash_infos.empty());
+}
+
+TEST_F(V4GetHashProtocolManagerTest, TestUpdatesAreMerged) {
+  // We'll add one of the requested FullHashInfo objects into the cache, and
+  // inject the other one as a response from the server. The result should
+  // include both FullHashInfo objects.
+
+  net::TestURLFetcherFactory factory;
+  std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
+  HashPrefix prefix_1("exam");
+  FullHash full_hash_1("example.com/test");

[Nathan Parker, 2016/09/09 21:26:22]

nit: url-as-hash is confusing

[vakh (use Gerrit instead), 2016/09/09 23:25:18]

Done.

+  HashPrefix prefix_2("Everything");
+  FullHash full_hash_2("Everything's shiny, Cap'n.");
+
+  base::Time now = Time::Now();
+  SetTestClock(now, pm.get());
+
+  FullHashCache* cache = pm->full_hash_cache_for_tests();
+  CachedHashPrefixInfo* entry = &(*cache)[prefix_1];
+  entry->negative_ttl = now + base::TimeDelta::FromMinutes(100);
+  // Put one unexpired full hash in the cache for a store we'll look in.
+  entry->full_hash_infos.emplace_back(full_hash_1, GetUrlMalwareId(),
+                                      now + base::TimeDelta::FromSeconds(200));
+  // Put one unexpired full hash in the cache for a store we'll not look in.
+  entry->full_hash_infos.emplace_back(full_hash_1, GetUrlSocEngId(),
+                                      now + base::TimeDelta::FromSeconds(200));
+
+  // Request full hash information from two stores.
+  FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
+  full_hash_to_store_and_hash_prefixes[full_hash_1].push_back(
+      StoreAndHashPrefix(GetUrlMalwareId(), prefix_1));
+  full_hash_to_store_and_hash_prefixes[full_hash_2].push_back(
+      StoreAndHashPrefix(GetChromeUrlApiId(), prefix_2));
+
+  // Expect full hash information from both stores.
+  std::vector<FullHashInfo> expected_results;
+  expected_results.emplace_back(full_hash_1, GetUrlMalwareId(),
+                                now + base::TimeDelta::FromSeconds(200));
+  expected_results.emplace_back(full_hash_2, GetChromeUrlApiId(),
+                                now + base::TimeDelta::FromSeconds(300));
+  expected_results[1].metadata.api_permissions.insert("NOTIFICATIONS");
+
+  pm->GetFullHashes(
+      full_hash_to_store_and_hash_prefixes,
+      base::Bind(&V4GetHashProtocolManagerTest::ValidateGetV4HashResults,
+                 base::Unretained(this), expected_results));
+
+  SetupFetcherToReturnOKResponse(factory, GetStockV4HashResponseInfos());
+
+  // No error, back off multiplier is unchanged.
+  EXPECT_EQ(0ul, pm->gethash_error_count_);
+  EXPECT_EQ(1ul, pm->gethash_back_off_mult_);
+
+  // Verify the state of the cache.
+  ASSERT_EQ(2u, cache->size());
+  const CachedHashPrefixInfo& cached_result_1 = cache->at(prefix_1);
+  EXPECT_EQ(cached_result_1.negative_ttl,
+            now + base::TimeDelta::FromMinutes(100));
+  ASSERT_EQ(2u, cached_result_1.full_hash_infos.size());
+  EXPECT_EQ(full_hash_1, cached_result_1.full_hash_infos[0].full_hash);
+  EXPECT_EQ(GetUrlMalwareId(), cached_result_1.full_hash_infos[0].list_id);
+
+  const CachedHashPrefixInfo& cached_result_2 = cache->at(prefix_2);
+  EXPECT_EQ(cached_result_2.negative_ttl,
+            now + base::TimeDelta::FromSeconds(600));
+  ASSERT_EQ(1u, cached_result_2.full_hash_infos.size());
+  EXPECT_EQ(full_hash_2, cached_result_2.full_hash_infos[0].full_hash);
+  EXPECT_EQ(GetChromeUrlApiId(), cached_result_2.full_hash_infos[0].list_id);
+  EXPECT_TRUE(callback_called());
+}
+
+// The server responds back with full hash information containing metadata
+// information for one of the full hashes for the URL in test.
+TEST_F(V4GetHashProtocolManagerTest, TestGetFullHashesWithApisMergesMetadata) {
+  net::TestURLFetcherFactory factory;
+  const GURL url("https://www.example.com/more");
+  ThreatMetadata expected_md;
+  expected_md.api_permissions.insert("NOTIFICATIONS");
+  expected_md.api_permissions.insert("AUDIO_CAPTURE");
+  std::unique_ptr<V4GetHashProtocolManager> pm(CreateProtocolManager());
+  pm->GetFullHashesWithApis(
+      url, base::Bind(&V4GetHashProtocolManagerTest::ValidateGetV4ApiResults,
+                      base::Unretained(this), expected_md));
+
+  // The following two random looking strings value are two of the full hashes
+  // produced by UrlToFullHashes in v4_protocol_manager_util.h for the URL:
+  // "https://www.example.com/more"
+  std::vector<ResponseInfo> infos;
+  FullHash full_hash;
+  base::Base64Decode("1ZzJ0/7NjPkg6t0DAS8L5Jf7jA48Pn7opQcP4UXYeXc=",
+                     &full_hash);
+  ResponseInfo info(full_hash, GetChromeUrlApiId());
+  info.key_values.emplace_back("permission", "NOTIFICATIONS");
+  infos.push_back(info);
+
+  base::Base64Decode("4rPDSdcei1BiCOPnj9kgsy2O6Ua6X3iFBakqphB3ZfA=",
+                     &full_hash);
+  info = ResponseInfo(full_hash, GetChromeUrlApiId());
+  info.key_values.emplace_back("permission", "AUDIO_CAPTURE");
+  infos.push_back(info);
+
+  full_hash = FullHash("Everything's shiny, Cap'n.");
+  info = ResponseInfo(full_hash, GetChromeUrlApiId());
+  info.key_values.emplace_back("permission", "GEOLOCATION");
+  infos.push_back(info);
+  SetupFetcherToReturnOKResponse(factory, infos);
+
+  EXPECT_TRUE(callback_called());
 }
 
 }  // namespace safe_browsing