Database usage adjustment for .../history

chrome/browser/history/url_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/url_database.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ skipping 72 matching lines @@
   i->hidden_ = s.ColumnInt(6) != 0;
 }
 
 bool URLDatabase::GetURLRow(URLID url_id, URLRow* info) {
   // TODO(brettw) We need check for empty URLs to handle the case where
   // there are old URLs in the database that are empty that got in before
   // we added any checks. We should eventually be able to remove it
   // 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);
 
-  statement.BindInt64(0, url_id);
   if (statement.Step()) {
     FillURLRow(statement, info);
     return true;
   }
   return false;
 }
 
 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;
     FillURLRow(statement, &info);
     urls->push_back(info);
   }
   return true;
 }
 
 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
 
   if (info)
     FillURLRow(statement, info);
   return statement.ColumnInt64(0);
 }
 
 bool URLDatabase::UpdateURLRow(URLID url_id,
                                const history::URLRow& info) {
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "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();
 }
 
 URLID URLDatabase::AddURLInternal(const history::URLRow& info,
                                   bool is_temporary) {
   // This function is used to insert into two different tables, so we have to
   // do some shuffling. Unfortinately, we can't use the macro
   // HISTORY_URL_ROW_FIELDS because that specifies the table name which is
   // invalid in the insert syntax.
   #define ADDURL_COMMON_SUFFIX \
       " (url, title, visit_count, typed_count, "\
       "last_visit_time, hidden) "\
       "VALUES (?,?,?,?,?,?)"
   const char* statement_name;
   const char* statement_sql;
   if (is_temporary) {
     statement_name = "AddURLTemporary";
     statement_sql = "INSERT INTO temp_urls" ADDURL_COMMON_SUFFIX;
   } else {
     statement_name = "AddURL";
     statement_sql = "INSERT INTO urls" ADDURL_COMMON_SUFFIX;
   }
   #undef ADDURL_COMMON_SUFFIX
 
   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());
   statement.BindInt(3, info.typed_count());
   statement.BindInt64(4, info.last_visit().ToInternalValue());
   statement.BindInt(5, info.hidden() ? 1 : 0);
 
   if (!statement.Run()) {
     VLOG(0) << "Failed to add url " << info.url().possibly_invalid_spec()
             << " to table history.urls.";
     return 0;
   }
   return GetDB().GetLastInsertRowId();
 }
 
 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);
 
-  statement.BindInt64(0, id);
   if (!statement.Run())
     return false;
 
   // And delete any keyword visits.
   if (!has_keyword_search_terms_)
     return true;
 
   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();
 }
 
 bool URLDatabase::CreateTemporaryURLTable() {
   return CreateURLTable(true);
 }
 
 bool URLDatabase::CommitTemporaryURLTable() {
   // See the comments in the header file as well as
   // HistoryBackend::DeleteAllHistory() for more information on how this works
@@ skipping 20 matching lines @@
 
   return true;
 }
 
 bool URLDatabase::InitURLEnumeratorForEverything(URLEnumerator* enumerator) {
   DCHECK(!enumerator->initialized_);
   std::string sql("SELECT ");
   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;
 }
 
 bool URLDatabase::InitURLEnumeratorForSignificant(URLEnumerator* enumerator) {
   DCHECK(!enumerator->initialized_);
   std::string sql("SELECT ");
   sql.append(kURLRowFields);
   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);
   enumerator->statement_.BindInt(2, kLowQualityMatchTypedLimit);
   enumerator->initialized_ = true;
   return true;
 }
 
 bool URLDatabase::InitIconMappingEnumeratorForEverything(
     IconMappingEnumerator* enumerator) {
   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;
 }
 
 bool URLDatabase::AutocompleteForPrefix(const std::string& prefix,
                                         size_t max_results,
                                         bool typed_only,
                                         std::vector<history::URLRow>* results) {
   // NOTE: this query originally sorted by starred as the second parameter. But
   // as bookmarks is no longer part of the db we no longer include the order
   // by clause.
   results->clear();
   const char* sql;
   int line;
   if (typed_only) {
     sql = "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls "
         "WHERE url >= ? AND url < ? AND hidden = 0 AND typed_count > 0 "
         "ORDER BY typed_count DESC, visit_count DESC, last_visit_time DESC "
         "LIMIT ?";
     line = __LINE__;
   } else {
     sql = "SELECT" HISTORY_URL_ROW_FIELDS "FROM urls "
         "WHERE url >= ? AND url < ? AND hidden = 0 "
         "ORDER BY typed_count DESC, visit_count DESC, last_visit_time DESC "
         "LIMIT ?";
     line = __LINE__;
   }
   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
   // so we can be sure sqlite is comparing everything in 8-bit mode. Otherwise,
   // it will have to convert strings either to UTF-8 or UTF-16 for comparison.
   std::string end_query(prefix);
   end_query.push_back(std::numeric_limits<unsigned char>::max());
 
   statement.BindString(0, prefix);
   statement.BindString(1, end_query);
@@ skipping 37 matching lines @@
   // those wouldn't use the index, and would run into problems with "wildcard"
   // characters that appear in URLs (% for LIKE, or *, ? for GLOB).
   std::string sql("SELECT ");
   sql.append(kURLRowFields);
   sql.append(" FROM urls WHERE url ");
   sql.append(allow_base ? ">=" : ">");
   sql.append(" ? AND url < :end AND url = substr(:end, 1, length(url)) "
              "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);
   statement.BindInt(3, min_typed);
 
   if (!statement.Step())
     return false;
 
   DCHECK(info);
   FillURLRow(statement, info);
@@ skipping 20 matching lines @@
           "keyword_search_terms (keyword_id, lower_term)")) {
     return false;
   }
 
   // For deletion.
   if (!GetDB().Execute(
           "CREATE INDEX IF NOT EXISTS keyword_search_terms_index2 ON "
           "keyword_search_terms (url_id)")) {
     return false;
   }
