[Thumbnails DB] Allow setting last_requested time when accessing favicons.

components/history/core/browser/thumbnail_database_unittest.cc

Index: components/history/core/browser/thumbnail_database_unittest.cc
diff --git a/components/history/core/browser/thumbnail_database_unittest.cc b/components/history/core/browser/thumbnail_database_unittest.cc
index 0ece892cfe4f636598149b10c95caa3af283da1e..71d714d139dcb8412869f7b7f5830b2725e61c8e 100644
--- a/components/history/core/browser/thumbnail_database_unittest.cc
+++ b/components/history/core/browser/thumbnail_database_unittest.cc
@@ -224,37 +224,141 @@ TEST_F(ThumbnailDatabaseTest, AddIconMapping) {
   EXPECT_EQ(id, icon_mappings.front().icon_id);
 }
 
-TEST_F(ThumbnailDatabaseTest, LastRequestedTime) {
+TEST_F(ThumbnailDatabaseTest,
+       AddOnDemandFaviconBitmapCreatesCorrectTimestamps) {
   ThumbnailDatabase db(NULL);
   ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
   db.BeginTransaction();
 
+  base::Time time;

[pkotwicz, 2017/06/07 17:40:53]

Can you please rename this variable to something more meaningful?

add_time perhaps?

[jkrcal, 2017/06/09 16:38:38]

Done.

+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &time));
   std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
   scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
 
   GURL url("http://google.com");
-  base::Time now = base::Time::Now();
-  favicon_base::FaviconID id =
-      db.AddFavicon(url, favicon_base::TOUCH_ICON, favicon, now, gfx::Size());
-  ASSERT_NE(0, id);
-
-  // Fetching the last requested time of a non-existent bitmap should fail.
-  base::Time last_requested = base::Time::UnixEpoch();
-  EXPECT_FALSE(db.GetFaviconBitmap(id + 1, NULL, &last_requested, NULL, NULL));
-  EXPECT_EQ(last_requested, base::Time::UnixEpoch());  // Remains unchanged.
-
-  // Fetching the last requested time of a bitmap that has no last request
-  // should return a null timestamp.
-  last_requested = base::Time::UnixEpoch();
-  EXPECT_TRUE(db.GetFaviconBitmap(id, NULL, &last_requested, NULL, NULL));
-  EXPECT_TRUE(last_requested.is_null());
-
-  // Setting the last requested time of an existing bitmap should succeed, and
-  // the set time should be returned by the corresponding "Get".
-  last_requested = base::Time::UnixEpoch();
-  EXPECT_TRUE(db.SetFaviconBitmapLastRequestedTime(id, now));
-  EXPECT_TRUE(db.GetFaviconBitmap(id, NULL, &last_requested, NULL, NULL));
-  EXPECT_EQ(last_requested, now);
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_DEMAND, time, gfx::Size());
+  ASSERT_NE(0, bitmap);
+
+  base::Time last_updated;
+  base::Time last_requested;
+  ASSERT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested,
+                                  nullptr, nullptr));
+  EXPECT_EQ(base::Time(), last_updated);
+  EXPECT_EQ(time, last_requested);
+}
+
+TEST_F(ThumbnailDatabaseTest, AddFaviconBitmapCreatesCorrectTimestamps) {
+  ThumbnailDatabase db(NULL);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time time;

[pkotwicz, 2017/06/07 17:40:53]

Ditto

[jkrcal, 2017/06/09 16:38:38]

Done.

+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &time));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_VISIT, time, gfx::Size());
+  ASSERT_NE(0, bitmap);
+
+  base::Time last_updated;
+  base::Time last_requested;
+  ASSERT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested,
+                                  nullptr, nullptr));
+  EXPECT_EQ(time, last_updated);
+  EXPECT_EQ(base::Time(), last_requested);
+}
+
+TEST_F(ThumbnailDatabaseTest, TouchUpdatesOnDemandFavicons) {
+  ThumbnailDatabase db(NULL);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Create an on-demand favicon.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size());
+  ASSERT_NE(0, bitmap);
+
+  base::Time end = start + base::TimeDelta::FromDays(14);

