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

chrome/browser/history/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 "chrome/browser/history/thumbnail_database.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/command_line.h"
@@ skipping 21 matching lines @@
   icon_mapping->mapping_id = statement.ColumnInt64(0);
   icon_mapping->icon_id = statement.ColumnInt64(1);
   icon_mapping->icon_type =
       static_cast<history::IconType>(statement.ColumnInt(2));
   icon_mapping->page_url = page_url;
 }
 
 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.
 static const int kImageQuality = 90;
 
 ThumbnailDatabase::IconMappingEnumerator::IconMappingEnumerator() {
 }
 
 ThumbnailDatabase::IconMappingEnumerator::~IconMappingEnumerator() {
 }
 
 bool ThumbnailDatabase::IconMappingEnumerator::GetNextIconMapping(
     IconMapping* icon_mapping) {
   if (!statement_.Step())
     return false;
   FillIconMapping(statement_, GURL(statement_.ColumnString(3)), icon_mapping);
   return true;
 }
 
 ThumbnailDatabase::ThumbnailDatabase()
     : history_publisher_(NULL),
       use_top_sites_(false) {
 }
 
 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;
 }
 
 ThumbnailDatabase::~ThumbnailDatabase() {
   // The DBCloseScoper will delete the DB and the cache.
 }
 
 sql::InitStatus ThumbnailDatabase::Init(
@@ skipping 11 matching lines @@
 
 #if defined(OS_MACOSX)
   // Exclude the thumbnails file from backups.
   base::mac::SetFileBackupExclusion(db_name);
 #endif
 
   // Create the tables.
   if (!meta_table_.Init(&db_, kCurrentVersionNumber,
                         kCompatibleVersionNumber) ||
       !InitThumbnailTable() ||
+      !InitFaviconBitmapsTable(&db_, false) ||
+      !InitFaviconBitmapsIndex() ||
       !InitFaviconsTable(&db_, false) ||
       !InitFaviconsIndex() ||
       !InitIconMappingTable(&db_, false) ||
       !InitIconMappingIndex()) {
     db_.Close();
     return sql::INIT_FAILURE;
   }
 
   // Version check. We should not encounter a database too old for us to handle
   // in the wild, so we try to continue in that case.
   if (meta_table_.GetCompatibleVersionNumber() > kCurrentVersionNumber) {
     LOG(WARNING) << "Thumbnail database is too new.";
     return sql::INIT_TOO_NEW;
   }
 
   int cur_version = meta_table_.GetVersionNumber();
   if (cur_version == 2) {
     ++cur_version;
     if (!UpgradeToVersion3())
       return CantUpgradeToVersion(cur_version);
   }
 
   if (cur_version == 3) {
     ++cur_version;
     if (!UpgradeToVersion4() || !MigrateIconMappingData(url_db))
       return CantUpgradeToVersion(cur_version);
   }
 
   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.";
 
   // Initialization is complete.
   if (!transaction.Commit()) {
     db_.Close();
     return sql::INIT_FAILURE;
   }
 
   return sql::INIT_OK;
@@ skipping 69 matching lines @@
   // Note: if you update the schema, don't forget to update
   // CopyToTemporaryFaviconTable as well.
   const char* name = is_temporary ? "temp_favicons" : "favicons";
   if (!db->DoesTableExist(name)) {
     std::string sql;
     sql.append("CREATE TABLE ");
     sql.append(name);
     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;
   }
   return true;
 }
 
 bool ThumbnailDatabase::InitFaviconsIndex() {
   // Add an index on the url column.
   return
       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();
 }
 
 void ThumbnailDatabase::CommitTransaction() {
   db_.CommitTransaction();
 }
 
 void ThumbnailDatabase::RollbackTransaction() {
   db_.RollbackTransaction();
@@ skipping 107 matching lines @@
   double current_boring_score = select_statement.ColumnDouble(0);
   bool current_clipping = select_statement.ColumnBool(1);
   bool current_at_top = select_statement.ColumnBool(2);
   base::Time last_updated =
       base::Time::FromTimeT(select_statement.ColumnInt64(3));
   *score = ThumbnailScore(current_boring_score, current_clipping,
                           current_at_top, last_updated);
   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()));
   } else {
     statement.BindNull(0);
   }
   statement.BindInt64(1, time.ToTimeT());
   statement.BindInt64(2, icon_id);
 
   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();
 }
 
 FaviconID ThumbnailDatabase::GetFaviconIDForFaviconURL(const GURL& icon_url,
                                                        int required_icon_type,
                                                        IconType* icon_type) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT id, icon_type FROM favicons WHERE url=? AND (icon_type & ? > 0) "
       "ORDER BY icon_type DESC"));
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url));
   statement.BindInt(1, required_icon_type);
 
   if (!statement.Step())
     return 0;  // not cached
 
   if (icon_type)
     *icon_type = static_cast<IconType>(statement.ColumnInt(1));
   return statement.ColumnInt64(0);
 }
 
 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;
 }
 
 FaviconID ThumbnailDatabase::AddFavicon(const GURL& icon_url,
                                         IconType icon_type) {
 
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO favicons (url, icon_type) VALUES (?, ?)"));
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url));
   statement.BindInt(1, icon_type);
 
   if (!statement.Run())
     return 0;
   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();
 }
 
 bool ThumbnailDatabase::GetIconMappingForPageURL(const GURL& page_url,
                                                  IconType required_icon_type,
                                                  IconMapping* icon_mapping) {
   std::vector<IconMapping> icon_mappings;
   if (!GetIconMappingsForPageURL(page_url, &icon_mappings))
     return false;
 
@@ skipping 111 matching lines @@
 
   IconMapping info;
   while (e.GetNextIconMapping(&info)) {
     // TODO: Using bulk insert to improve the performance.
     if (!AddIconMapping(info.page_url, info.icon_id))
       return false;
   }
   return true;
 }
 
