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

components/history/core/browser/thumbnail_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/thumbnail_database.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 40 matching lines @@
 //                    the link tag. The FAVICON type is used for the default
 //                    favicon.ico favicon.
 //
 // favicon_bitmaps    This table contains the PNG encoded bitmap data of the
 //                    favicons. There is a separate row for every size in a
 //                    multi resolution bitmap. The bitmap data is associated
 //                    to the favicon via the |icon_id| field which matches
 //                    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 {
 
 // For this database, schema migrations are deprecated after two
 // years.  This means that the oldest non-deprecated version should be
 // two years old or greater (thus the migrations to get there are
 // older).  Databases containing deprecated versions will be cleared
 // at startup.  Since this database is a cache, losing old data is not
 // 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
 // Version 4: 5f104d76/r77288 by sky@chromium.org on 2011-03-08 (deprecated)
 // Version 3: 09911bf3/r15 by initial.commit on 2008-07-26 (deprecated)
 
 // 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.
 
 void FillIconMapping(const sql::Statement& statement,
                      const GURL& page_url,
                      IconMapping* icon_mapping) {
   icon_mapping->mapping_id = statement.ColumnInt64(0);
   icon_mapping->icon_id = statement.ColumnInt64(1);
   icon_mapping->icon_type =
       static_cast<favicon_base::IconType>(statement.ColumnInt(2));
@@ skipping 201 matching lines @@
       ")";
   if (!db->Execute(kFaviconsSql))
     return false;
 
   const char kFaviconBitmapsSql[] =
       "CREATE TABLE IF NOT EXISTS favicon_bitmaps"
       "("
       "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"
       ")";
   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;
 }
 
 // NOTE(shess): Schema modifications must consider initial creation in
 // |InitImpl()|, recovery in |RecoverDatabaseOrRaze()|, and history pruning in
 // |RetainDataForPageUrls()|.
 bool InitIndices(sql::Connection* db) {
   const char kIconMappingUrlIndexSql[] =
       "CREATE INDEX IF NOT EXISTS icon_mapping_page_url_idx"
       " ON icon_mapping(page_url)";
   const char kIconMappingIdIndexSql[] =
       "CREATE INDEX IF NOT EXISTS icon_mapping_icon_id_idx"
       " ON icon_mapping(icon_id)";
   if (!db->Execute(kIconMappingUrlIndexSql) ||
       !db->Execute(kIconMappingIdIndexSql)) {
     return false;
   }
 
   const char kFaviconsIndexSql[] =
       "CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)";
   if (!db->Execute(kFaviconsIndexSql))
     return false;
 
   const char kFaviconBitmapsIndexSql[] =
       "CREATE INDEX IF NOT EXISTS favicon_bitmaps_icon_id ON "
       "favicon_bitmaps(icon_id)";
   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;
 }
 
 enum RecoveryEventType {
   RECOVERY_EVENT_RECOVERED = 0,
   RECOVERY_EVENT_FAILED_SCOPER,
   RECOVERY_EVENT_FAILED_META_VERSION_ERROR,  // obsolete
   RECOVERY_EVENT_FAILED_META_VERSION_NONE,  // obsolete
   RECOVERY_EVENT_FAILED_META_WRONG_VERSION6,  // obsolete
   RECOVERY_EVENT_FAILED_META_WRONG_VERSION5,  // obsolete
@@ skipping 32 matching lines @@
 // Recover the database to the extent possible, razing it if recovery
 // is not possible.
 // TODO(shess): This is mostly just a safe proof of concept.  In the
 // real world, this database is probably not worthwhile recovering, as
 // opposed to just razing it and starting over whenever corruption is
 // detected.  So this database is a good test subject.
 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();
 
   // For histogram purposes.
   size_t favicons_rows_recovered = 0;
   size_t favicon_bitmaps_rows_recovered = 0;
   size_t icon_mapping_rows_recovered = 0;
   int64 original_size = 0;
   base::GetFileSize(db_path, &original_size);
@@ skipping 33 matching lines @@
   // the code simple.  http://crbug.com/327485 for numbers.
   DCHECK_LE(kDeprecatedVersionNumber, 6);
   if (version <= 6) {
     sql::Recovery::Unrecoverable(recovery.Pass());
     RecordRecoveryEvent(RECOVERY_EVENT_DEPRECATED);
     return;
   }
 
   // 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;
   }
 
   // Recover to current schema version.
   sql::MetaTable recover_meta_table;
   if (!recover_meta_table.Init(recovery->db(), kCurrentVersionNumber,
                                kCompatibleVersionNumber)) {
     sql::Recovery::Rollback(recovery.Pass());
@@ skipping 142 matching lines @@
     if (status == sql::INIT_OK)
       return status;
 
     meta_table_.Reset();
     db_.Close();
   }
   return status;
 }
 
 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() {
   db_.BeginTransaction();
 }
 
 void ThumbnailDatabase::CommitTransaction() {
   db_.CommitTransaction();
 }
 
