Database usage adjustment for .../history

chrome/browser/history/top_sites_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 "base/file_util.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/history/top_sites.h"
@@ skipping 103 matching lines @@
 }
 
 void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
                                          URLToImagesMap* thumbnails) {
   sql::Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "SELECT url, url_rank, title, thumbnail, redirects, "
       "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;
   }
 
   urls->clear();
   thumbnails->clear();
 
   while (statement.Step()) {
     // Results are sorted by url_rank.
     MostVisitedURL url;
@@ skipping 56 matching lines @@
 
 void TopSitesDatabase::UpdatePageThumbnail(
     const MostVisitedURL& url, const Images& thumbnail) {
   sql::Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "UPDATE thumbnails SET "
       "title = ?, thumbnail = ?, redirects = ?, "
       "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);
   if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) {
     statement.BindBlob(1, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
   statement.BindString(2, GetRedirects(url));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(3, score.boring_score);
   statement.BindBool(4, score.good_clipping);
   statement.BindBool(5, score.at_top);
   statement.BindInt64(6, score.time_at_snapshot.ToInternalValue());
   statement.BindBool(7, score.load_completed);
   statement.BindString(8, url.url.spec());
   if (!statement.Run())
     NOTREACHED() << db_->GetErrorMessage();

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

This line should be redundant?

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

Done.

 }
 
 void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
                                             int new_rank,
                                             const Images& thumbnail) {
   int count = GetRowCount();
 
   sql::Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "INSERT OR REPLACE INTO thumbnails "
       "(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());
   statement.BindInt(1, count);  // Make it the last url.
   statement.BindString16(2, url.title);
   if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) {
     statement.BindBlob(3, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
   statement.BindString(4, GetRedirects(url));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(5, score.boring_score);
   statement.BindBool(6, score.good_clipping);
   statement.BindBool(7, score.at_top);
   statement.BindInt64(8, score.time_at_snapshot.ToInternalValue());
   statement.BindBool(9, score.load_completed);
   if (!statement.Run())
     NOTREACHED() << db_->GetErrorMessage();

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

The error message is redundant?

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

Done. (Missed a bunch in this file, whether later adds or merges or whatever.
Once these are all in I'll do another sweep for laggards.)

 
   UpdatePageRankNoTransaction(url, new_rank);
 }
 
 void TopSitesDatabase::UpdatePageRank(const MostVisitedURL& url,
                                           int new_rank) {
   sql::Transaction transaction(db_.get());
   transaction.Begin();
   UpdatePageRankNoTransaction(url, new_rank);
   transaction.Commit();
@@ skipping 11 matching lines @@
   // Shift the ranks.
   if (prev_rank > new_rank) {
     // Shift up
     sql::Statement shift_statement(db_->GetCachedStatement(
         SQL_FROM_HERE,
         "UPDATE thumbnails "
         "SET url_rank = url_rank + 1 "
         "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(
         SQL_FROM_HERE,
         "UPDATE thumbnails "
         "SET url_rank = url_rank - 1 "
         "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.
   sql::Statement set_statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "UPDATE thumbnails "
       "SET url_rank = ? "
       "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,
                                             Images* thumbnail) {
   sql::Statement statement(db_->GetCachedStatement(
       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;
 
   std::vector<unsigned char> data;
   statement.ColumnBlobAsVector(0, &data);
   thumbnail->thumbnail = RefCountedBytes::TakeVector(&data);
   thumbnail->thumbnail_score.boring_score = statement.ColumnDouble(1);
   thumbnail->thumbnail_score.good_clipping = statement.ColumnBool(2);
   thumbnail->thumbnail_score.at_top = statement.ColumnBool(3);
   thumbnail->thumbnail_score.time_at_snapshot =
       base::Time::FromInternalValue(statement.ColumnInt64(4));
   return true;
 }
 
 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())
+    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:

 
-  if (select_statement.Step())
-    result = select_statement.ColumnInt(0);
-
-  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.
 bool TopSitesDatabase::RemoveURL(const MostVisitedURL& url) {
   int old_rank = GetURLRank(url);
   if (old_rank < 0)
     return false;
 
   sql::Transaction transaction(db_.get());
   transaction.Begin();
   // Decrement all following ranks.
   sql::Statement shift_statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "UPDATE thumbnails "
       "SET url_rank = url_rank - 1 "
       "WHERE url_rank > ?"));
-  if (!shift_statement)
+  shift_statement.BindInt(0, old_rank);
+
+  if (!shift_statement.Run())
     return false;
-  shift_statement.BindInt(0, old_rank);
-  shift_statement.Run();
 
   sql::Statement delete_statement(
       db_->GetCachedStatement(SQL_FROM_HERE,
                               "DELETE FROM thumbnails WHERE url = ?"));
-  if (!delete_statement)
+  delete_statement.BindString(0, url.url.spec());
+
+  if (!delete_statement.Run())
     return false;
-  delete_statement.BindString(0, url.url.spec());
-  delete_statement.Run();
 
   return transaction.Commit();
 }
 
 sql::Connection* TopSitesDatabase::CreateDB(const FilePath& db_name) {
   scoped_ptr<sql::Connection> db(new sql::Connection());
   // Settings copied from ThumbnailDatabase.
   db->set_error_delegate(GetErrorHandlerForThumbnailDb());
   db->set_page_size(4096);
   db->set_cache_size(32);
 
   if (!db->Open(db_name)) {
     LOG(ERROR) << db->GetErrorMessage();
     return NULL;
   }
 
   return db.release();
 }
 
 }  // namespace history