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..af8eef90644a34fae7bd53ba09a777dd2bc7a95b 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() ||
+      !InitFaviconBitmapsTable(&db_, false) ||
+      !InitFaviconBitmapsIndex() ||
       !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,12 +229,10 @@ 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,"
-               "sizes LONGVARCHAR)");
+               "sizes LONGVARCHAR DEFAULT '0 0')");

[sky, 2012/08/16 16:12:57]

why 0 0 as a default?

[pkotwicz, 2012/08/16 19:22:46]

The default is 0x0 because it is a very recognizable value.
It is very clear that 0x0 is not the size of the bitmap whereas any other value
can be mistaken as such.
In http://codereview.chromium.org/10802066/, we store all bitmaps at 0x0 so that
we only put a single bitmap per favicon into the database. Using 0x0 will make
transitioning into a state where we store the real bitmap size easy.

[sky, 2012/08/16 20:01:41]

0x0 implies there is one known size, 0x0. That isn't the case though, there are
no known sizes. I think it should be an empty string.

     if (!db->Execute(sql.c_str()))
       return false;
   }
@@ -238,6 +245,34 @@ bool ThumbnailDatabase::InitFaviconsIndex() {
       db_.Execute("CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)");
 }
 
+bool ThumbnailDatabase::InitFaviconBitmapsTable(sql::Connection* db,
+                                                bool is_temporary) {
+  // Note: if you update the schema, don't forget to update
+  // CopyFaviconAndFaviconBitmapsToTemporaryTables as well.
+  const char* name = is_temporary ? "temp_favicon_bitmaps" : "favicon_bitmaps";
+  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::InitFaviconBitmapsIndex() {
+  // Add an index on the icon_id column.
+  return db_.Execute("CREATE INDEX IF NOT EXISTS favicon_bitmaps_icon_id ON "
+                     "favicon_bitmaps(icon_id)");
+}
+
 void ThumbnailDatabase::BeginTransaction() {
   db_.BeginTransaction();
 }
@@ -365,13 +400,71 @@ bool ThumbnailDatabase::ThumbnailScoreForId(URLID id,
   return true;
 }
 
-bool ThumbnailDatabase::SetFavicon(
-    URLID icon_id,
+bool ThumbnailDatabase::GetFaviconBitmaps(
+    FaviconID icon_id,
+    std::vector<FaviconBitmap>* favicon_bitmaps) {
+  DCHECK(icon_id);
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT id, last_updated, image_data, width, height FROM favicon_bitmaps "
+      "WHERE icon_id=?"));
+  statement.BindInt64(0, icon_id);
+
+  bool result = false;

[sky, 2012/08/16 16:12:57]

Why do we need a return value here? Can't they check the size of
favicon_bitmaps?

[pkotwicz, 2012/08/16 19:22:46]

We use return values in ThumbnailDatabase so that we can be lazy in unit tests.
In particular some unit tests do this:
EXPECT_FALSE(thumbnail_db_.GetFaviconBitmaps(icon_id, NULL)).
This is consistent with what we do for ThumbnailDatabase::GetIconMappings()

+  while (statement.Step()) {
+    result = true;
+    if (!favicon_bitmaps)
+      return result;
+
+    FaviconBitmap favicon_bitmap;
+    favicon_bitmap.bitmap_id = statement.ColumnInt64(0);
+    favicon_bitmap.icon_id = icon_id;
+    favicon_bitmap.last_updated =
+        base::Time::FromTimeT(statement.ColumnInt64(1));
+    if (statement.ColumnByteLength(2) > 0) {
+      scoped_refptr<base::RefCountedBytes> data = new base::RefCountedBytes();
+      statement.ColumnBlobAsVector(2, &data->data());
+      favicon_bitmap.bitmap_data = data;
+    }
+    favicon_bitmap.pixel_size = gfx::Size(statement.ColumnInt(3),
+                                          statement.ColumnInt(4));
+    favicon_bitmaps->push_back(favicon_bitmap);
+  }
+  return result;
+}
+
+FaviconBitmapID ThumbnailDatabase::AddFaviconBitmap(
+    FaviconID icon_id,
+    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);
+  if (icon_data->size()) {
+    statement.BindBlob(1, icon_data->front(),
+                       static_cast<int>(icon_data->size()));
+  } else {
+    statement.BindNull(1);
+  }
+  statement.BindInt64(2, time.ToTimeT());
+  statement.BindInt(3, pixel_size.width());
+  statement.BindInt(4, pixel_size.height());
+
+  if (!statement.Run())
+    return 0;
+  return db_.GetLastInsertRowId();
+}
+
+bool ThumbnailDatabase::SetFaviconBitmap(
+    FaviconID 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=?"));
+      "UPDATE favicon_bitmaps SET image_data=?, last_updated=? "

[sky, 2012/08/16 16:12:57]

This might reset multiple rows. Shouldn't the where be on id? Or if you really
need icon_id, shouldn't this remove then insert so that we know there is only
one row?

[pkotwicz, 2012/08/16 19:22:46]

Changed this to remove and insert as you suggested.
Yes, we should be passing in and using FaviconBitmapID. This is what the
followup CL does.

+      "WHERE icon_id=?"));
   if (icon_data->size()) {
     statement.BindBlob(0, icon_data->front(),
                        static_cast<int>(icon_data->size()));
@@ -384,11 +477,20 @@ bool ThumbnailDatabase::SetFavicon(
   return statement.Run();
 }
 
-bool ThumbnailDatabase::SetFaviconLastUpdateTime(FaviconID icon_id,
-                                                 base::Time time) {
+bool ThumbnailDatabase::SetFaviconSizes(FaviconID icon_id,
+                                        const std::string& sizes) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "UPDATE favicons SET last_updated=? WHERE id=?"));
-  statement.BindInt64(0, time.ToTimeT());
+      "UPDATE favicons SET sizes=? WHERE id=?"));
+  statement.BindString(0, sizes);
+  statement.BindInt64(1, icon_id);
+
+  return statement.Run();
+}
+
+bool ThumbnailDatabase::SetFaviconOutOfDate(FaviconID icon_id) {
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "UPDATE favicon_bitmaps SET last_updated=? WHERE icon_id=?"));
+  statement.BindInt64(0, 0);
   statement.BindInt64(1, icon_id);
 
   return statement.Run();
