More SQL statement usage regularization. Back-touch some

chrome/browser/net/sqlite_origin_bound_cert_store.cc

Index: chrome/browser/net/sqlite_origin_bound_cert_store.cc
diff --git a/chrome/browser/net/sqlite_origin_bound_cert_store.cc b/chrome/browser/net/sqlite_origin_bound_cert_store.cc
index 9c6538e26704086bebf7afd2c9c386d67a7c1881..42d092d2f47b594224f6533ef54707df062c7825 100644
--- a/chrome/browser/net/sqlite_origin_bound_cert_store.cc
+++ b/chrome/browser/net/sqlite_origin_bound_cert_store.cc
@@ -176,8 +176,7 @@ bool SQLiteOriginBoundCertStore::Backend::Load(
   sql::Statement smt(db_->GetUniqueStatement(
       "SELECT origin, private_key, cert, cert_type, expiration_time, "
       "creation_time FROM origin_bound_certs"));
-  if (!smt) {
-    NOTREACHED() << "select statement prep failed";
+  if (!smt.is_valid()) {
     db_.reset();
     return false;
   }
@@ -220,13 +219,13 @@ bool SQLiteOriginBoundCertStore::Backend::EnsureDatabaseVersion() {
     if (!db_->Execute("ALTER TABLE origin_bound_certs ADD COLUMN cert_type "
                       "INTEGER")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
-                   << "version 2.";
+                   << "version 4.";

[Scott Hess - ex-Googler, 2012/02/13 22:04:04]

Nope, this stanza is upgrading to version 2, not version 4.

[Greg Billock, 2012/02/13 22:48:59]

I think you're right. I was reacting to wtc's comment that they should all be
the same, but I think that was premature.

       return false;
     }
     // All certs in version 1 database are rsa_sign, which has a value of 1.
     if (!db_->Execute("UPDATE origin_bound_certs SET cert_type = 1")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
-                   << "version 2.";
+                   << "version 4.";
       return false;
     }
     ++cur_version;
@@ -259,15 +258,28 @@ bool SQLiteOriginBoundCertStore::Backend::EnsureDatabaseVersion() {
 
     sql::Statement smt(db_->GetUniqueStatement(
         "SELECT origin, cert FROM origin_bound_certs"));
+    if (!smt.is_valid()) {

[Scott Hess - ex-Googler, 2012/02/13 22:04:04]

I think this would be reasonable to leave as !stmt.is_valid() ||
!other_stmt.is_valid(), etc.  No real gain to short-circuiting.

[Greg Billock, 2012/02/13 22:48:59]

Done.

+      LOG(WARNING) << "Unable to update origin bound cert database to "
+                   << "version 4.";
+      return false;
+    }
+
     sql::Statement update_expires_smt(db_->GetUniqueStatement(
         "UPDATE origin_bound_certs SET expiration_time = ? WHERE origin = ?"));
+    if (!update_expires_smt.is_valid()) {
+      LOG(WARNING) << "Unable to update origin bound cert database to "
+                   << "version 4.";
+      return false;
+    }
+
     sql::Statement update_creation_smt(db_->GetUniqueStatement(
         "UPDATE origin_bound_certs SET creation_time = ? WHERE origin = ?"));
-    if (!smt || !update_expires_smt || !update_creation_smt) {
+    if (!update_creation_smt.is_valid()) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 4.";
       return false;
     }
+
     while (smt.Step()) {
       std::string origin = smt.ColumnString(0);
       std::string cert_from_db;
@@ -382,23 +394,18 @@ void SQLiteOriginBoundCertStore::Backend::Commit() {
   sql::Statement add_smt(db_->GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO origin_bound_certs (origin, private_key, cert, cert_type, "
       "expiration_time) VALUES (?,?,?,?,?)"));
-  if (!add_smt) {
-    NOTREACHED();
+  if (!add_smt.is_valid())
     return;
-  }
 
   sql::Statement del_smt(db_->GetCachedStatement(SQL_FROM_HERE,
                              "DELETE FROM origin_bound_certs WHERE origin=?"));
-  if (!del_smt) {
-    NOTREACHED();
+  if (!del_smt.is_valid())
     return;
-  }
 
   sql::Transaction transaction(db_.get());
-  if (!transaction.Begin()) {
-    NOTREACHED();
+  if (!transaction.Begin())
     return;
-  }
+
   for (PendingOperationsList::iterator it = ops.begin();
        it != ops.end(); ++it) {
     // Free the certs as we commit them to the database.