Changes favicon database to support storing bitmaps of different sizes for the same icon_url

chrome/browser/history/thumbnail_database.cc

Index: chrome/browser/history/thumbnail_database.cc
diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc
index 0397f961e3859bec530897d2e8c13f3bcc8d212f..e4e25cb983f795872db69881090aac265347e235 100644
--- a/chrome/browser/history/thumbnail_database.cc
+++ b/chrome/browser/history/thumbnail_database.cc
@@ -39,8 +39,8 @@ static void FillIconMapping(const sql::Statement& statement,
 namespace history {
 
 // Version number of the database.
-static const int kCurrentVersionNumber = 5;
-static const int kCompatibleVersionNumber = 5;
+static const int kCurrentVersionNumber = 6;
+static const int kCompatibleVersionNumber = 6;
 
 // Use 90 quality (out of 100) which is pretty high, because we're very
 // sensitive to artifacts for these small sized, highly detailed images.
@@ -66,7 +66,7 @@ ThumbnailDatabase::ThumbnailDatabase()
 }
 
 sql::InitStatus ThumbnailDatabase::CantUpgradeToVersion(int cur_version) {
-  LOG(WARNING) << "Unable to update to thumbnail database to version 4" <<
+  LOG(WARNING) << "Unable to update to thumbnail database to version " <<
                cur_version << ".";
   db_.Close();
   return sql::INIT_FAILURE;
@@ -98,6 +98,8 @@ sql::InitStatus ThumbnailDatabase::Init(
   if (!meta_table_.Init(&db_, kCurrentVersionNumber,
                         kCompatibleVersionNumber) ||
       !InitThumbnailTable() ||
+      !InitFaviconFramesTable(&db_, false) ||
+      !InitFaviconFramesIndex() ||
       !InitFaviconsTable(&db_, false) ||
       !InitFaviconsIndex() ||
       !InitIconMappingTable(&db_, false) ||
@@ -127,10 +129,17 @@ sql::InitStatus ThumbnailDatabase::Init(
   }
 
   if (cur_version == 4) {
+    ++cur_version;
     if (!UpgradeToVersion5())
       return CantUpgradeToVersion(cur_version);
   }
 
+  if (cur_version == 5) {
+    ++cur_version;
+    if (!UpgradeToVersion6())
+      return CantUpgradeToVersion(cur_version);
+  }
+
   LOG_IF(WARNING, cur_version < kCurrentVersionNumber) <<
       "Thumbnail database version " << cur_version << " is too old to handle.";
 
@@ -220,8 +229,6 @@ bool ThumbnailDatabase::InitFaviconsTable(sql::Connection* db,
     sql.append("("
                "id INTEGER PRIMARY KEY,"
                "url LONGVARCHAR NOT NULL,"
-               "last_updated INTEGER DEFAULT 0,"
-               "image_data BLOB,"
                // Set the default icon_type as FAVICON to be consistent with
                // table upgrade in UpgradeToVersion4().
                "icon_type INTEGER DEFAULT 1,"
@@ -238,6 +245,34 @@ bool ThumbnailDatabase::InitFaviconsIndex() {
       db_.Execute("CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)");
 }
 
+bool ThumbnailDatabase::InitFaviconFramesTable(sql::Connection* db,
+                                               bool is_temporary) {
+  // Note: if you update the schema, don't forget to update
+  // CopyToTemporaryFaviconFramesTable as well.
+  const char* name = is_temporary ? "temp_favicon_frames" : "favicon_frames";
+  if (!db->DoesTableExist(name)) {
+    std::string sql;
+    sql.append("CREATE TABLE ");
+    sql.append(name);
+    sql.append("("
+               "id INTEGER PRIMARY KEY,"
+               "icon_id INTEGER,"
+               "last_updated INTEGER DEFAULT 0,"
+               "image_data BLOB,"
+               "width INTEGER DEFAULT 0,"
+               "height INTEGER DEFAULT 0)");
+    if (!db->Execute(sql.c_str()))
+      return false;
+  }
+  return true;
+}
+
+bool ThumbnailDatabase::InitFaviconFramesIndex() {
+  // Add an index on the icon_id column.
+  return db_.Execute("CREATE INDEX IF NOT EXISTS favicon_frames_icon_id ON "
+                     "favicon_frames(icon_id)");
+}
+
 void ThumbnailDatabase::BeginTransaction() {
   db_.BeginTransaction();
 }
@@ -365,30 +400,30 @@ bool ThumbnailDatabase::ThumbnailScoreForId(URLID id,
   return true;
 }
 
-bool ThumbnailDatabase::SetFavicon(
+bool ThumbnailDatabase::AddFaviconFrame(
     URLID icon_id,
     scoped_refptr<base::RefCountedMemory> icon_data,
     base::Time time) {
   DCHECK(icon_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicons SET image_data=?, last_updated=? WHERE id=?"));
+      "INSERT INTO favicon_frames (icon_id, image_data, last_updated) VALUES "
+      "(?, ?, ?)"));
+  statement.BindInt64(0, icon_id);
   if (icon_data->size()) {
-    statement.BindBlob(0, icon_data->front(),
+    statement.BindBlob(1, icon_data->front(),
                        static_cast<int>(icon_data->size()));
   } else {
-    statement.BindNull(0);
+    statement.BindNull(1);
   }
-  statement.BindInt64(1, time.ToTimeT());
-  statement.BindInt64(2, icon_id);
+  statement.BindInt64(2, time.ToTimeT());
 
   return statement.Run();
 }
 
-bool ThumbnailDatabase::SetFaviconLastUpdateTime(FaviconID icon_id,
-                                                 base::Time time) {
+bool ThumbnailDatabase::SetFaviconOutOfDate(FaviconID icon_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicons SET last_updated=? WHERE id=?"));
-  statement.BindInt64(0, time.ToTimeT());
+      "UPDATE favicon_frames SET last_updated=? WHERE icon_id=?"));
+  statement.BindInt64(0, 0);
   statement.BindInt64(1, icon_id);
 
   return statement.Run();
@@ -419,9 +454,33 @@ bool ThumbnailDatabase::GetFavicon(
     IconType* icon_type) {
   DCHECK(icon_id);
 
+  if (!GetFaviconFrame(icon_id, last_updated, png_icon_data))
+    return false;
+
+  if (icon_url || icon_type) {
+    sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+        "SELECT url, icon_type FROM favicons WHERE id=?"));
+    statement.BindInt64(0, icon_id);
+
+    if (!statement.Step())
+      return false;  // No entry for the id.
+
+    if (icon_url)
+      *icon_url = GURL(statement.ColumnString(0));
+    if (icon_type)
+      *icon_type = static_cast<history::IconType>(statement.ColumnInt(1));
+  }
+
+  return true;
+}
+
+bool ThumbnailDatabase::GetFaviconFrame(
+    FaviconID icon_id,
+    base::Time* last_updated,
+    std::vector<unsigned char>* png_icon_data) {
+  DCHECK(icon_id);
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "SELECT last_updated, image_data, url, icon_type "
-      "FROM favicons WHERE id=?"));
+      "SELECT last_updated, image_data FROM favicon_frames WHERE icon_id=?"));
   statement.BindInt64(0, icon_id);
 
   if (!statement.Step())
@@ -431,11 +490,6 @@ bool ThumbnailDatabase::GetFavicon(
     *last_updated = base::Time::FromTimeT(statement.ColumnInt64(0));
   if (statement.ColumnByteLength(1) > 0)
     statement.ColumnBlobAsVector(1, png_icon_data);
-  if (icon_url)
-    *icon_url = GURL(statement.ColumnString(2));
-  if (icon_type)
-    *icon_type = static_cast<history::IconType>(statement.ColumnInt(3));
-
   return true;
 }
 
@@ -453,11 +507,19 @@ FaviconID ThumbnailDatabase::AddFavicon(const GURL& icon_url,
 }
 
 bool ThumbnailDatabase::DeleteFavicon(FaviconID id) {
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+  bool success = true;
+  sql::Statement statement;
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicons WHERE id = ?"));
   statement.BindInt64(0, id);
+  success &= statement.Run();

[sky, 2012/08/02 19:50:31]

If this fails, shouldn't this return?

 
-  return statement.Run();
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM favicon_frames WHERE icon_id = ?"));
+  statement.BindInt64(0, id);
+  success &= statement.Run();
+
+  return success;
 }
 
 bool ThumbnailDatabase::GetIconMappingForPageURL(const GURL& page_url,
@@ -608,9 +670,8 @@ bool ThumbnailDatabase::CommitTemporaryIconMappingTable() {
 
 FaviconID ThumbnailDatabase::CopyToTemporaryFaviconTable(FaviconID source) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "INSERT INTO temp_favicons (url, last_updated, image_data, icon_type)"
-      "SELECT url, last_updated, image_data, icon_type "
-      "FROM favicons WHERE id = ?"));
+      "INSERT INTO temp_favicons (url, icon_type, sizes) "
+      "SELECT url, icon_type, sizes FROM favicons WHERE id = ?"));
   statement.BindInt64(0, source);
 
   if (!statement.Run())
