Database usage adjustment for .../history

chrome/browser/history/thumbnail_database.cc

 // Copyright (c) 2011 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 158 matching lines @@
   const char* alterations[] = {
     "ALTER TABLE thumbnails ADD boring_score DOUBLE DEFAULT 1.0",
     "ALTER TABLE thumbnails ADD good_clipping INTEGER DEFAULT 0",
     "ALTER TABLE thumbnails ADD at_top INTEGER DEFAULT 0",
     "ALTER TABLE thumbnails ADD last_updated INTEGER DEFAULT 0",
     NULL
   };
 
   for (int i = 0; alterations[i] != NULL; ++i) {
     if (!db_.Execute(alterations[i])) {
-      NOTREACHED();
       return false;
     }
   }
 
   meta_table_.SetVersionNumber(3);
   meta_table_.SetCompatibleVersionNumber(std::min(3, kCompatibleVersionNumber));
   return true;
 }
 
 bool ThumbnailDatabase::RecreateThumbnailTable() {
@@ skipping 42 matching lines @@
 void ThumbnailDatabase::CommitTransaction() {
   db_.CommitTransaction();
 }
 
 void ThumbnailDatabase::Vacuum() {
   DCHECK(db_.transaction_nesting() == 0) <<
       "Can not have a transaction when vacuuming.";
   ignore_result(db_.Execute("VACUUM"));
 }
 
-void ThumbnailDatabase::SetPageThumbnail(
+bool ThumbnailDatabase::SetPageThumbnail(
     const GURL& url,
     URLID id,
     const gfx::Image* thumbnail,
     const ThumbnailScore& score,
     base::Time time) {
   if (use_top_sites_) {
     LOG(WARNING) << "Use TopSites instead.";
-    return;  // Not possible after migration to TopSites.
+    return false;  // Not possible after migration to TopSites.
   }
 
-  if (thumbnail) {
-    bool add_thumbnail = true;
-    ThumbnailScore current_score;
-    if (ThumbnailScoreForId(id, &current_score)) {
-      add_thumbnail = ShouldReplaceThumbnailWith(current_score, score);
-    }
+  if (!thumbnail)
+    return DeleteThumbnail(id);
 
-    if (add_thumbnail) {
-      sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
-          "INSERT OR REPLACE INTO thumbnails "
-          "(url_id, boring_score, good_clipping, at_top, last_updated, data) "
-          "VALUES (?,?,?,?,?,?)"));
-      if (!statement)
-        return;
+  bool add_thumbnail = true;
+  ThumbnailScore current_score;
+  if (ThumbnailScoreForId(id, &current_score)) {
+    add_thumbnail = ShouldReplaceThumbnailWith(current_score, score);
+  }
 
-      std::vector<unsigned char> jpeg_data;
-      bool encoded = gfx::JPEGEncodedDataFromImage(*thumbnail, kImageQuality,
-                                                   &jpeg_data);
-      if (encoded) {
-        statement.BindInt64(0, id);
-        statement.BindDouble(1, score.boring_score);
-        statement.BindBool(2, score.good_clipping);
-        statement.BindBool(3, score.at_top);
-        statement.BindInt64(4, score.time_at_snapshot.ToTimeT());
-        statement.BindBlob(5, &jpeg_data[0],
-                           static_cast<int>(jpeg_data.size()));
-        if (!statement.Run())
-          NOTREACHED() << db_.GetErrorMessage();
-      }
+  if (!add_thumbnail)
+    return true;
 
-      // Publish the thumbnail to any indexers listening to us.
-      // The tests may send an invalid url. Hence avoid publishing those.
-      if (url.is_valid() && history_publisher_ != NULL)
-        history_publisher_->PublishPageThumbnail(jpeg_data, url, time);
-    }
-  } else {
-    if (!DeleteThumbnail(id) )
-      DLOG(WARNING) << "Unable to delete thumbnail";
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "INSERT OR REPLACE INTO thumbnails "
+      "(url_id, boring_score, good_clipping, at_top, last_updated, data) "
+      "VALUES (?,?,?,?,?,?)"));
+  if (!statement.is_valid())
+    return false;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Is this likely enough to happen to be worth bothering with?  Logically it seems
like statement should be under if(encoded) anyhow.

[Greg Billock, 2012/01/04 19:20:20]

Agreed.

