Database usage adjustment for .../history

chrome/browser/history/top_sites_database.cc

Index: chrome/browser/history/top_sites_database.cc
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index b4819328b816a426d1f1b82f3f403cac5527182d..90d4d3ef1617de71c4b7b07f5e9e271cd899f494 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -121,7 +121,7 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
       "boring_score, good_clipping, at_top, last_updated, load_completed "
       "FROM thumbnails ORDER BY url_rank "));
 
-  if (!statement) {
+  if (!statement.is_valid()) {
     LOG(WARNING) << db_->GetErrorMessage();
     return;
   }
@@ -198,7 +198,7 @@ void TopSitesDatabase::UpdatePageThumbnail(
       "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, "
       "load_completed = ? "
       "WHERE url = ? "));
-  if (!statement)
+  if (!statement.is_valid())

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

Is this still necessary?

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

Done.

     return;
 
   statement.BindString16(0, url.title);
@@ -229,7 +229,7 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
       "(url, url_rank, title, thumbnail, redirects, "
       "boring_score, good_clipping, at_top, last_updated, load_completed) "
       "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
-  if (!statement)
+  if (!statement.is_valid())
     return;

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

Still necessary?

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

Done.

 
   statement.BindString(0, url.url.spec());
@@ -279,8 +279,7 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(
         "WHERE url_rank >= ? AND url_rank < ?"));
     shift_statement.BindInt(0, new_rank);
     shift_statement.BindInt(1, prev_rank);
-    if (shift_statement)
-      shift_statement.Run();
+    shift_statement.Run();
   } else if (prev_rank < new_rank) {
     // Shift down
     sql::Statement shift_statement(db_->GetCachedStatement(
@@ -290,8 +289,7 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(
         "WHERE url_rank > ? AND url_rank <= ?"));
     shift_statement.BindInt(0, prev_rank);
     shift_statement.BindInt(1, new_rank);
-    if (shift_statement)
-      shift_statement.Run();
+    shift_statement.Run();
   }
 
   // Set the url's rank.
@@ -302,8 +300,7 @@ void TopSitesDatabase::UpdatePageRankNoTransaction(
       "WHERE url == ?"));
   set_statement.BindInt(0, new_rank);
   set_statement.BindString(1, url.url.spec());
-  if (set_statement)
-    set_statement.Run();
+  set_statement.Run();
 }
 
 bool TopSitesDatabase::GetPageThumbnail(const GURL& url,
@@ -312,12 +309,6 @@ bool TopSitesDatabase::GetPageThumbnail(const GURL& url,
       SQL_FROM_HERE,
       "SELECT thumbnail, boring_score, good_clipping, at_top, last_updated "
       "FROM thumbnails WHERE url=?"));
-
-  if (!statement) {
-    LOG(WARNING) << db_->GetErrorMessage();
-    return false;
-  }
-
   statement.BindString(0, url.spec());
   if (!statement.Step())
     return false;
@@ -334,37 +325,25 @@ bool TopSitesDatabase::GetPageThumbnail(const GURL& url,
 }
 
 int TopSitesDatabase::GetRowCount() {
-  int result = 0;
   sql::Statement select_statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT COUNT (url) FROM thumbnails"));
-  if (!select_statement) {
-    LOG(WARNING) << db_->GetErrorMessage();
-    return result;
-  }
-
   if (select_statement.Step())
-    result = select_statement.ColumnInt(0);
+    return select_statement.ColumnInt(0);

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

AFAICT, having this return anything other than a single row with a single
integer would be an error, as COUNT(...) can't return NULL or an empty result.

OTOH, I guess it's more consistent this way.  OK.

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

This is returning 0 on error, which is a bit of an error-swallowing misfeature,
but in this case I'm guessing it ends up not mattering?

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

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

Yeah, I think it doesn't matter (or, at least, it's probably one of dozens of
cases like this which we haven't noticed, yet).

On 2012/01/04 19:20:20, Greg Billock wrote:

 
-  return result;
+  return 0;
 }
 
 int TopSitesDatabase::GetURLRank(const MostVisitedURL& url) {
-  int result = -1;
   sql::Statement select_statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT url_rank "
       "FROM thumbnails WHERE url=?"));
-  if (!select_statement) {
-    LOG(WARNING) << db_->GetErrorMessage();
-    return result;
-  }
-
   select_statement.BindString(0, url.url.spec());
   if (select_statement.Step())
-    result = select_statement.ColumnInt(0);
+    return select_statement.ColumnInt(0);
 
-  return result;
+  return -1;
 }
 
 // Remove the record for this URL. Returns true iff removed successfully.
@@ -381,18 +360,18 @@ bool TopSitesDatabase::RemoveURL(const MostVisitedURL& url) {
       "UPDATE thumbnails "
       "SET url_rank = url_rank - 1 "
       "WHERE url_rank > ?"));
-  if (!shift_statement)
-    return false;
   shift_statement.BindInt(0, old_rank);
-  shift_statement.Run();
+
+  if (!shift_statement.Run())
+    return false;
 
   sql::Statement delete_statement(
       db_->GetCachedStatement(SQL_FROM_HERE,
                               "DELETE FROM thumbnails WHERE url = ?"));
-  if (!delete_statement)
-    return false;
   delete_statement.BindString(0, url.url.spec());
-  delete_statement.Run();
+
+  if (!delete_statement.Run())
+    return false;
 
   return transaction.Commit();
 }