@@ -414,27 +516,46 @@ FaviconID ThumbnailDatabase::GetFaviconIDForFaviconURL(const GURL& icon_url,
 bool ThumbnailDatabase::GetFavicon(
     FaviconID icon_id,
     base::Time* last_updated,
-    std::vector<unsigned char>* png_icon_data,
+    scoped_refptr<base::RefCountedMemory>* png_icon_data,
     GURL* icon_url,
     IconType* icon_type) {
   DCHECK(icon_id);
 
+  std::vector<FaviconBitmap> favicon_bitmaps;
+  if (!GetFaviconBitmaps(icon_id, &favicon_bitmaps))
+    return false;
+
+  if (favicon_bitmaps.size() == 0)

[sky, 2012/08/16 16:12:57]

empty

[pkotwicz, 2012/08/16 19:22:46]

Done.

+    return false;
+
+  if (last_updated)
+    *last_updated = favicon_bitmaps[0].last_updated;
+
+  *png_icon_data = favicon_bitmaps[0].bitmap_data;
+
+  return GetFaviconHeader(icon_id, icon_url, icon_type, NULL);
+}
+
+bool ThumbnailDatabase::GetFaviconHeader(
+    FaviconID icon_id,
+    GURL* icon_url,
+    IconType* icon_type,
+    std::string* sizes) {
+  DCHECK(icon_id);
+
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-      "SELECT last_updated, image_data, url, icon_type "
-      "FROM favicons WHERE id=?"));
+      "SELECT url, icon_type, sizes FROM favicons WHERE id=?"));
   statement.BindInt64(0, icon_id);
 
   if (!statement.Step())
     return false;  // No entry for the id.
 
-  if (last_updated)
-    *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));
+    *icon_url = GURL(statement.ColumnString(0));
   if (icon_type)
-    *icon_type = static_cast<history::IconType>(statement.ColumnInt(3));
+    *icon_type = static_cast<history::IconType>(statement.ColumnInt(1));
+  if (sizes)
+    *sizes = statement.ColumnString(2);
 
   return true;
 }
@@ -452,11 +573,33 @@ FaviconID ThumbnailDatabase::AddFavicon(const GURL& icon_url,
   return db_.GetLastInsertRowId();
 }
 