+
+  std::vector<unsigned char> jpeg_data;
+  bool encoded = gfx::JPEGEncodedDataFromImage(
+      *thumbnail, kImageQuality, &jpeg_data);
+  if (encoded) {
+    statement.BindInt64(0, id);
+    statement.BindDouble(1, score.boring_score);
+    statement.BindBool(2, score.good_clipping);
+    statement.BindBool(3, score.at_top);
+    statement.BindInt64(4, score.time_at_snapshot.ToTimeT());
+    statement.BindBlob(5, &jpeg_data[0],
+                       static_cast<int>(jpeg_data.size()));

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Sigh, the parameter should have been a size_t in the first place...

[Greg Billock, 2012/01/04 19:20:20]

Shall I add a todo?

[Scott Hess - ex-Googler, 2012/01/10 22:04:56]

Over in BindBlob()?  I think that makes sense.  SQLite doesn't seem to make use
of size_t, but I think it would make sense for sql/ to provide that interface,
rather than exporting it to the caller.

+    if (!statement.Run())
+      return false;
   }
+
+  // Publish the thumbnail to any indexers listening to us.
+  // The tests may send an invalid url. Hence avoid publishing those.
+  if (url.is_valid() && history_publisher_ != NULL)
+    history_publisher_->PublishPageThumbnail(jpeg_data, url, time);
+
+  return true;
 }
 
 bool ThumbnailDatabase::GetPageThumbnail(URLID id,
                                          std::vector<unsigned char>* data) {
   if (use_top_sites_) {
     LOG(WARNING) << "Use TopSites instead.";
     return false;  // Not possible after migration to TopSites.
   }
 
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT data FROM thumbnails WHERE url_id=?"));
-  if (!statement)
-    return false;
+  statement.BindInt64(0, id);
 
-  statement.BindInt64(0, id);
   if (!statement.Step())
     return false;  // don't have a thumbnail for this ID
 
   statement.ColumnBlobAsVector(0, data);
   return true;
 }
 
 bool ThumbnailDatabase::DeleteThumbnail(URLID id) {
   if (use_top_sites_) {
     return true;  // Not possible after migration to TopSites.
   }
 
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM thumbnails WHERE url_id = ?"));
-  if (!statement)
-    return false;
+  statement.BindInt64(0, id);
 
-  statement.BindInt64(0, id);
   return statement.Run();
 }
 
 bool ThumbnailDatabase::ThumbnailScoreForId(URLID id,
                                             ThumbnailScore* score) {
   if (use_top_sites_) {
     LOG(WARNING) << "Use TopSites instead.";
     return false;  // Not possible after migration to TopSites.
   }
+  DCHECK(score);

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

If you're going to add this, push it above the if() so that you're testing the
API's static usage, which presumably shouldn't change based on use_top_sites_.

[Greg Billock, 2012/01/04 19:20:20]

Done.

 
   // Fetch the current thumbnail's information to make sure we
   // aren't replacing a good thumbnail with one that's worse.
   sql::Statement select_statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT boring_score, good_clipping, at_top, last_updated "
       "FROM thumbnails WHERE url_id=?"));
-  if (!select_statement) {
-    NOTREACHED() << "Couldn't build select statement!";
-  } else {
-    select_statement.BindInt64(0, id);
-    if (select_statement.Step()) {
-      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;
-    }
-  }
+  select_statement.BindInt64(0, id);
 
-  return false;
+  if (!select_statement.Step())
+    return false;
+
+  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,
                                    scoped_refptr<RefCountedMemory> icon_data,
                                    base::Time time) {
   DCHECK(icon_id);
   if (icon_data->size()) {
     sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
         "UPDATE favicons SET image_data=?, last_updated=? WHERE id=?"));
-    if (!statement)
-      return 0;
-
     statement.BindBlob(0, icon_data->front(),
                        static_cast<int>(icon_data->size()));
     statement.BindInt64(1, time.ToTimeT());
     statement.BindInt64(2, icon_id);
+
     return statement.Run();
   } else {
     sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
         "UPDATE favicons SET image_data=NULL, last_updated=? WHERE id=?"));

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

This would probably be cleaner with a single statement using BindNull(0). 
Please put a TODO(shess): to that end, if you don't care to go there.

[Greg Billock, 2012/01/04 19:20:20]

Done.

-    if (!statement)
-      return 0;
-
     statement.BindInt64(0, time.ToTimeT());
     statement.BindInt64(1, icon_id);
+
     return statement.Run();
   }
 }
 
 bool ThumbnailDatabase::SetFaviconLastUpdateTime(FaviconID icon_id,
                                                  base::Time time) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE favicons SET last_updated=? WHERE id=?"));
