Report downloaded resources at most once

components/precache/core/precache_database_unittest.cc

Index: components/precache/core/precache_database_unittest.cc
diff --git a/components/precache/core/precache_database_unittest.cc b/components/precache/core/precache_database_unittest.cc
index 8d00c79f6f6c0c36bc49e985454d235f5c118dd3..333eb9783744f8d2a660322b65820c63696b46fb 100644
--- a/components/precache/core/precache_database_unittest.cc
+++ b/components/precache/core/precache_database_unittest.cc
@@ -9,6 +9,8 @@
 #include <map>
 #include <memory>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/containers/hash_tables.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
@@ -263,7 +265,7 @@ TEST_F(PrecacheDatabaseTest, PrecacheOverNetwork) {
 }
 
 TEST_F(PrecacheDatabaseTest, PrecacheFromCacheWithURLTableEntry) {
-  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime);
+  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime, false);
   RecordPrecacheFromCache(kURL, kFetchTime, kSize);
 
   // The URL table entry should have been updated to have |kFetchTime| as the
@@ -326,7 +328,7 @@ TEST_F(PrecacheDatabaseTest, FetchOverNetwork_Cellular) {
 }
 
 TEST_F(PrecacheDatabaseTest, FetchOverNetworkWithURLTableEntry) {
-  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime);
+  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime, false);
   RecordFetchFromNetwork(kURL, kLatency, kFetchTime, kSize);
 
   // The URL table entry should have been deleted.
@@ -343,7 +345,7 @@ TEST_F(PrecacheDatabaseTest, FetchOverNetworkWithURLTableEntry) {
 }
 
 TEST_F(PrecacheDatabaseTest, FetchFromCacheWithURLTableEntry_NonCellular) {
-  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime);
+  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime, false);
   RecordFetchFromCache(kURL, kFetchTime, kSize);
 
   // The URL table entry should have been deleted.
@@ -361,7 +363,7 @@ TEST_F(PrecacheDatabaseTest, FetchFromCacheWithURLTableEntry_NonCellular) {
 }
 
 TEST_F(PrecacheDatabaseTest, FetchFromCacheWithURLTableEntry_Cellular) {
-  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime);
+  precache_url_table()->AddURL(kURL, kReferrerID, true, kOldFetchTime, false);
   RecordFetchFromCacheCellular(kURL, kFetchTime, kSize);
 
   // The URL table entry should have been deleted.
@@ -397,9 +399,9 @@ TEST_F(PrecacheDatabaseTest, DeleteExpiredPrecacheHistory) {
   const base::Time k61DaysAgo = kToday - base::TimeDelta::FromDays(61);
 
   precache_url_table()->AddURL(GURL("http://expired-precache.com"), kReferrerID,
-                               true, k61DaysAgo);
+                               true, k61DaysAgo, false);
   precache_url_table()->AddURL(GURL("http://old-precache.com"), kReferrerID,
-                               true, k59DaysAgo);
+                               true, k59DaysAgo, false);
 
   precache_database_->DeleteExpiredPrecacheHistory(kToday);
 
@@ -533,6 +535,26 @@ TEST_F(PrecacheDatabaseTest, GetURLListForReferrerHost) {
                                                  kNewFetchTime);
   precache_database_->UpdatePrecacheReferrerHost("empty.com", 3, kNewFetchTime);
 
+  // Add two resources that shouldn't appear in downloaded.
+  Flush();  // We need to write the referrer_host_id.
+  const std::string already_reported_and_not_refetch =
+      "http://foo.com/already-reported-and-not-refetch.js";
+
+  precache_database_->RecordURLPrefetch(GURL(already_reported_and_not_refetch),
+                                        "foo.com", kPrecacheTime, false, kSize);
+  const std::string already_reported_and_in_cache =
+      "http://foo.com/already-reported-and-in-cache.js";
+  precache_database_->RecordURLPrefetch(GURL(already_reported_and_in_cache),
+                                        "foo.com", kPrecacheTime, false, kSize);
+  {
+    // Let's mark existing resources as is_download_reported = 1 by calling
+    // GetURLListForReferrerHost.
+    std::vector<GURL> unused_a, unused_b;
+    auto id = precache_database_->GetReferrerHost("foo.com").id;
+    ASSERT_NE(PrecacheReferrerHostEntry::kInvalidId, id);
+    precache_database_->GetURLListForReferrerHost(id, &unused_a, &unused_b);
+  }
+
   struct test_resource_info {
     std::string url;
     bool is_user_browsed;
@@ -582,6 +604,8 @@ TEST_F(PrecacheDatabaseTest, GetURLListForReferrerHost) {
                                             kPrecacheTime, false, kSize);
     }
   }