@@ skipping 89 matching lines @@
     *png_icon_data = data;
   }
 
   if (pixel_size) {
     *pixel_size = gfx::Size(statement.ColumnInt(2),
                             statement.ColumnInt(3));
   }
   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,
     base::Time time,
     const gfx::Size& pixel_size) {
   DCHECK(icon_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO favicon_bitmaps (icon_id, image_data, last_updated, width, "
       "height) VALUES (?, ?, ?, ?, ?)"));
   statement.BindInt64(0, icon_id);
@@ skipping 35 matching lines @@
     FaviconBitmapID bitmap_id,
     base::Time time) {
   DCHECK(bitmap_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE favicon_bitmaps SET last_updated=? WHERE id=?"));
   statement.BindInt64(0, time.ToInternalValue());
   statement.BindInt64(1, bitmap_id);
   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=?"));
   statement.BindInt64(0, bitmap_id);
   return statement.Run();
 }
 
 bool ThumbnailDatabase::SetFaviconOutOfDate(favicon_base::FaviconID icon_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE favicon_bitmaps SET last_updated=? WHERE icon_id=?"));
@@ skipping 64 matching lines @@
   favicon_base::FaviconID icon_id = AddFavicon(icon_url, icon_type);
   if (!icon_id || !AddFaviconBitmap(icon_id, icon_data, time, pixel_size))
     return 0;
 
   return icon_id;
 }
 
 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();
 }
 
 bool ThumbnailDatabase::GetIconMappingsForPageURL(
     const GURL& page_url,
     int required_icon_types,
     std::vector<IconMapping>* filtered_mapping_data) {
   std::vector<IconMapping> mapping_data;
   if (!GetIconMappingsForPageURL(page_url, &mapping_data))
@@ skipping 177 matching lines @@
   const char kRenameFaviconsTable[] =
       "ALTER TABLE favicons RENAME TO old_favicons";
   const char kCopyFavicons[] =
       "INSERT INTO favicons (id, url, icon_type) "
       "SELECT mapping.new_icon_id, old.url, old.icon_type "
       "FROM old_favicons AS old "
       "JOIN temp.icon_id_mapping AS mapping "
       "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[] =
       "INSERT INTO favicon_bitmaps "
       "  (icon_id, last_updated, image_data, width, height) "
       "SELECT mapping.new_icon_id, old.last_updated, "
       "    old.image_data, old.width, old.height "
       "FROM old_favicon_bitmaps AS old "
       "JOIN temp.icon_id_mapping AS mapping "
       "ON (old.icon_id = mapping.old_icon_id)";
@@ skipping 87 matching lines @@
 
   // TODO(shess): Failing Begin() implies that something serious is
   // wrong with the database.  Raze() may be in order.
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   // Exclude the thumbnails file from backups.
   base::mac::SetFileBackupExclusion(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
   // atomically and users have been stranded.  Drop those tables and
   // move on.
   // TODO(shess): Prove it?  Audit all cases and see if it's possible
   // that this implies non-atomic update, and should thus be handled
   // via the corruption handler.
   ignore_result(db_.Execute("DROP TABLE IF EXISTS temp_favicons"));
   ignore_result(db_.Execute("DROP TABLE IF EXISTS temp_favicon_bitmaps"));
@@ skipping 37 matching lines @@
     if (!UpgradeToVersion6())
       return CantUpgradeToVersion(cur_version);
   }
 
   if (cur_version == 6) {
     ++cur_version;
     if (!UpgradeToVersion7())
       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.";
 
   // Initialization is complete.
   if (!transaction.Commit())
     return sql::INIT_FAILURE;
 
   // Raze the database if the structure of the favicons database is not what
   // it should be. This error cannot be detected via the SQL error code because
   // the error code for running SQL statements against a database with missing
@@ skipping 60 matching lines @@
       db_.Execute("CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)");
 
   if (!success)
     return false;
 
   meta_table_.SetVersionNumber(7);
   meta_table_.SetCompatibleVersionNumber(std::min(7, kCompatibleVersionNumber));
   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");
 }
 
 }  // namespace history