-  if (!statement)
-    return 0;
-
   statement.BindInt64(0, time.ToTimeT());
   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"));
-  if (!statement)
-    return 0;
-
   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,
     GURL* icon_url) {
   DCHECK(icon_id);
 
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT last_updated, image_data, url FROM favicons WHERE id=?"));
-  if (!statement)
-    return 0;
-
   statement.BindInt64(0, icon_id);
 
   if (!statement.Step())
     return false;  // No entry for the id.
 
   *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));
 
   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 (?, ?)"));
-  if (!statement)
-    return 0;
-
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(icon_url));
   statement.BindInt(1, icon_type);
+
   if (!statement.Run())
     return 0;
   return db_.GetLastInsertRowId();
 }
 
 bool ThumbnailDatabase::DeleteFavicon(FaviconID id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM favicons WHERE id = ?"));
-  if (!statement)
-    return false;
+  statement.BindInt64(0, 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 12 matching lines @@
 bool ThumbnailDatabase::GetIconMappingsForPageURL(
     const GURL& page_url,
     std::vector<IconMapping>* mapping_data) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT icon_mapping.id, icon_mapping.icon_id, favicons.icon_type "
       "FROM icon_mapping "
       "INNER JOIN favicons "
       "ON icon_mapping.icon_id = favicons.id "
       "WHERE icon_mapping.page_url=? "
       "ORDER BY favicons.icon_type DESC"));
-  if (!statement)
-    return false;
-
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(page_url));
 
   bool result = false;
   while (statement.Step()) {
     result = true;
     if (!mapping_data)
       return result;
 
     IconMapping icon_mapping;
     FillIconMapping(statement, page_url, &icon_mapping);
     mapping_data->push_back(icon_mapping);
   }
   return result;
 }
 
 IconMappingID ThumbnailDatabase::AddIconMapping(const GURL& page_url,
                                                 FaviconID icon_id) {
   return AddIconMapping(page_url, icon_id, false);
 }
 
 bool ThumbnailDatabase::UpdateIconMapping(IconMappingID mapping_id,
                                           FaviconID icon_id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "UPDATE icon_mapping SET icon_id=? WHERE id=?"));
-  if (!statement)
-    return 0;
-
   statement.BindInt64(0, icon_id);
   statement.BindInt64(1, mapping_id);
+
   return statement.Run();
 }
 
 bool ThumbnailDatabase::DeleteIconMappings(const GURL& page_url) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM icon_mapping WHERE page_url = ?"));
-  if (!statement)
-    return false;
+  statement.BindString(0, URLDatabase::GURLToDatabaseURL(page_url));
 
-  statement.BindString(0, URLDatabase::GURLToDatabaseURL(page_url));
   return statement.Run();
 }
 
 bool ThumbnailDatabase::HasMappingFor(FaviconID id) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT id FROM icon_mapping "
       "WHERE icon_id=?"));
-  if (!statement)
-    return false;
+  statement.BindInt64(0, id);
 
-  statement.BindInt64(0, id);
   return statement.Step();
 }
 
 bool ThumbnailDatabase::CloneIconMapping(const GURL& old_page_url,
                                          const GURL& new_page_url) {
   sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
       "SELECT icon_id FROM icon_mapping "
       "WHERE page_url=?"));
-  if (!statement)
+  if (!statement.is_valid())
     return false;
 
   // Do nothing if there are existing bindings
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(new_page_url));
   if (statement.Step())
     return true;
 
   statement.Assign(db_.GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO icon_mapping (page_url, icon_id) "
         "SELECT ?, icon_id FROM icon_mapping "
         "WHERE page_url = ?"));
-  if (!statement)
+  if (!statement.is_valid())
     return false;

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

Is this still necessary?

[Greg Billock, 2012/01/04 19:20:20]

Removed.

