Create a class to represent a DOM Storage Database.

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
new file mode 100644
index 0000000000000000000000000000000000000000..90ad5695af216a4f3aa076e8fde3b15a8578f1ac
--- /dev/null
+++ b/webkit/dom_storage/dom_storage_database.cc
@@ -0,0 +1,192 @@
+// 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 "webkit/dom_storage/dom_storage_database.h"
+
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "sql/diagnostic_error_delegate.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace {
+
+class HistogramUniquifier {
+ public:
+  static const char* name() { return "Sqlite.DomStorageDatabase.Error"; }
+};
+
+sql::ErrorDelegate* GetErrorHandlerForDomStorageDatabase() {
+  return new sql::DiagnosticErrorDelegate<HistogramUniquifier>();
+}
+
+}  // anon namespace
+
+namespace dom_storage {
+
+DomStorageDatabase::DomStorageDatabase(const FilePath& file_path)
+    : file_path_(file_path),
+      db_(NULL) {
+  DCHECK(!file_path_.empty());
+}
+
+void DomStorageDatabase::ReadAllValues(
+    DomStorageDatabase::ValuesMap* result) {
+  if (!LazyOpen(false))
+    return;
+
+  sql::Statement stmt(db_->GetCachedStatement(SQL_FROM_HERE,
+                                              "SELECT * from ItemTable"));
+  DCHECK(stmt.is_valid());
+  if (!stmt)

[michaeln, 2012/02/01 21:03:11]

i'm looking at http://codereview.chromium.org/9249025/ which is about adjusting
sqlite usage patterns to be consistent, including removing checks for is_valid()
and early returns

in light of that i think these s/b removed

[benm (inactive), 2012/02/02 12:14:49]

Removed the early returns but kept DCHECK, as I think they're useful for
debugging.

+    return;
+
+  while (stmt.Step()) {
+    string16 key = stmt.ColumnString16(0);
+    string16 value;
+    stmt.ColumnBlobAsString16(1, &value);
+    (*result)[key] = NullableString16(value, false);
+  }
+}
+
+bool DomStorageDatabase::CommitChanges(bool clear_all_first,
+                                       const ValuesMap& changes) {
+  if (!LazyOpen(!changes.empty()))
+    return false;
+
+  sql::Transaction transaction(db_.get());
+  if (!transaction.Begin())
+    return false;
+
+  if (clear_all_first) {
+    if (!db_->Execute("DELETE FROM ItemTable"))
+      return false;
+  }
+
+  ValuesMap::const_iterator it = changes.begin();
+  sql::Statement stmt;

[michaeln, 2012/02/01 21:03:11]

can this be moved into the body of the for loop?
also style nit: statement instead of stmt

[benm (inactive), 2012/02/02 12:14:49]

Done, updated stmt everywhere

+  for(; it != changes.end(); ++it) {
+    string16 key = it->first;
+    NullableString16 value = it->second;
+    if (value.is_null()) {
+      stmt.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
+         "DELETE FROM ItemTable WHERE key=?"));
+      stmt.BindString16(0, key);
+    } else {
+      stmt.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
+        "INSERT INTO ItemTable VALUES (?,?)"));
+      stmt.BindString16(0, key);
+      string16 value_str = value.string();

[michaeln, 2012/02/01 21:03:11]

can the local be removed to avoid making a copy

[benm (inactive), 2012/02/02 12:14:49]

Done.

+      stmt.BindBlob(1, value_str.data(), value_str.length() * sizeof(char16));
+    }
+    DCHECK(stmt.is_valid());
+    stmt.Run();
+  }
+  return transaction.Commit();
+}
+
+bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
+  if (IsOpen())
+    return true;
+
+  bool database_exists = file_util::PathExists(file_path_);
+
+  if (!database_exists && !create_if_needed) {
+    // If the file doesn't exist already and we haven't been asked to create
+    // a file on disk, then we don't bother opening the database. This means
+    // we wait until we absolutely need to put something onto disk before we
+    // do so.
+    return false;
+  }
+
+  db_.reset(new sql::Connection());
+  db_->set_error_delegate(GetErrorHandlerForDomStorageDatabase());
+  if (!db_->Open(file_path_)) {
+    LOG(WARNING) << "Unable to open DOM storage database at "

[michaeln, 2012/02/01 21:03:11]

what should we do at this point?

[benm (inactive), 2012/02/02 12:14:49]

Good question :) My thinking was that it's ok to return false from this function
and let the caller deal with it.

I suppose we could create an in-memory database and try opening on disk again
when we come to commit changes. Wdyt, probably not for the initial checkin but I
could add a TODO?

[michaeln, 2012/02/03 04:45:15]

The caller is every method that may want to read or write. I think we have to
deal with this kind of error condition more centrally, not needed for this
initial checkin but prior to shipping.
That usage of in-memory dbs scares me :) What if the file on disk contains
values, and whaddaya-know it can be opened later? A page just ran seeing an
empty area and wrote a bunch of values with that emptiness in mind.

Hmmm... unittests could run much faster if they operated on in-memory dbs, it
might make sense to allow this class to treat empty file path as meaning use an
in-memory db and to massage the tests to take advantage of that where possible?

[benm (inactive), 2012/02/03 11:46:40]

Good point, added a TODO to handle this failure to open the file better. I
suppose what we should really do is delete the path on disk (if it exists) and
try again. If that still fails or the file wasn't there in the first place, then
we can set the flag and LocalStorage will effectively be unavailable. Unless of
course we did try and implement some sort of in-memory db and treat localstorage
as being effectively session storage... it wouldn't matter if the file on disk
had values already as we have nuked it anyway or it wasn't there to begin
with... might be a good fallback but perhaps not entirely in line with the spec
(i.e. local storage should only be expired by the user or for security reasons),
but the spec naturally doesn't specify what to do in this kind of exceptional
circumstance ... I guess if we were using an in memory db we could try one final
time to sync it to disk when the storage area is closed something like that...
That's a nice idea, I guess we'd still want a test or two writing the file to
disk to ensure that code path stays working but the rest could be in memory.
I'll add a todo in the test file.