+FaviconID ThumbnailDatabase::AddFavicon(
+    const GURL& icon_url,
+    IconType icon_type,
+    const std::string& sizes,
+    scoped_refptr<base::RefCountedMemory> icon_data,
+    base::Time time,
+    const gfx::Size& pixel_size) {
+  FaviconID icon_id = AddFavicon(icon_url, icon_type);
+  if (!icon_id ||
+      !SetFaviconSizes(icon_id, sizes) ||
+      !AddFaviconBitmap(icon_id, icon_data, time, pixel_size)) {
+    return 0;
+  }
+  return icon_id;
+}
+
 bool ThumbnailDatabase::DeleteFavicon(FaviconID id) {
-  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+  sql::Statement statement;
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicons WHERE id = ?"));
   statement.BindInt64(0, id);
+  if (!statement.Run())
+    return false;
 
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM favicon_bitmaps WHERE icon_id = ?"));
+  statement.BindInt64(0, id);
   return statement.Run();
 }
 
@@ -588,49 +731,73 @@ bool ThumbnailDatabase::MigrateIconMappingData(URLDatabase* url_db) {
   return true;
 }
 
-IconMappingID ThumbnailDatabase::AddToTemporaryIconMappingTable(
-    const GURL& page_url, const FaviconID icon_id) {
-  return AddIconMapping(page_url, icon_id, true);
+bool ThumbnailDatabase::InitTemporaryTables() {
+  return InitIconMappingTable(&db_, true) &&
+         InitFaviconsTable(&db_, true) &&
+         InitFaviconBitmapsTable(&db_, true);
 }
 
-bool ThumbnailDatabase::CommitTemporaryIconMappingTable() {
-  // Delete the old icon_mapping table.
-  if (!db_.Execute("DROP TABLE icon_mapping"))
-    return false;
+bool ThumbnailDatabase::CommitTemporaryTables() {
+  const char* main_tables[] = { "icon_mapping",
+                                "favicons",
+                                "favicon_bitmaps" };
+  const char* temporary_tables[] = { "temp_icon_mapping",
+                                     "temp_favicons",
+                                     "temp_favicon_bitmaps" };
 

[sky, 2012/08/16 16:12:57]

DCHECK_EQ(arraysize(main_tables), arraysize(temporary_tables))

[pkotwicz, 2012/08/16 19:22:46]

Done.

-  // Rename the temporary one.
-  if (!db_.Execute("ALTER TABLE temp_icon_mapping RENAME TO icon_mapping"))
-    return false;
+  for (size_t i = 0; i < arraysize(main_tables); ++i) {
+    // Delete the main table.
+    std::string sql;
+    sql.append("DROP TABLE ");
+    sql.append(main_tables[i]);
+    if (!db_.Execute(sql.c_str()))
+      return false;
+
+    // Rename the temporary table.
+    sql.clear();
+    sql.append("ALTER TABLE ");
+    sql.append(temporary_tables[i]);
+    sql.append(" RENAME TO ");
+    sql.append(main_tables[i]);
+    if (!db_.Execute(sql.c_str()))
+      return false;
+  }
 
-  // The renamed table needs the index (the temporary table doesn't have one).
-  return InitIconMappingIndex();
+  // The renamed tables needs indices (the temporary tables don't have any).
+  return InitIconMappingIndex() &&
+         InitFaviconsIndex() &&
+         InitFaviconBitmapsIndex();
 }
 
-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 = ?"));
+IconMappingID ThumbnailDatabase::AddToTemporaryIconMappingTable(
+    const GURL& page_url, const FaviconID icon_id) {
+  return AddIconMapping(page_url, icon_id, true);
+}
+
+FaviconID ThumbnailDatabase::CopyFaviconAndFaviconBitmapsToTemporaryTables(
+    FaviconID source) {
+  sql::Statement statement;
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
+      "INSERT INTO temp_favicons (url, icon_type, sizes) "
+      "SELECT url, icon_type, sizes FROM favicons WHERE id = ?"));
   statement.BindInt64(0, source);
 
   if (!statement.Run())
     return 0;
 
-  // We return the ID of the newly inserted favicon.
-  return db_.GetLastInsertRowId();
-}
-
-bool ThumbnailDatabase::CommitTemporaryFaviconTable() {
-  // Delete the old favicons table.
-  if (!db_.Execute("DROP TABLE favicons"))
-    return false;
+  FaviconID new_favicon_id = db_.GetLastInsertRowId();
 
-  // Rename the temporary one.
-  if (!db_.Execute("ALTER TABLE temp_favicons RENAME TO favicons"))
-    return false;
+  statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
+      "INSERT INTO temp_favicon_bitmaps (icon_id, last_updated, image_data, "
+      "width, height) "
+      "SELECT ?, last_updated, image_data, width, height "
+      "FROM favicon_bitmaps WHERE icon_id = ?"));
+  statement.BindInt64(0, new_favicon_id);
+  statement.BindInt64(1, source);
+  if (!statement.Run())
+    return 0;
 
