Delete empty dom storage databases on destruction.

webkit/dom_storage/dom_storage_database.cc

Index: webkit/dom_storage/dom_storage_database.cc
diff --git a/webkit/dom_storage/dom_storage_database.cc b/webkit/dom_storage/dom_storage_database.cc
index 673ddde5380433e9b6d246b2799765cbef82a548..dc5cf3bba8561357d1889fbd066ec9e8ae6432c5 100644
--- a/webkit/dom_storage/dom_storage_database.cc
+++ b/webkit/dom_storage/dom_storage_database.cc
@@ -30,7 +30,8 @@ DomStorageDatabase::DomStorageDatabase(const FilePath& file_path)
     : file_path_(file_path),
       db_(NULL),
       failed_to_open_(false),
-      tried_to_recreate_(false) {
+      tried_to_recreate_(false),
+      known_to_be_empty_(false) {
   // Note: in normal use we should never get an empty backing path here.
   // However, the unit test for this class defines another constructor
   // that will bypass this check to allow an empty path that signifies
@@ -40,6 +41,11 @@ DomStorageDatabase::DomStorageDatabase(const FilePath& file_path)
 }
 
 DomStorageDatabase::~DomStorageDatabase() {
+  if (known_to_be_empty_ && !file_path_.empty()) {
+    // Delete the db from disk, it's empty.
+    Close();
+    file_util::Delete(file_path_, false);
+  }
 }
 
 void DomStorageDatabase::ReadAllValues(ValuesMap* result) {
@@ -63,6 +69,7 @@ bool DomStorageDatabase::CommitChanges(bool clear_all_first,
   if (!LazyOpen(!changes.empty()))
     return false;
 
+  bool old_known_to_be_empty = known_to_be_empty_;
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return false;
@@ -70,8 +77,11 @@ bool DomStorageDatabase::CommitChanges(bool clear_all_first,
   if (clear_all_first) {
     if (!db_->Execute("DELETE FROM ItemTable"))
       return false;
+    known_to_be_empty_ = true;
   }
 
+  bool did_delete = false;
+  bool did_insert = false;
   ValuesMap::const_iterator it = changes.begin();
   for(; it != changes.end(); ++it) {
     sql::Statement statement;
@@ -81,17 +91,32 @@ bool DomStorageDatabase::CommitChanges(bool clear_all_first,
       statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
          "DELETE FROM ItemTable WHERE key=?"));
       statement.BindString16(0, key);
+      did_delete = true;
     } else {
       statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
           "INSERT INTO ItemTable VALUES (?,?)"));
       statement.BindString16(0, key);
       statement.BindBlob(1, value.string().data(),
                          value.string().length() * sizeof(char16));
+      known_to_be_empty_ = false;
+      did_insert = true;
     }
     DCHECK(statement.is_valid());
     statement.Run();
   }
-  return transaction.Commit();
+
+  if (!known_to_be_empty_ && did_delete && !did_insert) {
+    // See if the database is empty.

[michaeln, 2012/02/27 19:28:48]

consider removing the comment, the code is clear enough

[benm (inactive), 2012/02/27 19:36:15]

Done.

+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT count(key) from ItemTable"));
+    if (statement.Step())
+      known_to_be_empty_ = statement.ColumnInt(0) == 0;

[michaeln, 2012/02/27 19:28:48]

if you think it's more readable, consider...

known_to_be_empty_ = Step() &&
                    (ColumnInt(0) == 0);

[benm (inactive), 2012/02/27 19:36:15]

I think I prefer the if () then approach like it is now.

+  }
+
+  bool success = transaction.Commit();
+  if (!success)
+    known_to_be_empty_ = old_known_to_be_empty;
+  return success;
 }
 
 bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
@@ -149,18 +174,22 @@ bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
     // and whether it's usable (i.e. not corrupted).
     SchemaVersion current_version = DetectSchemaVersion();
 
+    bool valid_database = false;
     if (current_version == V2) {
-      return true;
+      valid_database = true;
     } else if (current_version == V1) {
       if (UpgradeVersion1To2())
-        return true;
+        valid_database = true;
+    }
+
+    if (!valid_database) {
+      // Try and recover by attempting to delete the file and start again.
+      Close();
+      return DeleteFileAndRecreate();
     }
   }
 
-  // This is the exceptional case - to try and recover we'll attempt
-  // to delete the file and start again.
-  Close();
-  return DeleteFileAndRecreate();
+  return true;

[michaeln, 2012/02/27 19:28:48]

looks like this introduces a bug in the 'new database' case (up above), if table
creation fails this method returns true.

in the spirit of extreme programming principles, i don't see what the changes in
this method have to do with deleting empties on exit and suggest undoing them

(having said that... imo, an improvement would be to make the 'new database'
case return early always instead of only sometimes, because that would make the
type of error being introduced here less likely to be introduced, i think i
tried to make that point in an earlier code review:)

[benm (inactive), 2012/02/27 19:36:15]

Good spot.
Probably for the best. :)
Will do it in another CL :)

 }
 
 DomStorageDatabase::SchemaVersion DomStorageDatabase::DetectSchemaVersion() {