+                 << file_path_.value();
+    return false;
+  }
+
+  // sql::Connection uses UTF-8 encoding, but WebCore style databases use
+  // UTF-16, so ensure we match.
+  ignore_result(db_->Execute("PRAGMA encoding=\"UTF-16\""));
+
+  if (!db_->DoesTableExist("ItemTable"))
+    return CreateTable();
+
+  // Table exists, so ensure we're at the right version, upgrading if
+  // necessary.
+  if (UpgradeVersion1To2IfNeeded())
+    return true;
+
+  // Upgrade failed, drop table and start fresh.
+  if (db_->Execute("DROP TABLE ItemTable"))
+    return CreateTable();
+
+  return false;

[michaeln, 2012/02/01 21:03:11]

upon return, will IsOpen() report 'true' since theres an open db connection...
but the schema will not be setup?

in these failed cases, maybe .reset() the db_ connection altogether?

[benm (inactive), 2012/02/02 12:14:49]

good call

+}
+
+bool DomStorageDatabase::CreateTable() {
+  // Current version is 2.
+  return CreateTableV2();

[michaeln, 2012/02/01 21:03:11]

doesn't look like we really need two separate methods here?

[benm (inactive), 2012/02/02 12:14:49]

My thinking was that it's useful to have a specific V2 function for the upgrade
process, and a generic function that will always create a table at the current
version. i.e. if there was a version 3, CreateTable would be updated to return
CreateTableV3().

[michaeln, 2012/02/03 04:45:15]

Still don't see what value the indirection adds. There's only code to create the
current version ever, so why two methods? My thinking is less is more.

[benm (inactive), 2012/02/03 11:46:40]

My thinking is that if we introduce a V3 database (which would probably be
something we could consider doing to improve performance, or what not) then we'd
write an UpgradeV2ToV3 function that would use CreateTableV3, and change
CreateTable so that it would call CreateTableV3. But we'd still want
CreateTableV2 so it can be used in UpgradeV1ToV2.

+}
+
+bool DomStorageDatabase::CreateTableV2() {
+  if (!LazyOpen(false))

[michaeln, 2012/02/01 21:03:11]

should this method DCHECK that the db_ connection is open instead?

[benm (inactive), 2012/02/02 12:14:49]

sgtm

+    return false;
+
+  return db_->Execute(
+      "CREATE TABLE IF NOT EXISTS ItemTable ("
+      "key TEXT UNIQUE ON CONFLICT REPLACE, "
+      "value BLOB NOT NULL ON CONFLICT FAIL)");
+}
+
+bool DomStorageDatabase::UpgradeVersion1To2IfNeeded() {
+  sql::Statement stmt(db_->GetCachedStatement(SQL_FROM_HERE,
+      "SELECT * FROM ItemTable"));
+  DCHECK(stmt.is_valid());
+  if (!stmt)
+    return false;
+
+  // Quick check to see if we need to upgrade or not. The single
+  // effect of V1 -> V2 is to change the value column from type
+  // TEXT to type BLOB.
+  sql::ColType valueColumnType = stmt.DeclaredColumnType(1);

[michaeln, 2012/02/01 21:03:11]

nit: nix camelCase variable naming

[benm (inactive), 2012/02/02 12:14:49]

Done.

+  if (valueColumnType == sql::COLUMN_TYPE_BLOB)
+    return true;
+
+  if (valueColumnType != sql::COLUMN_TYPE_TEXT) {
+    // Something is messed up. This is not a V1 database.

[michaeln, 2012/02/01 21:03:11]

what should we do at this point? delete it and start over?

[benm (inactive), 2012/02/02 12:14:49]

Yeah, that is taken care of by the LazyOpen function. If the upgrade method
returns false, it assumes the upgrade failed and drops the db and creates a
fresh table at the right version. Will add a note about return value in the
header

[michaeln, 2012/02/03 04:45:15]

LazyOpen continues to try to work with the existing file, but if its botched to
the point of being unopenable as a sqlite db, or that calling DROP TABLE
fails... we'll be wedged.

[benm (inactive), 2012/02/03 11:46:40]

Good point. However I don't think that we can get to this point if the database
was unopenable - we would have dropped out of LazyOpen before we got to the
point that we try and upgrade. I've added a TODO to handle failing to open the
file more gracefully in LazyOpen. I can also add a TODO to handle the failure of
the DROP in LazyOpen better.

+    return false;
+  }
+
+  // Need to migrate from TEXT value column to BLOB.
+  ValuesMap values;
+  while (stmt.Step()) {
+    string16 key = stmt.ColumnString16(0);
+    string16 value = stmt.ColumnString16(1);
+    values[key] = NullableString16(value, false);
+  }
+
+  sql::Transaction migration(db_.get());
+  if (migration.Begin()) {

[michaeln, 2012/02/01 21:03:11]

prefer early returns if (!begin()) return false

[benm (inactive), 2012/02/02 12:14:49]

Done.

+    if (db_->Execute("DROP TABLE ItemTable")) {
+      CreateTableV2();
+      if (CommitChanges(false, values)) {
+        return migration.Commit();
+      }
+    }
+  }
+  return false;
+}
+
+void DomStorageDatabase::Close() {
+  if (!IsOpen())
+    return;
+
+  db_->Close();
+  db_.reset(NULL);
+}
+
+}  // namespace dom_storage