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..10d2ad08e7a157843e58d5bfd48da3d76595bfca 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),
+      values_stored_(0) {
   // 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 (IsOpen() && values_stored_ == 0) {
+    // Delete the db from disk, it's empty.
+    Close();
+    file_util::Delete(file_path_, false);

[michaeln, 2012/02/25 02:55:55]

maybe skip this if file_path.empty()?

if (known_to_be_empty_ && !file_path.empty()) {
  Close();
  file_util::Delete(...);
}

[benm (inactive), 2012/02/27 12:06:25]

Done.

+  }
 }
 
 void DomStorageDatabase::ReadAllValues(ValuesMap* result) {
@@ -70,6 +76,7 @@ bool DomStorageDatabase::CommitChanges(bool clear_all_first,
   if (clear_all_first) {
     if (!db_->Execute("DELETE FROM ItemTable"))
       return false;
+    values_stored_ = 0;
   }
 
   ValuesMap::const_iterator it = changes.begin();
@@ -81,12 +88,14 @@ bool DomStorageDatabase::CommitChanges(bool clear_all_first,
       statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
          "DELETE FROM ItemTable WHERE key=?"));
       statement.BindString16(0, key);
+      values_stored_--;
     } 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));
+      values_stored_++;

[michaeln, 2012/02/25 02:55:55]

This count will not be accurate, consider this is just an update.

Keeping count is overkill. All we need is 'known_to_be_empty_' at dtor time, not
a count. I think we can do this in a lighter weight way given a bool data member
to that effect.

- init to false in the ctor
- dont modify LazyOpen in any way (value remains unmodified upon return)
  no extra queries on open
  (its ok to not determine on open because open only happens via the public
   ReadAllValues and CommitChanges methods which will determine the truth
   themselves)
- set to true in the if clear_all_first clause
- set to false here in the INSERT clause, and set a local to 'did_insert'
- set a local to 'did_delete' in the DELETE clause

after the for loop where we INSERT &| DELETE items, and before the COMMIT

if (!known_to_be_empty_ && did_delete && !did_insert) {
  knowns_to_be_empty_ = (select count == 0);
}

COMMIT

wdyt?

[benm (inactive), 2012/02/27 12:06:25]

Good catch, i made the test a bit more robust.
sg. I kept the small refactoring in LazyOpen, but removed the query.
Do we need the did_insert local? We can just set known_to_be_empty_ false on
insert (as if it's an add or replace we still won't have an empty db). I guess
it would optimise the case that we have a bunch of inserts/deletes in
changed_values_, so that we don't have to run the SELECT COUNT (as there was an
insert which we know will leave the db non-empty)

     }
     DCHECK(statement.is_valid());
     statement.Run();
@@ -149,18 +158,28 @@ 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) {
+      // This is the exceptional case - to try and recover we'll attempt
+      // to delete the file and start again.
+      Close();
+      return DeleteFileAndRecreate();
+    }
+
+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
+                             "SELECT count(key) from ItemTable"));
+    if (statement.Step())
+      values_stored_ = statement.ColumnInt(0);
   }
 
-  // This is the exceptional case - to try and recover we'll attempt
-  // to delete the file and start again.
-  Close();
-  return DeleteFileAndRecreate();
+  return true;
 }
 
 DomStorageDatabase::SchemaVersion DomStorageDatabase::DetectSchemaVersion() {