Add last_requested field to the favicon_bitmaps table of the favicons database.

chrome/browser/history/thumbnail_database_unittest.cc

Index: chrome/browser/history/thumbnail_database_unittest.cc
diff --git a/chrome/browser/history/thumbnail_database_unittest.cc b/chrome/browser/history/thumbnail_database_unittest.cc
index 0b2c6ab9bd06463515e6698613766555b8b47540..11ef9d10c657170efecb07a32d0b4f153531e198 100644
--- a/chrome/browser/history/thumbnail_database_unittest.cc
+++ b/chrome/browser/history/thumbnail_database_unittest.cc
@@ -68,9 +68,9 @@ void VerifyTablesAndColumns(sql::Connection* db) {
   // [meta], [favicons], [favicon_bitmaps], and [icon_mapping].
   EXPECT_EQ(4u, sql::test::CountSQLTables(db));
 
-  // Implicit index on [meta], index on [favicons], index on
+  // Implicit index on [meta], index on [favicons], two indices on
   // [favicon_bitmaps], two indices on [icon_mapping].
-  EXPECT_EQ(5u, sql::test::CountSQLIndices(db));
+  EXPECT_EQ(6u, sql::test::CountSQLIndices(db));
 
   // [key] and [value].
   EXPECT_EQ(2u, sql::test::CountTableColumns(db, "meta"));
@@ -78,8 +78,9 @@ void VerifyTablesAndColumns(sql::Connection* db) {
   // [id], [url], and [icon_type].
   EXPECT_EQ(3u, sql::test::CountTableColumns(db, "favicons"));
 
-  // [id], [icon_id], [last_updated], [image_data], [width], and [height].
-  EXPECT_EQ(6u, sql::test::CountTableColumns(db, "favicon_bitmaps"));
+  // [id], [icon_id], [last_updated], [last_requested], [image_data], [width],
+  // and [height].
+  EXPECT_EQ(7u, sql::test::CountTableColumns(db, "favicon_bitmaps"));
 
   // [id], [page_url], and [icon_id].
   EXPECT_EQ(3u, sql::test::CountTableColumns(db, "icon_mapping"));
@@ -213,6 +214,38 @@ TEST_F(ThumbnailDatabaseTest, AddIconMapping) {
   EXPECT_EQ(id, icon_mappings.front().icon_id);
 }
 
+TEST_F(ThumbnailDatabaseTest, LastRequestedTime) {
+  ThumbnailDatabase db(NULL);
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+  db.BeginTransaction();
+
+  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());
+  EXPECT_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.GetFaviconBitmapLastRequestedTime(id + 1, &last_requested));
+  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.GetFaviconBitmapLastRequestedTime(id, &last_requested));
+  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".
+  EXPECT_TRUE(db.SetFaviconBitmapLastRequestedTime(id, now));
+  EXPECT_TRUE(db.GetFaviconBitmapLastRequestedTime(id, &last_requested));
+  EXPECT_EQ(last_requested, now);
+ }
+
 TEST_F(ThumbnailDatabaseTest, UpdateIconMapping) {
   ThumbnailDatabase db(NULL);
   ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
@@ -766,6 +799,42 @@ TEST_F(ThumbnailDatabaseTest, Version7) {
                                kBlob2));
 }
 
+// Test loading version 8 database.
+TEST_F(ThumbnailDatabaseTest, Version8) {
+  scoped_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v8.sql");
+  ASSERT_TRUE(db.get() != NULL);
+  VerifyTablesAndColumns(&db->db_);
+
+  EXPECT_TRUE(CheckPageHasIcon(db.get(),
+                               kPageUrl1,
+                               favicon_base::FAVICON,
+                               kIconUrl1,
+                               kLargeSize,
+                               sizeof(kBlob1),
+                               kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(),
+                               kPageUrl2,
+                               favicon_base::FAVICON,
+                               kIconUrl2,
+                               kLargeSize,
+                               sizeof(kBlob2),
+                               kBlob2));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(),
+                               kPageUrl3,
+                               favicon_base::FAVICON,
+                               kIconUrl1,
+                               kLargeSize,
+                               sizeof(kBlob1),
+                               kBlob1));
+  EXPECT_TRUE(CheckPageHasIcon(db.get(),
+                               kPageUrl3,
+                               favicon_base::TOUCH_ICON,
+                               kIconUrl3,
+                               kLargeSize,
+                               sizeof(kBlob2),
+                               kBlob2));
+}
+
 TEST_F(ThumbnailDatabaseTest, Recovery) {
   // This code tests the recovery module in concert with Chromium's
   // custom recover virtual table.  Under USE_SYSTEM_SQLITE, this is
@@ -778,7 +847,7 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
 
   // Create an example database.
   {
-    EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v7.sql"));
+    EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v8.sql"));

[Scott Hess - ex-Googler, 2015/03/17 22:07:44]

Create test for Recovery7 which runs the recovery from a version 7 file.  I
think it would be reasonable to just copy Recovery test and tweak the contents
appropriately to init from the right golden file.

In some of these things it might seem like it would be "better" to
consolidate/rewrite/refactor to make the code smaller/easier/something.  I have
found this to not be generally true WRT persistence.  The databases in the wild
are spread across multiple states, so having a straightforward view of the code
can be really helpful if/when things go awry.

Not that they're going to go awry here.  But I'm sure every time I've had to
reconstruct things from revision control the original author didn't think things
would go awry, either :-).

[Roger McFarlane (Chromium), 2015/03/19 17:55:51]

Done.

 
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));