Database usage adjustment for .../history

chrome/browser/webdata/keyword_table.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/webdata/keyword_table.h"
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/stats_counters.h"
@@ skipping 106 matching lines @@
         "usage_count INTEGER DEFAULT 0,"
         "input_encodings VARCHAR,"
         "suggest_url VARCHAR,"
         "prepopulate_id INTEGER DEFAULT 0,"
         "autogenerate_keyword INTEGER DEFAULT 0,"
         "logo_id INTEGER DEFAULT 0,"
         "created_by_policy INTEGER DEFAULT 0,"
         "instant_url VARCHAR,"
         "last_modified INTEGER DEFAULT 0,"
         "sync_guid VARCHAR)")) {
-      NOTREACHED();
       return false;
     }
     if (!UpdateBackupSignature())
       return false;
   }
   return true;
 }
 
 bool KeywordTable::IsSyncable() {
   return true;
 }
 
 bool KeywordTable::AddKeyword(const TemplateURL& url) {
   DCHECK(url.id());
   // Be sure to change kUrlIdPosition if you add columns
   sql::Statement s(db_->GetUniqueStatement(
       "INSERT INTO keywords "
       "(short_name, keyword, favicon_url, url, safe_for_autoreplace, "
       "originating_url, date_created, usage_count, input_encodings, "
       "show_in_default_list, suggest_url, prepopulate_id, "
       "autogenerate_keyword, logo_id, created_by_policy, instant_url, "
       "last_modified, sync_guid, id) VALUES "
       "(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)"));
   BindURLToStatement(url, &s);
   s.BindInt64(kUrlIdPosition, url.id());
 
-  if (!s.Run()) {
+  if (!s.Run())
     return false;
-  }
+
   return UpdateBackupSignature();
 }
 
 bool KeywordTable::RemoveKeyword(TemplateURLID id) {
   DCHECK(id);
   sql::Statement s(
       db_->GetUniqueStatement("DELETE FROM keywords WHERE id = ?"));
   s.BindInt64(0, id);
 
   return s.Run() && UpdateBackupSignature();
@@ skipping 53 matching lines @@
     LOG(ERROR) << "No default search id backup found.";
     return NULL;
   }
   sql::Statement s(db_->GetUniqueStatement(
       "SELECT id, short_name, keyword, favicon_url, url, "
       "safe_for_autoreplace, originating_url, date_created, "
       "usage_count, input_encodings, show_in_default_list, "
       "suggest_url, prepopulate_id, autogenerate_keyword, logo_id, "
       "created_by_policy, instant_url, last_modified, sync_guid "
       "FROM keywords_backup WHERE id=?"));
