Hook up DomStorageArea with a DomStorageDatabase.

webkit/dom_storage/dom_storage_area.cc

Index: webkit/dom_storage/dom_storage_area.cc
diff --git a/webkit/dom_storage/dom_storage_area.cc b/webkit/dom_storage/dom_storage_area.cc
index e394f64db22b35b3eb2d320417194dd1b2df34c2..6d6b347e38c19acbaadd1362613c85602cedce52 100644
--- a/webkit/dom_storage/dom_storage_area.cc
+++ b/webkit/dom_storage/dom_storage_area.cc
@@ -3,57 +3,106 @@
 // found in the LICENSE file.
 
 #include "webkit/dom_storage/dom_storage_area.h"
+
+#include "base/bind.h"
+#include "base/time.h"
 #include "webkit/dom_storage/dom_storage_map.h"
 #include "webkit/dom_storage/dom_storage_namespace.h"
+#include "webkit/dom_storage/dom_storage_task_runner.h"
 #include "webkit/dom_storage/dom_storage_types.h"
+#include "webkit/fileapi/file_system_util.h"
 
 namespace dom_storage {
 
 DomStorageArea::DomStorageArea(
     int64 namespace_id, const GURL& origin,
     const FilePath& directory, DomStorageTaskRunner* task_runner)
-    : namespace_id_(namespace_id),
-      origin_(origin),
+    : namespace_id_(namespace_id), origin_(origin),
       directory_(directory),
       task_runner_(task_runner),
-      map_(new DomStorageMap(kPerAreaQuota)) {
+      map_(new DomStorageMap(kPerAreaQuota)),
+      backing_(NULL),
+      initial_import_done_(false),
+      clear_all_next_commit_(false),
+      commit_in_flight_(false) {
+
+  if (namespace_id == kLocalStorageNamespaceId && !directory.empty()) {
+    FilePath path = directory.Append(DatabaseFileNameFromOrigin(origin_));
+    backing_.reset(new DomStorageDatabase(path));
+  } else {
+    // Not a local storage area or no directory specified for backing
+    // database, (i.e. it's an incognito profile).
+    initial_import_done_ = true;
+  }
 }
 
 DomStorageArea::~DomStorageArea() {
+  if (clear_all_next_commit_ || !changed_values_.empty()) {
+    // Still some data left that was not committed to disk, try
+    // now.
+    CommitChanges();
+  }
+  // TODO(benm): What if the last commit failed and there's still data
+  // left not synced to disk?
+  DCHECK(!clear_all_next_commit_);
+  DCHECK(changed_values_.empty());

[michaeln, 2012/02/23 05:15:29]

Would it make sense to move these checks into the CommitChanges method? Not sure
we need them though really. Is the intent to catch a programming error or to
inform a developer that the db is hosed for some reason. If the latter, is that
something that should be DCHECKed?

Ultimately, UMA stats on how often backing->CommitChanges and ReadAllValues()
fail might be more useful than these dchecks?

[benm (inactive), 2012/02/23 12:27:40]

Yeah, I actually had the same though re. UMA tracking last night after looking
over this patch after uploading. I think it makes sense to do that. I'm not
entirely sure how much work it is to integrate with UMA though, so I'd rather
tackle it in a separate patch (unless it's really simple?). Will add a todo.

 }
 
 unsigned DomStorageArea::Length() {
+  InitialImportIfNeeded();
   return map_->Length();
 }
 
 NullableString16 DomStorageArea::Key(unsigned index) {
+  InitialImportIfNeeded();
   return map_->Key(index);
 }
 
 NullableString16 DomStorageArea::GetItem(const string16& key) {
+  InitialImportIfNeeded();
   return map_->GetItem(key);
 }
 
-bool DomStorageArea::SetItem(
-    const string16& key, const string16& value,
-    NullableString16* old_value) {
+bool DomStorageArea::SetItem(const string16& key,
+                             const string16& value,
+                             NullableString16* old_value) {
+  InitialImportIfNeeded();
+
   if (!map_->HasOneRef())
     map_ = map_->DeepCopy();
-  return map_->SetItem(key, value, old_value);
+  bool success = map_->SetItem(key, value, old_value);
+  if (success && backing_.get()) {
+    changed_values_[key] = NullableString16(value, false);
+    ScheduleCommitChanges();
+  }
+  return success;
 }
 
-bool DomStorageArea::RemoveItem(
-    const string16& key,
-    string16* old_value) {
+bool DomStorageArea::RemoveItem(const string16& key, string16* old_value) {
+  InitialImportIfNeeded();
   if (!map_->HasOneRef())
     map_ = map_->DeepCopy();
-  return map_->RemoveItem(key, old_value);
+  bool success = map_->RemoveItem(key, old_value);
+  if (success && backing_.get()) {
+    changed_values_[key] = NullableString16(true);
+    ScheduleCommitChanges();
+  }
+  return success;
 }
 
 bool DomStorageArea::Clear() {
+  InitialImportIfNeeded();
   if (map_->Length() == 0)
     return false;
+
   map_ = new DomStorageMap(kPerAreaQuota);
+
+  if (backing_.get()) {
+    changed_values_.clear();
+    clear_all_next_commit_ = true;
+    ScheduleCommitChanges();
+  }
+
   return true;
 }
 
@@ -61,10 +110,66 @@ DomStorageArea* DomStorageArea::ShallowCopy(int64 destination_namespace_id) {
   DCHECK_NE(kLocalStorageNamespaceId, namespace_id_);
   DCHECK_NE(kLocalStorageNamespaceId, destination_namespace_id);
   // SessionNamespaces aren't backed by files on disk.
+  DCHECK(!backing_.get());
+
   DomStorageArea* copy = new DomStorageArea(destination_namespace_id, origin_,
                                             FilePath(), task_runner_);
   copy->map_ = map_;
   return copy;
 }
 
+void DomStorageArea::InitialImportIfNeeded() {
+  if (initial_import_done_)
+    return;
+
+  DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
+  DCHECK(backing_.get());
+
+  ValuesMap initial_values;
+  backing_->ReadAllValues(&initial_values);
+  map_->SwapValues(&initial_values);
+  initial_import_done_ = true;
+}
+
+void DomStorageArea::ScheduleCommitChanges() {
+  DCHECK_EQ(kLocalStorageNamespaceId, namespace_id_);
+  DCHECK(backing_.get());
+  DCHECK(clear_all_next_commit_ || !changed_values_.empty());
+  DCHECK(task_runner_.get());
+
+  // There is no guarantee that an in flight commit will
+  // ever actually get processed. For example the task_runner_'s
+  // message loop could unexpectedly quit before the delayed task is
+  // fired and leave this flag set. But there's no way for us to determine
+  // that has happened, so in that case, we'll rely on the destructor
+  // to commit all the remaining changes to disk.

[michaeln, 2012/02/23 05:15:29]

The comment is more about the code in the dtor than the code here, wdyt about
moving it there.

[benm (inactive), 2012/02/23 12:27:40]

sgtm

+  if (commit_in_flight_)
+    return;
+
+  if (!task_runner_->PostDelayedTask(
+      FROM_HERE, base::Bind(&DomStorageArea::CommitChanges, this),
+      base::TimeDelta::FromSeconds(1))) {
+    // If we know that the task will not run, try and save changes synchronously
+    // so that we don't lose data.

[michaeln, 2012/02/23 05:15:29]

I think we should not handle this unexpected case in this way. Under normal
circumstances, this should not happen. The messageloops and workerpool we're
running on should be there.

Maybe DCHECK that the task is posted. If that check triggers, it would indicate
that something is wrong in the embedding program (a programming error that
failed to start a thread or shutdown a workerpool sooner than it should have
been).

commit_in_flight_ = PostDelayedTask(...);
DCHECK(commit_in_flight_);  

[benm (inactive), 2012/02/23 12:27:40]

ok, sg.

+    CommitChanges();
+  } else
+    commit_in_flight_ = true;
+}
+
+void DomStorageArea::CommitChanges() {
+  DCHECK(backing_.get());
+  if (backing_->CommitChanges(clear_all_next_commit_, changed_values_)) {
+    clear_all_next_commit_ = false;
+    changed_values_.clear();
+  }
+  commit_in_flight_ = false;
+}
+
+// static
+FilePath DomStorageArea::DatabaseFileNameFromOrigin(const GURL& origin) {
+  std::string filename = fileapi::GetOriginIdentifierFromURL(origin)
+      + ".localstorage";
+  return FilePath().AppendASCII(filename);
+}
+
 }  // namespace dom_storage