@@ -633,6 +694,30 @@ bool ThumbnailDatabase::CommitTemporaryFaviconTable() {
   return InitFaviconsIndex();
 }
 
+void ThumbnailDatabase::CopyToTemporaryFaviconFramesTable(
+    FaviconID source) {
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "INSERT INTO temp_favicon_frames (icon_id, last_updated, image_data, "
+      "width, height) "
+      "SELECT icon_id, last_updated, image_data, width, height "

[sky, 2012/08/02 19:50:31]

This is inserting the old id when it needs to insert the new id. Additionally
it's querying using 'id' when it should be using icon_id. Make sure you have
test coverage for this.

+      "FROM favicon_frames WHERE id = ?"));
+  statement.BindInt64(0, source);
+  statement.Run();
+}
+
+bool ThumbnailDatabase::CommitTemporaryFaviconFrameTable() {
+  // Delete the old favicon frames table.
+  if (!db_.Execute("DROP TABLE favicon_frames"))
+    return false;
+
+  // Rename the temporary one.
+  if (!db_.Execute("ALTER TABLE temp_favicon_frames RENAME TO favicon_frames"))
+    return false;
+
+  // The renamed table needs an index (the temporary table doesn't have one).
+  return InitFaviconFramesIndex();
+}
+
 bool ThumbnailDatabase::NeedsMigrationToTopSites() {
   return !use_top_sites_;
 }
