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 04b9a5a1266c07bcced327045856cad5d1989018..02fb6a2f4e5c5546db7108a191f120c7f9a22aeb 100644
--- a/chrome/browser/history/thumbnail_database_unittest.cc
+++ b/chrome/browser/history/thumbnail_database_unittest.cc
@@ -68,8 +68,8 @@ 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
-  // [favicon_bitmaps], two indices on [icon_mapping].
+  // Implicit index on [meta], index on [favicons], index on [favicon_bitmaps],
+  // two indices on [icon_mapping].
   EXPECT_EQ(5u, sql::test::CountSQLIndices(db));
 
   // [key] and [value].
@@ -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], [image_data], [width], [height] and
+  // [last_requested].
+  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));

[huangs, 2015/03/19 19:14:11]

NIT: add redundant
last_requested = base::Time::UnixEpoch();

to make this case independent of the last?

[Roger McFarlane (Chromium), 2015/03/20 14:40:59]

Done.

Not strictly necessary since last_requested should otherwise be either UnixEpoch
or null, but not "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_));
@@ -671,39 +704,49 @@ TEST_F(ThumbnailDatabaseTest, Version5) {
   ASSERT_TRUE(db.get() != NULL);
   VerifyTablesAndColumns(&db->db_);
 
+  // Version 5 is deprecated, the data should all be gone.
+  VerifyDatabaseEmpty(&db->db_);
+}
+
+// Test loading version 6 database.
+TEST_F(ThumbnailDatabaseTest, Version6) {
+  scoped_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v6.sql");
+  ASSERT_TRUE(db.get() != NULL);
+  VerifyTablesAndColumns(&db->db_);
+
   EXPECT_TRUE(CheckPageHasIcon(db.get(),
                                kPageUrl1,
                                favicon_base::FAVICON,
                                kIconUrl1,
-                               gfx::Size(),
+                               kLargeSize,
                                sizeof(kBlob1),
                                kBlob1));
   EXPECT_TRUE(CheckPageHasIcon(db.get(),
                                kPageUrl2,
                                favicon_base::FAVICON,
                                kIconUrl2,
-                               gfx::Size(),
+                               kLargeSize,
                                sizeof(kBlob2),
                                kBlob2));
   EXPECT_TRUE(CheckPageHasIcon(db.get(),
                                kPageUrl3,
                                favicon_base::FAVICON,
                                kIconUrl1,
-                               gfx::Size(),
+                               kLargeSize,
                                sizeof(kBlob1),
                                kBlob1));
   EXPECT_TRUE(CheckPageHasIcon(db.get(),
                                kPageUrl3,
                                favicon_base::TOUCH_ICON,
                                kIconUrl3,
-                               gfx::Size(),
+                               kLargeSize,
                                sizeof(kBlob2),
                                kBlob2));
 }
 
-// Test loading version 6 database.
-TEST_F(ThumbnailDatabaseTest, Version6) {
-  scoped_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v6.sql");
+// Test loading version 7 database.
+TEST_F(ThumbnailDatabaseTest, Version7) {
+  scoped_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v7.sql");
   ASSERT_TRUE(db.get() != NULL);
   VerifyTablesAndColumns(&db->db_);
 
@@ -737,9 +780,9 @@ TEST_F(ThumbnailDatabaseTest, Version6) {
                                kBlob2));
 }
 
-// Test loading version 7 database.
-TEST_F(ThumbnailDatabaseTest, Version7) {
-  scoped_ptr<ThumbnailDatabase> db = LoadFromGolden("Favicons.v7.sql");
+// 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_);
 
@@ -785,7 +828,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"));
 
     sql::Connection raw_db;
     EXPECT_TRUE(raw_db.Open(file_name_));
@@ -910,6 +953,118 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
   }
 }
 
+TEST_F(ThumbnailDatabaseTest, Recovery7) {
+  // This code tests the recovery module in concert with Chromium's
+  // custom recover virtual table.  Under USE_SYSTEM_SQLITE, this is
+  // not available.  This is detected dynamically because corrupt
+  // databases still need to be handled, perhaps by Raze(), and the
+  // recovery module is an obvious layer to abstract that to.
+  // TODO(shess): Handle that case for real!
+  if (!sql::Recovery::FullRecoverySupported())
+    return;
+
+  // Create an example database without loading into ThumbnailDatabase
+  // (which would upgrade it).
+  EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v7.sql"));
+
+  // Corrupt the |icon_mapping.page_url| index by deleting an element
+  // from the backing table but not the index.
+  {
+    sql::Connection raw_db;
+    EXPECT_TRUE(raw_db.Open(file_name_));
+    ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
+  }
+  const char kIndexName[] = "icon_mapping_page_url_idx";
+  const char kDeleteSql[] =
+      "DELETE FROM icon_mapping WHERE page_url = 'http://yahoo.com/'";
+  EXPECT_TRUE(
+      sql::test::CorruptTableOrIndex(file_name_, kIndexName, kDeleteSql));
+
+  // Database should be corrupt at the SQLite level.
+  {
+    sql::Connection raw_db;
+    EXPECT_TRUE(raw_db.Open(file_name_));
+    ASSERT_NE("ok", sql::test::IntegrityCheck(&raw_db));
+  }
+
+  // Open the database and access the corrupt index. Note that this upgrades
+  // the database.
+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+    ThumbnailDatabase db(NULL);
+    ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+
+    // Data for kPageUrl2 was deleted, but the index entry remains,
+    // this will throw SQLITE_CORRUPT.  The corruption handler will
+    // recover the database and poison the handle, so the outer call
+    // fails.
+    EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
+
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
+  }
+
+  // Check that the database is recovered at the SQLite level.
+  {
+    sql::Connection raw_db;
+    EXPECT_TRUE(raw_db.Open(file_name_));
+    ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
+
+    // Check that the expected tables exist.
+    VerifyTablesAndColumns(&raw_db);
+  }
+
+  // Database should also be recovered at higher levels.
+  {
+    ThumbnailDatabase db(NULL);
+    ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+
+    // Now this fails because there is no mapping.
+    EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
+
+    // Other data was retained by recovery.
+    EXPECT_TRUE(CheckPageHasIcon(&db,
+                                 kPageUrl1,
+                                 favicon_base::FAVICON,
+                                 kIconUrl1,
+                                 kLargeSize,
+                                 sizeof(kBlob1),
+                                 kBlob1));
+  }
+
+  // Corrupt the database again by adjusting the header.
+  EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_));
+
+  // Database is unusable at the SQLite level.
+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+    sql::Connection raw_db;
+    EXPECT_TRUE(raw_db.Open(file_name_));
+    EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check"));
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
+  }
+
+  // Database should be recovered during open.
+  {
+    sql::ScopedErrorIgnorer ignore_errors;
+    ignore_errors.IgnoreError(SQLITE_CORRUPT);
+    ThumbnailDatabase db(NULL);
+    ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));
+
+    EXPECT_FALSE(db.GetIconMappingsForPageURL(kPageUrl2, NULL));
+    EXPECT_TRUE(CheckPageHasIcon(&db,
+                                 kPageUrl1,
+                                 favicon_base::FAVICON,
+                                 kIconUrl1,
+                                 kLargeSize,
+                                 sizeof(kBlob1),
+                                 kBlob1));
+
+    ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
+  }
+}
+
 TEST_F(ThumbnailDatabaseTest, Recovery6) {
   // TODO(shess): See comment at top of Recovery test.
   if (!sql::Recovery::FullRecoverySupported())