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..62d09d75d53464ba604ed739492034aac180dc41 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;
   }
@@ -259,15 +258,27 @@ bool SQLiteOriginBoundCertStore::Backend::EnsureDatabaseVersion() {
 
     sql::Statement smt(db_->GetUniqueStatement(
         "SELECT origin, cert FROM origin_bound_certs"));
+    if (!smt.is_valid()) {
+      LOG(WARNING) << "Unable to update origin bound cert database to "
+                   << "version 3.";

[wtc, 2012/02/10 18:30:53]

All the warning log messages should in this function should
say the same thing:
  LOG(WARNING) << "Unable to update origin bound cert database to "
               << "version 4.";

This also applies to the warning message on line 278 below.

[Greg Billock, 2012/02/10 23:54:54]

Done.

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

Hmm, just saw this WRT my review.  Note that _this_ code is in the if
(cur_version <= 3) case, whereas the above "version 2" change which I referenced
was in the (cur_version == 1) case.  The log line should refer to the version
the database will be in after the local code in the if() statement runs, not the
version the overall function will leave things.  So if a new version is added,
the old update code will be left alone.

+      return false;
+    }
+
     sql::Statement update_expires_smt(db_->GetUniqueStatement(
         "UPDATE origin_bound_certs SET expiration_time = ? WHERE origin = ?"));
-    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_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 (!update_creation_smt.is_valid()) {
+      LOG(WARNING) << "Unable to create origin bound cert database.";
+      return false;
+    }
+
     while (smt.Step()) {
       std::string origin = smt.ColumnString(0);
       std::string cert_from_db;
@@ -382,23 +393,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.