Database usage adjustment for .../history

chrome/browser/history/url_database.cc

Index: chrome/browser/history/url_database.cc
diff --git a/chrome/browser/history/url_database.cc b/chrome/browser/history/url_database.cc
index 4a020b97bc924dd7d1b812797dc559c040aa9025..bb16352e9f82ac3324c8df85d575eb615ae74773 100644
--- a/chrome/browser/history/url_database.cc
+++ b/chrome/browser/history/url_database.cc
@@ -90,10 +90,8 @@ bool URLDatabase::GetURLRow(URLID url_id, URLRow* info) {
   // when all inputs are using GURL (which prohibit empty input).
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls WHERE id=?"));
-  if (!statement)
-    return false;
-
   statement.BindInt64(0, url_id);
+
   if (statement.Step()) {
     FillURLRow(statement, info);
     return true;
@@ -104,8 +102,6 @@ bool URLDatabase::GetURLRow(URLID url_id, URLRow* info) {
 bool URLDatabase::GetAllTypedUrls(std::vector<history::URLRow>* urls) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls WHERE typed_count > 0"));
-  if (!statement)
-    return false;

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

This can now return true in case of empty results.  Is that appropriate?

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

I think that's what this did before. The above should only return false in case
of invalid statement, and the .h file looks like the docs are right about that.

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

As it stands, it's always returning true, so if it is appropriate to succeed in
case of error, then it could have a void return.  Or maybe it should return
Succeeded()?

 
   while (statement.Step()) {
     URLRow info;
@@ -118,11 +114,9 @@ bool URLDatabase::GetAllTypedUrls(std::vector<history::URLRow>* urls) {
 URLID URLDatabase::GetRowForURL(const GURL& url, history::URLRow* info) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls WHERE url=?"));
-  if (!statement)
-    return 0;
-
   std::string url_string = GURLToDatabaseURL(url);
   statement.BindString(0, url_string);
+
   if (!statement.Step())
     return 0;  // no data
 
@@ -137,15 +131,13 @@ bool URLDatabase::UpdateURLRow(URLID url_id,
       "UPDATE urls SET title=?,visit_count=?,typed_count=?,last_visit_time=?,"
         "hidden=?"
       "WHERE id=?"));
-  if (!statement)
-    return false;
-
   statement.BindString16(0, info.title());
   statement.BindInt(1, info.visit_count());
   statement.BindInt(2, info.typed_count());
   statement.BindInt64(3, info.last_visit().ToInternalValue());
   statement.BindInt(4, info.hidden() ? 1 : 0);
   statement.BindInt64(5, url_id);
+
   return statement.Run();
 }
 
@@ -172,11 +164,6 @@ URLID URLDatabase::AddURLInternal(const history::URLRow& info,
 
   sql::Statement statement(GetDB().GetCachedStatement(
       sql::StatementID(statement_name), statement_sql));
-  if (!statement) {
-    NOTREACHED() << GetDB().GetErrorMessage();
-    return 0;
-  }
-
   statement.BindString(0, GURLToDatabaseURL(info.url()));
   statement.BindString16(1, info.title());
   statement.BindInt(2, info.visit_count());
@@ -195,10 +182,8 @@ URLID URLDatabase::AddURLInternal(const history::URLRow& info,
 bool URLDatabase::DeleteURLRow(URLID id) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM urls WHERE id = ?"));
-  if (!statement)
-    return false;
-
   statement.BindInt64(0, id);
+
   if (!statement.Run())
     return false;
 
@@ -208,9 +193,8 @@ bool URLDatabase::DeleteURLRow(URLID id) {
 
   sql::Statement del_keyword_visit(GetDB().GetCachedStatement(SQL_FROM_HERE,
                           "DELETE FROM keyword_search_terms WHERE url_id=?"));
-  if (!del_keyword_visit)
-    return false;
   del_keyword_visit.BindInt64(0, id);
+
   return del_keyword_visit.Run();
 }
 
@@ -251,10 +235,6 @@ bool URLDatabase::InitURLEnumeratorForEverything(URLEnumerator* enumerator) {
   sql.append(kURLRowFields);
   sql.append(" FROM urls");
   enumerator->statement_.Assign(GetDB().GetUniqueStatement(sql.c_str()));
-  if (!enumerator->statement_) {
-    NOTREACHED() << GetDB().GetErrorMessage();
-    return false;
-  }

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

I'm not entirely convinced that removing these is appropriate (well, the error
message is, but the return value maybe not).  But I need to move on for the day,
so I'll need to circle back to the rest of this file.

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

OK. Is the above build-up of a query manually kosher? Shouldn't we be using
BindInt there instead of manually  strappending? The macro is used thrice. Is
saving 40 bytes or whatever worth it?

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

In general I hate dynamically-generated queries, persistence is not the area to
save a few bytes.  But, by the same token, for a clean-up pass like you're
doing, I'm a little nervous about making substantive changes.  In this case, it
seems like there are a couple levels of dynamism going on, so nervous on top of
nervous.

   enumerator->initialized_ = true;
   return true;
 }
@@ -266,10 +246,6 @@ bool URLDatabase::InitURLEnumeratorForSignificant(URLEnumerator* enumerator) {
   sql.append(" FROM urls WHERE last_visit_time >= ? OR visit_count >= ? OR "
              "typed_count >= ?");
   enumerator->statement_.Assign(GetDB().GetUniqueStatement(sql.c_str()));
-  if (!enumerator->statement_) {
-    NOTREACHED() << GetDB().GetErrorMessage();
-    return false;
-  }
   enumerator->statement_.BindInt64(
       0, AutocompleteAgeThreshold().ToInternalValue());
   enumerator->statement_.BindInt(1, kLowQualityMatchVisitLimit);
@@ -283,10 +259,6 @@ bool URLDatabase::InitIconMappingEnumeratorForEverything(
   DCHECK(!enumerator->initialized_);
   enumerator->statement_.Assign(GetDB().GetUniqueStatement(
       "SELECT url, favicon_id FROM urls WHERE favicon_id <> 0"));
-  if (!enumerator->statement_) {
-    NOTREACHED() << GetDB().GetErrorMessage();
-    return false;
-  }
   enumerator->initialized_ = true;
   return true;
 }
@@ -316,8 +288,6 @@ bool URLDatabase::AutocompleteForPrefix(const std::string& prefix,
   }
   sql::Statement statement(
       GetDB().GetCachedStatement(sql::StatementID(__FILE__, line), sql));
-  if (!statement)
-    return false;
 
   // We will find all strings between "prefix" and this string, which is prefix
   // followed by the maximum character size. Use 8-bit strings for everything
@@ -375,11 +345,6 @@ bool URLDatabase::FindShortestURLFromBase(const std::string& base,
              "AND hidden = 0 AND visit_count >= ? AND typed_count >= ? "
              "ORDER BY url LIMIT 1");
   sql::Statement statement(GetDB().GetUniqueStatement(sql.c_str()));
-  if (!statement) {
-    NOTREACHED() << GetDB().GetErrorMessage();
-    return false;
-  }
-
   statement.BindString(0, base);
   statement.BindString(1, url);   // :end
   statement.BindInt(2, min_visits);
@@ -420,6 +385,7 @@ bool URLDatabase::CreateKeywordSearchTermsIndices() {
           "keyword_search_terms (url_id)")) {
     return false;
   }
+
   return true;
 }
 
@@ -436,19 +402,15 @@ bool URLDatabase::SetKeywordSearchTermsForURL(URLID url_id,
   sql::Statement exist_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT term FROM keyword_search_terms "
       "WHERE keyword_id = ? AND url_id = ?"));
-  if (!exist_statement)
-    return false;
   exist_statement.BindInt64(0, keyword_id);
   exist_statement.BindInt64(1, url_id);
+
   if (exist_statement.Step())
     return true;  // Term already exists, no need to add it.
 
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO keyword_search_terms (keyword_id, url_id, lower_term, term) "
       "VALUES (?,?,?,?)"));
