More SQL statement usage regularization. Back-touch some

chrome/browser/net/sqlite_origin_bound_cert_store.cc

 // Copyright (c) 2012 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/net/sqlite_origin_bound_cert_store.h"
 
 #include <list>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 158 matching lines @@
     db_.reset();
     return false;
   }
 
   db_->Preload();
 
   // Slurp all the certs into the out-vector.
   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;
   }
 
   while (smt.Step()) {
     std::string private_key_from_db, cert_from_db;
     smt.ColumnBlobAsString(1, &private_key_from_db);
     smt.ColumnBlobAsString(2, &cert_from_db);
     scoped_ptr<net::DefaultOriginBoundCertStore::OriginBoundCert> cert(
         new net::DefaultOriginBoundCertStore::OriginBoundCert(
@@ skipping 61 matching lines @@
 
     if (!db_->Execute("ALTER TABLE origin_bound_certs ADD COLUMN "
                       "creation_time INTEGER")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 4.";
       return false;
     }
 
     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;
       smt.ColumnBlobAsString(1, &cert_from_db);
       // Parse the cert and extract the real value and then update the DB.
       scoped_refptr<net::X509Certificate> cert(
           net::X509Certificate::CreateFromBytes(
               cert_from_db.data(), cert_from_db.size()));
       if (cert) {
         if (cur_version == 2) {
@@ skipping 94 matching lines @@
     num_pending_ = 0;
   }
 
   // Maybe an old timer fired or we are already Close()'ed.
   if (!db_.get() || ops.empty())
     return;
 
   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.
     scoped_ptr<PendingOperation> po(*it);
     switch (po->op()) {
       case PendingOperation::CERT_ADD: {
         add_smt.Reset();
         add_smt.BindString(0, po->cert().origin());
         const std::string& private_key = po->cert().private_key();
         add_smt.BindBlob(1, private_key.data(), private_key.size());
@@ skipping 96 matching lines @@
   if (backend_.get())
     backend_->SetClearLocalStateOnExit(clear_local_state);
 }
 
 void SQLiteOriginBoundCertStore::Flush(const base::Closure& completion_task) {
   if (backend_.get())
     backend_->Flush(completion_task);
   else if (!completion_task.is_null())
     MessageLoop::current()->PostTask(FROM_HERE, completion_task);
 }