+  precache_database_->RecordURLPrefetch(GURL(already_reported_and_in_cache),
+                                        "foo.com", kPrecacheTime, true, kSize);
   // Update some resources as used due to user browsing.
   for (const auto& test : tests) {
     for (const auto& resource : test.resource_info) {
@@ -600,35 +624,35 @@ TEST_F(PrecacheDatabaseTest, GetURLListForReferrerHost) {
     }
   }
   Flush();
-  // Verify the used and unused resources.
+  // Verify the used and downloaded resources.
   for (const auto& test : tests) {
-    std::vector<GURL> expected_used_urls, expected_unused_urls;
+    std::vector<GURL> expected_used_urls, expected_downloaded_urls;
     for (const auto& resource : test.resource_info) {
       if (resource.expected_in_used) {
         expected_used_urls.push_back(GURL(resource.url));
-      } else {
-        expected_unused_urls.push_back(GURL(resource.url));
       }
+      expected_downloaded_urls.push_back(GURL(resource.url));
     }
-    std::vector<GURL> actual_used_urls, actual_unused_urls;
+    std::vector<GURL> actual_used_urls, actual_downloaded_urls;
     auto referrer_id = precache_database_->GetReferrerHost(test.hostname).id;
     EXPECT_NE(PrecacheReferrerHostEntry::kInvalidId, referrer_id);
     precache_database_->GetURLListForReferrerHost(
-        referrer_id, &actual_used_urls, &actual_unused_urls);
-    EXPECT_THAT(expected_used_urls, ::testing::ContainerEq(actual_used_urls));
-    EXPECT_THAT(expected_unused_urls,
-                ::testing::ContainerEq(actual_unused_urls));
+        referrer_id, &actual_used_urls, &actual_downloaded_urls);
+    EXPECT_THAT(actual_used_urls, ::testing::ContainerEq(expected_used_urls));
+    EXPECT_THAT(actual_downloaded_urls,
+                ::testing::ContainerEq(expected_downloaded_urls))
+        << "Host: " << test.hostname;
   }
-  // Subsequent manifest updates should clear the used and unused resources.
+  // Subsequent manifest updates should clear the used and downloaded resources.
   for (const auto& test : tests) {
     precache_database_->UpdatePrecacheReferrerHost(test.hostname, 100,
                                                    kNewFetchTime);
     Flush();
-    std::vector<GURL> actual_used_urls, actual_unused_urls;
+    std::vector<GURL> actual_used_urls, actual_downloaded_urls;
     auto referrer_id = precache_database_->GetReferrerHost(test.hostname).id;
     EXPECT_NE(PrecacheReferrerHostEntry::kInvalidId, referrer_id);
     precache_database_->GetURLListForReferrerHost(
-        referrer_id, &actual_used_urls, &actual_unused_urls);
+        referrer_id, &actual_used_urls, &actual_downloaded_urls);
     EXPECT_TRUE(actual_used_urls.empty());

[twifkak, 2016/12/20 22:48:09]

change this to EXPECT_THAT(actual_used_urls, IsEmpty()) (for a better error
message) and add a check for actual_downloaded_urls too

[jamartin, 2016/12/20 23:14:07]

Done.

   }
   // Resources that were precached previously and not seen in user browsing
@@ -640,6 +664,9 @@ TEST_F(PrecacheDatabaseTest, GetURLListForReferrerHost) {
         expected_url_table_map[GURL(resource.url)] = kPrecacheTime;
     }
   }
+  expected_url_table_map[GURL(already_reported_and_not_refetch)] =
+      kPrecacheTime;
+  expected_url_table_map[GURL(already_reported_and_in_cache)] = kPrecacheTime;
   EXPECT_EQ(expected_url_table_map, GetActualURLTableMap());

[twifkak, 2016/12/20 22:48:09]

Add a resource for which is_download_reported gets reset to 0.

Also, this test is getting pretty long. Maybe you want to split out the new URLs
in a separate test looking only at downloaded_urls?

[jamartin, 2016/12/20 23:14:07]

Done.

 }