@@ -644,7 +729,8 @@ bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
   if (OpenDatabase(&favicons, new_db_file) != sql::INIT_OK)
     return false;
 
-  if (!InitFaviconsTable(&favicons, false) ||
+  if (!InitFaviconFramesTable(&favicons, false) ||
+      !InitFaviconsTable(&favicons, false) ||
       !InitIconMappingTable(&favicons, false)) {
     favicons.Close();
     return false;
@@ -678,10 +764,14 @@ bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
     }
   }
 
-  // Move favicons to the new DB.
-  if (!db_.Execute("INSERT OR REPLACE INTO new_favicons.favicons "
-                   "SELECT * FROM favicons")) {
-    DLOG(FATAL) << "Unable to copy favicons.";
+  // Move favicons and frame_frames to new DB.
+  bool successfully_moved_data =
+     db_.Execute("INSERT OR REPLACE INTO new_favicons.favicon_frames "
+                 "SELECT * FROM favicon_frames") &&
+     db_.Execute("INSERT OR REPLACE INTO new_favicons.favicons "
+                 "SELECT * FROM favicons");
+  if (!successfully_moved_data) {
+    DLOG(FATAL) << "Unable to copy favicons and frame_frames.";
     BeginTransaction();
     return false;
   }
@@ -704,7 +794,7 @@ bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
   if (!meta_table_.Init(&db_, kCurrentVersionNumber, kCompatibleVersionNumber))
     return false;
 
-  if (!InitFaviconsIndex())
+  if (!InitFaviconFramesIndex() || !InitFaviconsIndex())
     return false;
 
   // Reopen the transaction.
@@ -786,4 +876,24 @@ bool ThumbnailDatabase::UpgradeToVersion5() {
   return true;
 }
 
+bool ThumbnailDatabase::UpgradeToVersion6() {
+  bool success =
+      db_.Execute("INSERT INTO favicon_frames (icon_id, last_updated, "
+                  "image_data)"
+                  "SELECT id, last_updated, image_data FROM favicons") &&

[sky, 2012/08/02 19:50:31]

You need to update width/height here too.

+      db_.Execute("CREATE TABLE temp_favicons ("
+                  "id INTEGER PRIMARY KEY,"
+                  "url LONGVARCHAR NOT NULL,"
+                  "icon_type INTEGER DEFAULT 1,"
+                  "sizes LONGVARCHAR)") &&
+      db_.Execute("INSERT INTO temp_favicons (id, url, icon_type) "
+                  "SELECT id, url, icon_type FROM favicons") &&
+      db_.Execute("DROP TABLE favicons") &&
+      db_.Execute("ALTER TABLE temp_favicons RENAME TO favicons");
+
+  meta_table_.SetVersionNumber(6);
+  meta_table_.SetCompatibleVersionNumber(std::min(6, kCompatibleVersionNumber));
+  return success;
+}
+
 }  // namespace history