On 2012/01/03 23:41:57, shess wrote:

 
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(new_page_url));
   statement.BindString(1, URLDatabase::GURLToDatabaseURL(old_page_url));
   return statement.Run();
 }
 
 
 bool ThumbnailDatabase::MigrateIconMappingData(URLDatabase* url_db) {
   URLDatabase::IconMappingEnumerator e;
   if (!url_db->InitIconMappingEnumeratorForEverything(&e))
@@ skipping 24 matching lines @@
 
   // 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 = ?"));
-  if (!statement)
-    return 0;
   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"))
@@ skipping 13 matching lines @@
 
 bool ThumbnailDatabase::RenameAndDropThumbnails(const FilePath& old_db_file,
                                                 const FilePath& new_db_file) {
   // Init favicons table - same schema as the thumbnails.
   sql::Connection favicons;
   if (OpenDatabase(&favicons, new_db_file) != sql::INIT_OK)
     return false;
 
   if (!InitFaviconsTable(&favicons, false) ||
       !InitIconMappingTable(&favicons, false)) {
-    NOTREACHED() << "Couldn't init favicons and icon-mapping table.";
     favicons.Close();
     return false;
   }
   favicons.Close();
 
   // Can't attach within a transaction.
   if (transaction_nesting())
     CommitTransaction();
 
   // Attach new DB.
   {
     // This block is needed because otherwise the attach statement is
     // never cleared from cache and we can't close the DB :P
     sql::Statement attach(db_.GetUniqueStatement("ATTACH ? AS new_favicons"));
-    if (!attach) {
-      NOTREACHED() << "Unable to attach database.";
+    if (!attach.is_valid()) {
+      DLOG(FATAL) << "Unable to attach database.";
       // Keep the transaction open, even though we failed.
       BeginTransaction();
       return false;
     }
 
 #if defined(OS_POSIX)
     attach.BindString(0, new_db_file.value());
 #else
     attach.BindString(0, WideToUTF8(new_db_file.value()));
 #endif
 
     if (!attach.Run()) {
-      NOTREACHED() << db_.GetErrorMessage();
+      DLOG(FATAL) << db_.GetErrorMessage();

[Scott Hess - ex-Googler, 2012/01/03 23:41:57]

I can't tell if the Execute() cases would already be fatal, but wouldn't this
and the above already be fatal?

[Greg Billock, 2012/01/04 19:20:20]

I think you are right. We have a TODO to DLOG(FATAL) once any noncompliant
clients are converted to ExecuteAndReturnErrorCode. Shall I rely on that and
remove these? It is DLOG(ERROR) now...

[Scott Hess - ex-Googler, 2012/01/10 22:04:56]

If Execute() is currently not doing FATAL, then leave those for now and they can
be dealt with if/when someone gets up speed to deal with that case.  But
redundant fatals on Run and Step are redundant.

       BeginTransaction();
       return false;
     }
   }
 
   // Move favicons to the new DB.
   if (!db_.Execute("INSERT OR REPLACE INTO new_favicons.favicons "
                    "SELECT * FROM favicons")) {
-    NOTREACHED() << "Unable to copy favicons.";
+    DLOG(FATAL) << "Unable to copy favicons.";
     BeginTransaction();
     return false;
   }
 
   if (!db_.Execute("DETACH new_favicons")) {
-    NOTREACHED() << "Unable to detach database.";
+    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;
 
@@ skipping 45 matching lines @@
   const char* statement_name =
       is_temporary ? "add_temp_icon_mapping" : "add_icon_mapping";
 
   std::string sql;
   sql.append("INSERT INTO ");
   sql.append(name);
   sql.append("(page_url, icon_id) VALUES (?, ?)");
 
   sql::Statement statement(
       db_.GetCachedStatement(sql::StatementID(statement_name), sql.c_str()));
-  if (!statement)
-    return 0;
-
   statement.BindString(0, URLDatabase::GURLToDatabaseURL(page_url));
   statement.BindInt64(1, icon_id);
 
   if (!statement.Run())
     return 0;
 
   return db_.GetLastInsertRowId();
 }
 
 bool ThumbnailDatabase::IsLatestVersion() {
   return meta_table_.GetVersionNumber() == kCurrentVersionNumber;
 }
 
 bool ThumbnailDatabase::UpgradeToVersion4() {
   // Set the default icon type as favicon, so the current data are set
   // correctly.
   if (!db_.Execute("ALTER TABLE favicons ADD icon_type INTEGER DEFAULT 1")) {
-    NOTREACHED();
     return false;
   }
   meta_table_.SetVersionNumber(4);
   meta_table_.SetCompatibleVersionNumber(std::min(4, kCompatibleVersionNumber));
   return true;
 }
 
 bool ThumbnailDatabase::UpgradeToVersion5() {
   if (!db_.Execute("ALTER TABLE favicons ADD sizes LONGVARCHAR")) {
-    NOTREACHED();
     return false;
   }
   meta_table_.SetVersionNumber(5);
   meta_table_.SetCompatibleVersionNumber(std::min(5, kCompatibleVersionNumber));
   return true;
 }
 
 }  // namespace history