Delete empty dom storage databases on destruction.

BUG=106763
TEST=


Review URL: http://codereview.chromium.org/9467003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@123960 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 82 insertions, 2 deletions

diff --git a/webkit/dom_storage/dom_storage_database.cc b/webkit/dom_storage/dom_storage_database.cc
index 673ddde..3882f0c 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) {
@@ -56,6 +62,7 @@ void DomStorageDatabase::ReadAllValues(ValuesMap* result) {
     statement.ColumnBlobAsString16(1, &value);
     (*result)[key] = NullableString16(value, false);
   }
+  known_to_be_empty_ = result->empty();
 }
 
 bool DomStorageDatabase::CommitChanges(bool clear_all_first,
@@ -63,6 +70,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 +78,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 +92,31 @@ 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) {
+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT count(key) from ItemTable"));
+    if (statement.Step())
+      known_to_be_empty_ = statement.ColumnInt(0) == 0;
+  }
+
+  bool success = transaction.Commit();
+  if (!success)
+    known_to_be_empty_ = old_known_to_be_empty;
+  return success;
 }
 
 bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
diff --git a/webkit/dom_storage/dom_storage_database.h b/webkit/dom_storage/dom_storage_database.h
index 2a636c2..b9115fd 100644
--- a/webkit/dom_storage/dom_storage_database.h
+++ b/webkit/dom_storage/dom_storage_database.h
@@ -107,6 +107,7 @@ class DomStorageDatabase {
   scoped_ptr<sql::Connection> db_;
   bool failed_to_open_;
   bool tried_to_recreate_;
+  bool known_to_be_empty_;
 };
 
 }  // namespace dom_storage
diff --git a/webkit/dom_storage/dom_storage_database_unittest.cc b/webkit/dom_storage/dom_storage_database_unittest.cc
index 3f2ec26..f3a3db0 100644
--- a/webkit/dom_storage/dom_storage_database_unittest.cc
+++ b/webkit/dom_storage/dom_storage_database_unittest.cc
@@ -113,6 +113,60 @@ TEST(DomStorageDatabaseTest, SimpleOpenAndClose) {
   EXPECT_FALSE(db.IsOpen());
 }
 
+TEST(DomStorageDatabaseTest, CloseEmptyDatabaseDeletesFile) {
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  FilePath file_name = temp_dir.path().AppendASCII("TestDomStorageDatabase.db");
+  ValuesMap storage;
+  CreateMapWithValues(&storage);
+
+  // First test the case that explicitly clearing the database will
+  // trigger it's deletion from disk.
+  {
+    DomStorageDatabase db(file_name);
+    ASSERT_TRUE(db.CommitChanges(false, storage));
+  }
+  EXPECT_TRUE(file_util::PathExists(file_name));
+
+  {
+    // Check that reading an existing db with data in it
+    // keeps the DB on disk on close.
+    DomStorageDatabase db(file_name);
+    ValuesMap values;
+    db.ReadAllValues(&values);
+    EXPECT_EQ(storage.size(), values.size());
+  }
+
+  EXPECT_TRUE(file_util::PathExists(file_name));
+  storage.clear();
+
+  {
+    DomStorageDatabase db(file_name);
+    ASSERT_TRUE(db.CommitChanges(true, storage));
+  }
+  EXPECT_FALSE(file_util::PathExists(file_name));
+
+  // Now ensure that a series of updates and removals whose net effect
+  // is an empty database also triggers deletion.
+  CreateMapWithValues(&storage);
+  {
+    DomStorageDatabase db(file_name);
+    ASSERT_TRUE(db.CommitChanges(false, storage));
+  }
+
+  EXPECT_TRUE(file_util::PathExists(file_name));
+
+  {
+    DomStorageDatabase db(file_name);
+    ASSERT_TRUE(db.CommitChanges(false, storage));
+    ValuesMap::iterator it = storage.begin();
+    for (; it != storage.end(); ++it)
+      it->second = NullableString16(true);
+    ASSERT_TRUE(db.CommitChanges(false, storage));
+  }
+  EXPECT_FALSE(file_util::PathExists(file_name));
+}
+
 TEST(DomStorageDatabaseTest, TestLazyOpenIsLazy) {
   // This test needs to operate with a file on disk to ensure that we will
   // open a file that already exists when only invoking ReadAllValues.