-  // The renamed table needs the index (the temporary table doesn't have one).
-  return InitFaviconsIndex();
+  return new_favicon_id;
 }
 
 bool ThumbnailDatabase::NeedsMigrationToTopSites() {
@@ -639,12 +806,13 @@ bool ThumbnailDatabase::NeedsMigrationToTopSites() {
 
 bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
                                                 const FilePath& new_db_file) {
-  // Init favicons table - same schema as the thumbnails.
+  // Init favicons tables - same schema as the thumbnails.
   sql::Connection favicons;
   if (OpenDatabase(&favicons, new_db_file) != sql::INIT_OK)
     return false;
 
-  if (!InitFaviconsTable(&favicons, false) ||
+  if (!InitFaviconBitmapsTable(&favicons, false) ||
+      !InitFaviconsTable(&favicons, false) ||
       !InitIconMappingTable(&favicons, false)) {
     favicons.Close();
     return false;
@@ -678,10 +846,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 favicon_bitmaps to new DB.
+  bool successfully_moved_data =
+     db_.Execute("INSERT OR REPLACE INTO new_favicons.favicon_bitmaps "
+                 "SELECT * FROM favicon_bitmaps") &&
+     db_.Execute("INSERT OR REPLACE INTO new_favicons.favicons "
+                 "SELECT * FROM favicons");
+  if (!successfully_moved_data) {
+    DLOG(FATAL) << "Unable to copy favicons and bitmap_bitmaps.";
     BeginTransaction();
     return false;
   }
@@ -704,7 +876,7 @@ bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
   if (!meta_table_.Init(&db_, kCurrentVersionNumber, kCompatibleVersionNumber))
     return false;
 
-  if (!InitFaviconsIndex())
+  if (!InitFaviconBitmapsIndex() || !InitFaviconsIndex())
     return false;
 
   // Reopen the transaction.
@@ -786,4 +958,24 @@ bool ThumbnailDatabase::UpgradeToVersion5() {
   return true;
 }
 
+bool ThumbnailDatabase::UpgradeToVersion6() {
+  bool success =
+      db_.Execute("INSERT INTO favicon_bitmaps (icon_id, last_updated, "
+                  "image_data)"
+                  "SELECT id, last_updated, image_data FROM favicons") &&

[sky, 2012/08/16 16:12:57]

Don't you need to set the width/height?

[pkotwicz, 2012/08/16 19:22:46]

Done.

+      db_.Execute("CREATE TABLE temp_favicons ("
+                  "id INTEGER PRIMARY KEY,"
+                  "url LONGVARCHAR NOT NULL,"
+                  "icon_type INTEGER DEFAULT 1,"

[sky, 2012/08/16 16:12:57]

Document what 1 is here.

[pkotwicz, 2012/08/16 19:22:46]

Done.

+                  "sizes LONGVARCHAR DEFAULT '0 0')") &&

[sky, 2012/08/16 16:12:57]

Why default to 0x0?

[pkotwicz, 2012/08/16 19:22:46]

The default is 0x0 because it is a very recognizable value.
It is very clear that 0x0 is not the size of the bitmap whereas any other value
can be mistaken as such.
In http://codereview.chromium.org/10802066/, we store all bitmaps at 0x0 so that
we only put a single bitmap per favicon into the database. Using 0x0 will make
transitioning into a state where we store the real bitmap size easy.

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

[sky, 2012/08/16 16:12:57]

Early return if execute fails on the previous lines.

[pkotwicz, 2012/08/16 19:22:46]

Done.

+  meta_table_.SetCompatibleVersionNumber(std::min(6, kCompatibleVersionNumber));
+  return success;
+}
+
 }  // namespace history