[pkotwicz, 2017/06/07 17:40:53]

You should use kFaviconUpdateLastRequestedAfterDays here. Otherwise, it is
confusing why 14 days was chosen

[jkrcal, 2017/06/09 16:38:38]

Done.

+  EXPECT_TRUE(db.TouchOnDemandFavicon(url, end));
+
+  base::Time last_updated;
+  base::Time last_requested;
+  EXPECT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested,
+                                  nullptr, nullptr));
+  // Does not mess up with the last_updated field.

[pkotwicz, 2017/06/07 17:40:53]

Nit: 'mess up' -> 'mess'

[jkrcal, 2017/06/09 16:38:38]

Done.

+  EXPECT_EQ(base::Time(), last_updated);
+  EXPECT_EQ(end, last_requested);  // Updates the last_requested field.
+}
+
+TEST_F(ThumbnailDatabaseTest, TouchUpdatesOnlyInfrequently) {
+  ThumbnailDatabase db(NULL);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Create an on-demand favicon.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  ASSERT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_DEMAND, start, gfx::Size());
+  ASSERT_NE(0, bitmap);
+
+  base::Time end = start + base::TimeDelta::FromMinutes(1);
+  EXPECT_TRUE(db.TouchOnDemandFavicon(url, end));
+
+  base::Time last_requested;
+  EXPECT_TRUE(
+      db.GetFaviconBitmap(bitmap, nullptr, &last_requested, nullptr, nullptr));
+  EXPECT_EQ(start, last_requested);  // No update.
+}
+
+TEST_F(ThumbnailDatabaseTest, TouchDoesNotUpdateStandardFavicons) {
+  ThumbnailDatabase db(NULL);

[pkotwicz, 2017/06/07 17:40:53]

Nit: NULL -> nullptr (here and elsewhere in the new test cases)

[jkrcal, 2017/06/09 16:38:38]

Done.

+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  base::Time start;
+  ASSERT_TRUE(base::Time::FromUTCExploded({2017, 5, 0, 1, 0, 0, 0, 0}, &start));
+  std::vector<unsigned char> data(kBlob1, kBlob1 + sizeof(kBlob1));
+  scoped_refptr<base::RefCountedBytes> favicon(new base::RefCountedBytes(data));
+
+  // Create a standard favicon.
+  GURL url("http://google.com");
+  favicon_base::FaviconID icon = db.AddFavicon(url, favicon_base::FAVICON);
+  EXPECT_NE(0, icon);
+  FaviconBitmapID bitmap = db.AddFaviconBitmap(
+      icon, favicon, FaviconBitmapType::ON_VISIT, start, gfx::Size());
+  EXPECT_NE(0, bitmap);
+
+  base::Time end = start + base::TimeDelta::FromDays(14);
+  db.TouchOnDemandFavicon(url, end);
+
+  base::Time last_updated;
+  base::Time last_requested;
+  EXPECT_TRUE(db.GetFaviconBitmap(bitmap, &last_updated, &last_requested,
+                                  nullptr, nullptr));
+  EXPECT_EQ(start, last_updated);  // Does not mess up with last_updates.

[pkotwicz, 2017/06/07 17:40:53]

Nits:
'mess up' -> 'mess'
'last_udpates' -> 'last_updated'

[jkrcal, 2017/06/09 16:38:38]

Done.

+  EXPECT_EQ(base::Time(), last_requested);  // No update.
 }
 
 TEST_F(ThumbnailDatabaseTest, DeleteIconMappings) {
@@ -268,7 +372,8 @@ TEST_F(ThumbnailDatabaseTest, DeleteIconMappings) {
   GURL url("http://google.com");
   favicon_base::FaviconID id = db.AddFavicon(url, favicon_base::TOUCH_ICON);
   base::Time time = base::Time::Now();
-  db.AddFaviconBitmap(id, favicon, time, gfx::Size());
+  db.AddFaviconBitmap(id, favicon, FaviconBitmapType::ON_VISIT, time,
+                      gfx::Size());
   EXPECT_LT(0, db.AddIconMapping(url, id));
 
   favicon_base::FaviconID id2 = db.AddFavicon(url, favicon_base::FAVICON);
@@ -299,13 +404,16 @@ TEST_F(ThumbnailDatabaseTest, GetIconMappingsForPageURL) {
 
   favicon_base::FaviconID id1 = db.AddFavicon(url, favicon_base::TOUCH_ICON);
   base::Time time = base::Time::Now();
-  db.AddFaviconBitmap(id1, favicon, time, kSmallSize);
-  db.AddFaviconBitmap(id1, favicon, time, kLargeSize);
+  db.AddFaviconBitmap(id1, favicon, FaviconBitmapType::ON_VISIT, time,
+                      kSmallSize);
+  db.AddFaviconBitmap(id1, favicon, FaviconBitmapType::ON_VISIT, time,
+                      kLargeSize);
   EXPECT_LT(0, db.AddIconMapping(url, id1));
 
   favicon_base::FaviconID id2 = db.AddFavicon(url, favicon_base::FAVICON);
   EXPECT_NE(id1, id2);
-  db.AddFaviconBitmap(id2, favicon, time, kSmallSize);
+  db.AddFaviconBitmap(id2, favicon, FaviconBitmapType::ON_VISIT, time,
+                      kSmallSize);
   EXPECT_LT(0, db.AddIconMapping(url, id2));
 
   std::vector<IconMapping> icon_mappings;
@@ -341,19 +449,22 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrls) {
 
   favicon_base::FaviconID kept_id1 =
       db.AddFavicon(kIconUrl1, favicon_base::FAVICON);
-  db.AddFaviconBitmap(kept_id1, favicon1, base::Time::Now(), kLargeSize);
+  db.AddFaviconBitmap(kept_id1, favicon1, FaviconBitmapType::ON_VISIT,
+                      base::Time::Now(), kLargeSize);
   db.AddIconMapping(kPageUrl1, kept_id1);
   db.AddIconMapping(kPageUrl3, kept_id1);
   db.AddIconMapping(kPageUrl4, kept_id1);
 
   favicon_base::FaviconID unkept_id =
       db.AddFavicon(kIconUrl2, favicon_base::FAVICON);
-  db.AddFaviconBitmap(unkept_id, favicon1, base::Time::Now(), kLargeSize);
+  db.AddFaviconBitmap(unkept_id, favicon1, FaviconBitmapType::ON_VISIT,
+                      base::Time::Now(), kLargeSize);
   db.AddIconMapping(kPageUrl2, unkept_id);
 
   favicon_base::FaviconID kept_id2 =
       db.AddFavicon(kIconUrl5, favicon_base::FAVICON);
-  db.AddFaviconBitmap(kept_id2, favicon2, base::Time::Now(), kLargeSize);
+  db.AddFaviconBitmap(kept_id2, favicon2, FaviconBitmapType::ON_VISIT,
+                      base::Time::Now(), kLargeSize);
   db.AddIconMapping(kPageUrl5, kept_id2);
 
   // RetainDataForPageUrls() uses schema manipulations for efficiency.
@@ -439,8 +550,10 @@ TEST_F(ThumbnailDatabaseTest, DeleteFavicon) {
   GURL url("http://google.com");
   favicon_base::FaviconID id = db.AddFavicon(url, favicon_base::FAVICON);
   base::Time last_updated = base::Time::Now();
-  db.AddFaviconBitmap(id, favicon1, last_updated, kSmallSize);
-  db.AddFaviconBitmap(id, favicon2, last_updated, kLargeSize);
+  db.AddFaviconBitmap(id, favicon1, FaviconBitmapType::ON_VISIT, last_updated,
+                      kSmallSize);
+  db.AddFaviconBitmap(id, favicon2, FaviconBitmapType::ON_VISIT, last_updated,
+                      kLargeSize);
 
   EXPECT_TRUE(db.GetFaviconBitmaps(id, NULL));