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 c115c3cc8ca06ae560759357babc4462f6c51c85..fb43530e0fd8d7d0c624e3874bb9e61490036ffb 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: ????????/r?????? by rogerm@chromium on 2015-??-??

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

Don't worry too much about filling this in, usually the most recent change is
easy to find, it's the one that happened four years ago which takes forever.

No more svn means the /r? section isn't sensible any longer.  Also chromium.org.

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

Fair enough.

 // 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
@@ -86,7 +90,7 @@ 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 kCurrentVersionNumber = 8;
 const int kCompatibleVersionNumber = 7;
 const int kDeprecatedVersionNumber = 4;  // and earlier.
 
@@ -308,6 +312,7 @@ bool InitTables(sql::Connection* db) {
       "id INTEGER PRIMARY KEY,"
       "icon_id INTEGER NOT NULL,"
       "last_updated INTEGER DEFAULT 0,"
+      "last_requested INTEGER DEFAULT 0,"

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

Prefer to put the added column at the end, since that's where it will be for the
ALTER TABLE version.  One might argue "But this is how I would rather it be",
but reality is that a billion users (or so) will have it at the end, so it's
better to just let them all match.

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

Done.

       "image_data BLOB,"
       "width INTEGER DEFAULT 0,"
       "height INTEGER DEFAULT 0"
@@ -315,6 +320,16 @@ bool InitTables(sql::Connection* db) {
   if (!db->Execute(kFaviconBitmapsSql))
     return false;
 
+  // If we're updating to version 8, the favicon_bitmaps table already exists,
+  // so the above create was a NOP and we're still missing the last_requested
+  // column.

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

Pull this out to UpgradeToVersion8() like the other cases.  Also, try not to use
conditional logic, if the column already exists in a version-7 database, that
implies that transactional integrity has been broken and the correct response is
to recover the database, not to hope that everything is just fine.

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

I've clobbered the index over favicon_bitmaps(last_requested), so moving this to
the version 8 upgrade is now fine.

The "problem" that made me do it this way initially was that the code is
structured as:

  InitTablesOrFail();
  InitIndicesOrFail();
  if (previous inits didn't get you to the right version)
    VersionDeltaUpgrad();

InitTables() might be a NOP, which would cause InitIndices() to fail for missing
columns.

[Scott Hess - ex-Googler, 2015/03/19 21:32:22]

Grr.  I really hate our system.  I want to have it only call Init*() on fresh
databases and always do migrate on versioned databases.  Your point sounds
similar to if your database version N needs to be upgraded to N+2 where N+1 adds
a table and N+2 alters the table.  InitTables() adds the N+2 table directly then
the migrate script fails.

Unfortunately I haven't been brave enough to make this change.  Maybe it's time.

+  const char kFaviconBitmapsAddLastRequestedSql[] =
+      "ALTER TABLE favicon_bitmaps ADD COLUMN last_requested INTEGER DEFAULT 0";
+  if (!db->DoesColumnExist("favicon_bitmaps", "last_requested") &&
+      !db->Execute(kFaviconBitmapsAddLastRequestedSql)) {
+    return false;
+  }
+
   return true;
 }
 
@@ -344,6 +359,12 @@ bool InitIndices(sql::Connection* db) {
   if (!db->Execute(kFaviconBitmapsIndexSql))
     return false;
 
+  const char kFaviconBitmapsLastRequestedIndexSql[] =
+      "CREATE INDEX IF NOT EXISTS favicon_bitmaps_last_requested ON "
+      "favicon_bitmaps(last_requested)";
+  if (!db->Execute(kFaviconBitmapsLastRequestedIndexSql))
+    return false;

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

My earlier statement means this will have to be duplicated in the
UpgradeToVersion8() function.  That's fine, version 9 might remove this column,
in which case this code will go away, but the v8 upgrade transition will still
work to transition v7 databases to the v9 upgrade function.

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

I ended up removing this index entirely. I was going to use it to help limit the
range of bitmaps considered then doing garbage collection, but I don't think it
actually helps do that.

As an alternative, I could conceivably keep the index but not set a default
value for regular icon bitmaps (i.e., leave it as NULL) and set 0 for to signify
never-requested large icon bitmaps.

+
   return true;
 }
 
@@ -396,7 +417,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();
@@ -450,7 +471,7 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
 
   // Earlier versions have been handled or deprecated, later versions should be
   // impossible.
-  if (version != 7) {
+  if (version != 8) {

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

This should be < 7, in case someone's database was left corrupt just before an
upgrade.  This recovery code probably means that's a very small number, but it's
free...

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

Ah, quite right.

     sql::Recovery::Unrecoverable(recovery.Pass());
     RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
     return;
@@ -613,11 +634,40 @@ sql::InitStatus ThumbnailDatabase::Init(const base::FilePath& db_name) {
 }
 
 void ThumbnailDatabase::ComputeDatabaseMetrics() {
-  sql::Statement favicon_count(
+  // Count all icon files referenced by the DB.
+  sql::Statement count_statement(
       db_.GetCachedStatement(SQL_FROM_HERE, "SELECT COUNT(*) FROM favicons"));
   UMA_HISTOGRAM_COUNTS_10000(
       "History.NumFaviconsInDB",
-      favicon_count.Step() ? favicon_count.ColumnInt(0) : 0);
+      count_statement.Step() ? count_statement.ColumnInt(0) : 0);
+
+  // Count the subset of "large" icon files referenced by the DB.
+  count_statement.Assign(
+      db_.GetCachedStatement(
+          SQL_FROM_HERE, "SELECT COUNT(*) FROM favicons WHERE icon_type > ?"));

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

Is there any reason to re-use the same sql::Statement variable, rather than
using a different one per metric?  sql::Statement::Assign() is not heavily used
(though I wasn't quite able to get rid of it last time I looked at it), so I'd
tend to prefer using separate statements in case there's some subtle edge case. 
It would make sense to put each in a {} scope, probably.

[As an aside, you make me want to write some sort of GetSingleResultInt() helper
for sql::Connection.]

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

Done.

+  count_statement.BindInt64(0, favicon_base::FAVICON);
+  UMA_HISTOGRAM_COUNTS_10000(
+      "History.NumLargeFaviconsInDB",
+      count_statement.Step() ? count_statement.ColumnInt(0) : 0);

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

These new histograms don't seem directly related to the CL, but I assume they're
to inform future decisions?  They probably need to have histograms.xml changes,
though?

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

I'll pull out the large favicon histograms to a follow-on CL.

Thanks for the histograms.xml reminder. Actually, it looks like the previous
author also forgot to update histograms.xml... History.NumFaviconsInDB is
missing an entry. I'll fix that at the same time.

+
+  // Count all bitmap resources cached in the DB.
+  count_statement.Assign(
+      db_.GetCachedStatement(
+          SQL_FROM_HERE, "SELECT COUNT(*) FROM favicon_bitmaps"));
+  UMA_HISTOGRAM_COUNTS_10000(
+      "History.NumBitmapsInDB",
+      count_statement.Step() ? count_statement.ColumnInt(0) : 0);
+
+  // Count the subset of "large" bitmap resources cached in the DB.
+  count_statement.Assign(
+      db_.GetCachedStatement(
+          SQL_FROM_HERE,
+          "SELECT COUNT(*) FROM favicon_bitmaps "
+          "WHERE id IN (SELECT id FROM favicons WHERE icon_type > ?)"));
+  count_statement.BindInt64(0, favicon_base::FAVICON);
+  UMA_HISTOGRAM_COUNTS_10000(
+      "History.NumLargeBitmapsInDB",
+      count_statement.Step() ? count_statement.ColumnInt(0) : 0);
 }
 
 void ThumbnailDatabase::BeginTransaction() {
@@ -727,6 +777,22 @@ bool ThumbnailDatabase::GetFaviconBitmap(
   return true;
 }
 
+bool ThumbnailDatabase::GetFaviconBitmapLastRequestedTime(
+    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 +848,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=?"));
@@ -866,13 +943,13 @@ favicon_base::FaviconID ThumbnailDatabase::AddFavicon(
 bool ThumbnailDatabase::DeleteFavicon(favicon_base::FaviconID id) {
   sql::Statement statement;
   statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
-      "DELETE FROM favicons WHERE id = ?"));
+      "DELETE FROM favicons WHERE id=?"));

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

Egregious whitespace change is egregious.

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

Done.

   statement.BindInt64(0, id);
   if (!statement.Run())
     return false;
 
   statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
-      "DELETE FROM favicon_bitmaps WHERE icon_id = ?"));
+      "DELETE FROM favicon_bitmaps WHERE icon_id=?"));

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

Also here.

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

Done.

   statement.BindInt64(0, id);
   return statement.Run();
 }
@@ -1070,6 +1147,8 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
       "ON (old.id = mapping.old_icon_id)";
   const char kDropOldFaviconsTable[] = "DROP TABLE old_favicons";
 
+  // Note that we allow the last_requested field to be reset to the default
+  // value (0).
   const char kRenameFaviconBitmapsTable[] =
       "ALTER TABLE favicon_bitmaps RENAME TO old_favicon_bitmaps";
   const char kCopyFaviconBitmaps[] =
@@ -1177,7 +1256,7 @@ sql::InitStatus ThumbnailDatabase::InitImpl(const base::FilePath& db_name) {
 #endif
 
   // thumbnails table has been obsolete for a long time, remove any
-  // detrious.
+  // detritus.

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

That's just embarrassing!

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

:)

   ignore_result(db_.Execute("DROP TABLE IF EXISTS thumbnails"));
 
   // At some point, operations involving temporary tables weren't done
@@ -1235,6 +1314,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.";
 
@@ -1315,6 +1400,12 @@ bool ThumbnailDatabase::UpgradeToVersion7() {
   return true;
 }
 
+bool ThumbnailDatabase::UpgradeToVersion8() {

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

This should include the schema to do the upgrade.

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

Done.

+  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");
 }