-  if (!statement)
-    return false;
-
   statement.BindInt64(0, keyword_id);
   statement.BindInt64(1, url_id);
   statement.BindString16(2, base::i18n::ToLower(term));
@@ -461,10 +423,8 @@ bool URLDatabase::GetKeywordSearchTermRow(URLID url_id,
   DCHECK(url_id);
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT keyword_id, term FROM keyword_search_terms WHERE url_id=?"));
-  if (!statement)
-    return false;
-
   statement.BindInt64(0, url_id);
+
   if (!statement.Step())
     return false;
 
@@ -481,10 +441,8 @@ void URLDatabase::DeleteAllSearchTermsForKeyword(
   DCHECK(keyword_id);
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "DELETE FROM keyword_search_terms WHERE keyword_id=?"));
-  if (!statement)
-    return;
-
   statement.BindInt64(0, keyword_id);
+
   statement.Run();
 }
 
@@ -506,8 +464,6 @@ void URLDatabase::GetMostRecentKeywordSearchTerms(
       "JOIN urls u ON kv.url_id = u.id "
       "WHERE kv.keyword_id = ? AND kv.lower_term >= ? AND kv.lower_term < ? "
       "ORDER BY u.last_visit_time DESC LIMIT ?"));
-  if (!statement)
-    return;
 
   // NOTE: Keep this ToLower() call in sync with search_provider.cc.
   string16 lower_prefix = base::i18n::ToLower(prefix);