+bool ThumbnailDatabase::InitTemporaryTables() {
+  return InitIconMappingTable(&db_, true) &&
+         InitFaviconsTable(&db_, true) &&
+         InitFaviconBitmapsTable(&db_, true);
+}
+
+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.

+  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 tables needs indices (the temporary tables don't have any).
+  return InitIconMappingIndex() &&
+         InitFaviconsIndex() &&
+         InitFaviconBitmapsIndex();
+}
+
 IconMappingID ThumbnailDatabase::AddToTemporaryIconMappingTable(
     const GURL& page_url, const FaviconID icon_id) {
   return AddIconMapping(page_url, icon_id, true);
 }
 
-bool ThumbnailDatabase::CommitTemporaryIconMappingTable() {
-  // Delete the old icon_mapping table.
-  if (!db_.Execute("DROP TABLE icon_mapping"))
-    return false;
-
-  // Rename the temporary one.
-  if (!db_.Execute("ALTER TABLE temp_icon_mapping RENAME TO icon_mapping"))
-    return false;
-
-  // The renamed table needs the index (the temporary table doesn't have one).
-  return InitIconMappingIndex();
-}
-
-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 = ?"));
+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();
-}
+  FaviconID new_favicon_id = db_.GetLastInsertRowId();
 
-bool ThumbnailDatabase::CommitTemporaryFaviconTable() {
-  // Delete the old favicons table.
-  if (!db_.Execute("DROP TABLE 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;
 
-  // Rename the temporary one.
-  if (!db_.Execute("ALTER TABLE temp_favicons RENAME TO favicons"))
-    return false;
-
-  // The renamed table needs the index (the temporary table doesn't have one).
-  return InitFaviconsIndex();
+  return new_favicon_id;
 }
 
 bool ThumbnailDatabase::NeedsMigrationToTopSites() {
   return !use_top_sites_;
 }
 
 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;
   }
   favicons.Close();
 
   // Can't attach within a transaction.
   if (transaction_nesting())
     CommitTransaction();
 
@@ skipping 13 matching lines @@
 #else
     attach.BindString(0, WideToUTF8(new_db_file.value()));
 #endif
 
     if (!attach.Run()) {
       BeginTransaction();
       return false;
     }
   }
 
-  // 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;
   }
 
   if (!db_.Execute("DETACH new_favicons")) {
     DLOG(FATAL) << "Unable to detach database.";
     BeginTransaction();
     return false;
   }
 
   db_.Close();
 
   // Reset the DB to point to new file.
   if (OpenDatabase(&db_, new_db_file) != sql::INIT_OK)
     return false;
 
   file_util::Delete(old_db_file, false);
 
   meta_table_.Reset();
   if (!meta_table_.Init(&db_, kCurrentVersionNumber, kCompatibleVersionNumber))
     return false;
 
-  if (!InitFaviconsIndex())
+  if (!InitFaviconBitmapsIndex() || !InitFaviconsIndex())
     return false;
 
   // Reopen the transaction.
   BeginTransaction();
   use_top_sites_ = true;
   return true;
 }
 
 bool ThumbnailDatabase::InitIconMappingTable(sql::Connection* db,
                                              bool is_temporary) {
@@ skipping 61 matching lines @@
 
 bool ThumbnailDatabase::UpgradeToVersion5() {
   if (!db_.Execute("ALTER TABLE favicons ADD sizes LONGVARCHAR")) {
     return false;
   }
   meta_table_.SetVersionNumber(5);
   meta_table_.SetCompatibleVersionNumber(std::min(5, kCompatibleVersionNumber));
   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