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

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index 05a247dc4929d1460821356f19cd9d00a3b8ec87..b23f1ab9c53293212ce7e78b8d388d1c59c8f9cb 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -58,14 +58,17 @@ namespace history {
 //                    the |id| field in the appropriate row in the |favicons|
 //                    table.
 //
-//  id                Unique ID.
-//  icon_id           The ID of the favicon that the bitmap is associated to.
-//  last_updated      The time at which this favicon was inserted into the
+//   id               Unique ID.
+//   icon_id          The ID of the favicon that the bitmap is associated to.
+//   last_updated     The time at which this favicon was inserted into the
 //                    table. This is used to determine if it needs to be
 //                    redownloaded from the web.
-//  image_data        PNG encoded data of the favicon.
-//  width             Pixel width of |image_data|.
-//  height            Pixel height of |image_data|.
+//   last_requested   The time at which this bitmap was last requested. This is
+//                    used to determine the priority with which the bitmap
+//                    should be retained on cleanup.
+//   image_data       PNG encoded data of the favicon.
+//   width            Pixel width of |image_data|.
+//   height           Pixel height of |image_data|.
 
 namespace {
 
@@ -77,6 +80,7 @@ namespace {
 // fatal (in fact, very old data may be expired immediately at startup
 // anyhow).
 
+// Version 8: ???????? by rogerm@chromium.org on 2015-??-??
 // Version 7: 911a634d/r209424 by qsr@chromium.org on 2013-07-01
 // Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20
 // Version 5: e2ee8ae9/r105004 by groby@chromium.org on 2011-10-12 (deprecated)
@@ -86,8 +90,8 @@ namespace {
 // Version number of the database.
 // NOTE(shess): When changing the version, add a new golden file for
 // the new version and a test to verify that Init() works with it.
-const int kCurrentVersionNumber = 7;
-const int kCompatibleVersionNumber = 7;
+const int kCurrentVersionNumber = 8;
+const int kCompatibleVersionNumber = 8;
 const int kDeprecatedVersionNumber = 5;  // and earlier.
 
 void FillIconMapping(const sql::Statement& statement,
@@ -310,7 +314,10 @@ bool InitTables(sql::Connection* db) {
       "last_updated INTEGER DEFAULT 0,"
       "image_data BLOB,"
       "width INTEGER DEFAULT 0,"
-      "height INTEGER DEFAULT 0"
+      "height INTEGER DEFAULT 0,"
+      // This field is at the end so that fresh tables and migrated tables have
+      // the same layout.
+      "last_requested INTEGER DEFAULT 0"
       ")";
   if (!db->Execute(kFaviconBitmapsSql))
     return false;
@@ -396,7 +403,7 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
   // NOTE(shess): This code is currently specific to the version
   // number.  I am working on simplifying things to loosen the
   // dependency, meanwhile contact me if you need to bump the version.
-  DCHECK_EQ(7, kCurrentVersionNumber);
+  DCHECK_EQ(8, kCurrentVersionNumber);
 
   // TODO(shess): Reset back after?
   db->reset_error_callback();
@@ -448,9 +455,8 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
     return;
   }
 
-  // Earlier versions have been handled or deprecated, later versions should be
-  // impossible.
-  if (version != 7) {
+  // Earlier versions have been handled or deprecated.
+  if (version < 7) {
     sql::Recovery::Unrecoverable(recovery.Pass());
     RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
     return;
@@ -727,6 +733,22 @@ bool ThumbnailDatabase::GetFaviconBitmap(
   return true;
 }
 
+bool ThumbnailDatabase::GetFaviconBitmapLastRequestedTime(

[sky, 2015/03/30 14:51:42]

Generally the history code has tried to have functions that return all related
state, rather than a slew of functions for standalone data that was need at one
time or another. Rather than a new function, can you change GetFaviconBitmap()
to return the last_requested time as well. 

[Roger McFarlane (Chromium), 2015/03/31 18:25:49]

Done.

+    FaviconBitmapID bitmap_id,
+    base::Time* last_requested) {
+  DCHECK(bitmap_id);
+  DCHECK(last_requested);
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT last_requested FROM favicon_bitmaps WHERE id=?"));
+  statement.BindInt64(0, bitmap_id);
+
+  if (!statement.Step())
+    return false;
+
+  *last_requested = base::Time::FromInternalValue(statement.ColumnInt64(0));
+  return true;
+}
+
 FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
     favicon_base::FaviconID icon_id,
     const scoped_refptr<base::RefCountedMemory>& icon_data,
@@ -782,6 +804,17 @@ bool ThumbnailDatabase::SetFaviconBitmapLastUpdateTime(
   return statement.Run();
 }
 
+bool ThumbnailDatabase::SetFaviconBitmapLastRequestedTime(
+    FaviconBitmapID bitmap_id,
+    base::Time time) {
+  DCHECK(bitmap_id);
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "UPDATE favicon_bitmaps SET last_requested=? WHERE id=?"));
+  statement.BindInt64(0, time.ToInternalValue());
+  statement.BindInt64(1, bitmap_id);
+  return statement.Run();
+}
+
 bool ThumbnailDatabase::DeleteFaviconBitmap(FaviconBitmapID bitmap_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicon_bitmaps WHERE id=?"));
@@ -1090,9 +1123,9 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
       "ALTER TABLE favicon_bitmaps RENAME TO old_favicon_bitmaps";
   const char kCopyFaviconBitmaps[] =
       "INSERT INTO favicon_bitmaps "
-      "  (icon_id, last_updated, image_data, width, height) "
+      "  (icon_id, last_updated, image_data, width, height, last_requested) "
       "SELECT mapping.new_icon_id, old.last_updated, "
-      "    old.image_data, old.width, old.height "
+      "    old.image_data, old.width, old.height, old.last_requested "
       "FROM old_favicon_bitmaps AS old "
       "JOIN temp.icon_id_mapping AS mapping "
       "ON (old.icon_id = mapping.old_icon_id)";
@@ -1193,8 +1226,7 @@ sql::InitStatus ThumbnailDatabase::InitImpl(const base::FilePath& db_name) {
   base::mac::SetFileBackupExclusion(db_name);
 #endif
 
-  // thumbnails table has been obsolete for a long time, remove any
-  // detrious.
+  // thumbnails table has been obsolete for a long time, remove any detritus.
   ignore_result(db_.Execute("DROP TABLE IF EXISTS thumbnails"));
 
   // At some point, operations involving temporary tables weren't done
@@ -1246,6 +1278,12 @@ sql::InitStatus ThumbnailDatabase::InitImpl(const base::FilePath& db_name) {
       return CantUpgradeToVersion(cur_version);
   }
 
+  if (cur_version == 7) {
+    ++cur_version;
+    if (!UpgradeToVersion8())
+      return CantUpgradeToVersion(cur_version);
+  }
+
   LOG_IF(WARNING, cur_version < kCurrentVersionNumber) <<
       "Thumbnail database version " << cur_version << " is too old to handle.";
 
@@ -1300,6 +1338,18 @@ bool ThumbnailDatabase::UpgradeToVersion7() {
   return true;
 }
 
+bool ThumbnailDatabase::UpgradeToVersion8() {
+  // Add the last_requested column to the favicon_bitmaps table.
+  const char kFaviconBitmapsAddLastRequestedSql[] =
+      "ALTER TABLE favicon_bitmaps ADD COLUMN last_requested INTEGER DEFAULT 0";
+  if (!db_.Execute(kFaviconBitmapsAddLastRequestedSql))
+    return false;
+
+  meta_table_.SetVersionNumber(8);
+  meta_table_.SetCompatibleVersionNumber(std::min(8, kCompatibleVersionNumber));
+  return true;
+}
+
 bool ThumbnailDatabase::IsFaviconDBStructureIncorrect() {
   return !db_.IsSQLValid("SELECT id, url, icon_type FROM favicons");
 }