+
   return true;
 }
 
 bool URLDatabase::DropKeywordSearchTermsTable() {
   // This will implicitly delete the indices over the table.
   return GetDB().Execute("DROP TABLE keyword_search_terms");
 }
 
 bool URLDatabase::SetKeywordSearchTermsForURL(URLID url_id,
                                               TemplateURLID keyword_id,
                                               const string16& term) {
   DCHECK(url_id && keyword_id && !term.empty());
 
   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));
   statement.BindString16(3, term);
   return statement.Run();
 }
 
 bool URLDatabase::GetKeywordSearchTermRow(URLID url_id,
                                           KeywordSearchTermRow* row) {
   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);
 
-  statement.BindInt64(0, url_id);
   if (!statement.Step())
     return false;
 
   if (row) {
     row->url_id = url_id;
     row->keyword_id = statement.ColumnInt64(0);
     row->term = statement.ColumnString16(1);
   }
   return true;
 }
 
 void URLDatabase::DeleteAllSearchTermsForKeyword(
     TemplateURLID keyword_id) {
   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.BindInt64(0, keyword_id);
   statement.Run();
 }
 
 void URLDatabase::GetMostRecentKeywordSearchTerms(
     TemplateURLID keyword_id,
     const string16& prefix,
     int max_count,
     std::vector<KeywordSearchTermVisit>* matches) {
   // NOTE: the keyword_id can be zero if on first run the user does a query
   // before the TemplateURLService has finished loading. As the chances of this
   // occurring are small, we ignore it.
   if (!keyword_id)
     return;
 
   DCHECK(!prefix.empty());
   sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
       "SELECT DISTINCT kv.term, u.visit_count, u.last_visit_time "
       "FROM keyword_search_terms kv "
       "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);
   // This magic gives us a prefix search.
   string16 next_prefix = lower_prefix;
   next_prefix[next_prefix.size() - 1] =
       next_prefix[next_prefix.size() - 1] + 1;
   statement.BindInt64(0, keyword_id);
   statement.BindString16(1, lower_prefix);
   statement.BindString16(2, next_prefix);
@@ skipping 55 matching lines @@
   return GetDB().Execute(sql.c_str());
 }
 
 bool URLDatabase::CreateMainURLIndex() {
   // Index over URLs so we can quickly look up based on URL.
   return GetDB().Execute(
       "CREATE INDEX IF NOT EXISTS urls_url_index ON urls (url)");
 }
 
 }  // namespace history