-  if (!s) {
-    NOTREACHED() << "Statement prepare failed";
+  if (!s.is_valid()) {
     return NULL;
   }
   s.BindInt64(0, backup_id);
   if (!s.Step()) {
     LOG(ERROR) << "No default search provider with backup id.";

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

Drop the is_valid(), and use LOG_IF(s.Succeeded(), ERROR) to distinguish the
case there there was no error but the results were empty.

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

Done.

     return NULL;
   }
 
   scoped_ptr<TemplateURL> template_url(new TemplateURL());
   GetURLFromStatement(s, template_url.get());
   // ID has no meaning for the backup and should be 0 in case the TemplateURL
   // will be added to keywords if missing.
   template_url->set_id(0);
 
   if (!s.Succeeded()) {
     LOG(ERROR) << "Statement has not succeeded.";

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

Actually, I don't understand what this statement intends.  s.Step() being true
implies that s.Succeeded() should be true, doesn't it?

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

Yes, this is amiss. It is dead code. It could be there's a logic error here, as
this implies an incorrect reading of the Statement API.

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

     return NULL;
   }
   return template_url.release();
 }
 
 bool KeywordTable::DidDefaultSearchProviderChange() {
   if (!IsBackupSignatureValid()) {
     UMA_HISTOGRAM_ENUMERATION(
         protector::kProtectorHistogramDefaultSearchProvider,
         protector::kProtectorErrorBackupInvalid,
@@ skipping 135 matching lines @@
 }
 
 bool KeywordTable::MigrateToVersion41RewriteDefaultSearchProviderBackup() {
   // Due to crbug.com/101815 version 40 may contain corrupt or empty
   // signature. So ignore the signature and simply rewrite it.
   return MigrateToVersion40AddDefaultSearchProviderBackup();
 }
 
 bool KeywordTable::MigrateToVersion42AddFullDefaultSearchProviderBackup() {
   sql::Transaction transaction(db_);
-  if (!transaction.Begin()) {
-    NOTREACHED() << "Failed to start transaction";
+  if (!transaction.Begin())
     return false;
-  }
 
   int64 id = 0;
   if (!UpdateDefaultSearchProviderIDBackup(&id))
     return false;
 
   std::string keyword_backup;
   if (!UpdateDefaultSearchProviderBackup(id, &keyword_backup))
     return false;
 
   std::string keywords;
-  if (!GetTableContents("keywords", &keywords)) {
-    NOTREACHED() << "Can't get keywords table contents to sign";
+  if (!GetTableContents("keywords", &keywords))

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

Looks like failing this one would have caused many layers of log spew :-).

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

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

     return false;
-  }
 
   std::string data_to_sign = base::Int64ToString(id) +
                              keyword_backup +
                              keywords;
   std::string signature = protector::SignSetting(data_to_sign);
-  if (signature.empty()) {
-    NOTREACHED() << "Signature is empty";
+  if (signature.empty())

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

Is the NOTREACHED() in this case WRT sql/ API, or some other API?

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

Misfire. I should have kept NOTREACHED and done in the return false.

     return false;
-  }
-  if (!meta_table_->SetValue(kBackupSignatureKey, signature)) {
-    NOTREACHED() << "Failed to write signature.";
+  if (!meta_table_->SetValue(kBackupSignatureKey, signature))
     return false;
-  }
 
   return transaction.Commit();
 }
 
 bool KeywordTable::MigrateToVersion43AddKeywordsBackupTable() {
   return meta_table_->SetValue(kDefaultSearchBackupKey, std::string()) &&
          UpdateBackupSignature();
 }
 
 bool KeywordTable::GetSignatureData(std::string* backup) {
@@ skipping 17 matching lines @@
 bool KeywordTable::GetTableContents(const char* table_name,
                                     std::string* contents) {
   DCHECK(contents);
   std::string table_data;
 
   std::string query =
       "SELECT " + std::string(kKeywordColumnsConcatenated) +
       " FROM " + std::string(table_name) + " ORDER BY id ASC";
   sql::Statement s(db_->GetCachedStatement(sql::StatementID(table_name),
                                            query.c_str()));
-  if (!s) {
-    NOTREACHED() << "Statement prepare failed";
-    return false;
-  }
   while (s.Step())
     table_data += s.ColumnString(0);
-  if (!s.Succeeded()) {
-    NOTREACHED() << "Statement execution failed";
+  if (!s.Succeeded())
     return false;
-  }
+
   *contents = table_data;
   return true;
 }
 
 bool KeywordTable::UpdateBackupSignature() {
   sql::Transaction transaction(db_);
-  if (!transaction.Begin()) {
-    NOTREACHED() << "Failed to start transaction";
+  if (!transaction.Begin())
     return false;
-  }
 
   int64 id = 0;
   if (!UpdateDefaultSearchProviderIDBackup(&id)) {
     LOG(ERROR) << "Failed to update default search id backup.";
     return false;
   }
 
   // Backup of all keywords.
   if (db_->DoesTableExist("keywords_backup") &&
-      !db_->Execute("DROP TABLE keywords_backup"))
+      !db_->Execute("DROP TABLE keywords_backup")) {
     return false;
+  }
 
   if (!db_->Execute(
       "CREATE TABLE keywords_backup AS "
       "SELECT id, short_name, keyword, favicon_url, url, "
       "safe_for_autoreplace, originating_url, date_created, "
       "usage_count, input_encodings, show_in_default_list, "
       "suggest_url, prepopulate_id, autogenerate_keyword, logo_id, "
       "created_by_policy, instant_url, last_modified, sync_guid "
       "FROM keywords ORDER BY id ASC")) {
     LOG(ERROR) << "Failed to create keywords_backup table.";
     return false;
   }
 
   std::string data_to_sign;
   if (!GetSignatureData(&data_to_sign)) {
     LOG(ERROR) << "No data to sign.";
     return false;
   }
 
   std::string signature = protector::SignSetting(data_to_sign);
   if (signature.empty()) {
     LOG(ERROR) << "Signature is empty";
     return false;
   }
 
-  if (!meta_table_->SetValue(kBackupSignatureKey, signature)) {
-    NOTREACHED() << "Failed to write signature.";
+  if (!meta_table_->SetValue(kBackupSignatureKey, signature))
     return false;
-  }
 
   return transaction.Commit();
 }
 
 bool KeywordTable::IsBackupSignatureValid() {
   std::string signature;
   std::string signature_data;
   return meta_table_->GetValue(kBackupSignatureKey, &signature) &&
          GetSignatureData(&signature_data) &&
          protector::IsSettingValid(signature_data, signature);
@@ skipping 50 matching lines @@
   url->set_sync_guid(s.ColumnString(18));
 }
 
 bool KeywordTable::GetKeywordAsString(TemplateURLID id,
                                       const std::string& table_name,
                                       std::string* result) {
   std::string query =
       "SELECT " + std::string(kKeywordColumnsConcatenated) +
       " FROM " + table_name + " WHERE id=?";
   sql::Statement s(db_->GetUniqueStatement(query.c_str()));
-  if (!s) {
-    NOTREACHED() << "Statement prepare failed";
-    return false;
-  }
   s.BindInt64(0, id);
+
   if (!s.Step()) {
     LOG(WARNING) << "No keyword with id: " << id << ", ignoring.";
     return true;
   }
 
-  if (!s.Succeeded()) {
-    NOTREACHED() << "Statement failed.";
+  if (!s.Succeeded())
     return false;
-  }
 
   *result = s.ColumnString(0);
   return true;
 }
 
 bool KeywordTable::UpdateDefaultSearchProviderIDBackup(TemplateURLID* id) {
   DCHECK(id);
   int64 default_search_id = GetDefaultSearchProviderID();
   if (!meta_table_->SetValue(kDefaultSearchIDBackupKey,
                              default_search_id)) {
@@ skipping 14 matching lines @@
     return false;
   }
   if (!meta_table_->SetValue(kDefaultSearchBackupKey, backup_url)) {
     LOG(WARNING) << "Failed to update the keyword backup";
     return false;
   }
 
   *backup = backup_url;
   return true;
 }