Persist sessionStorage on disk.

webkit/dom_storage/dom_storage_database.h

Index: webkit/dom_storage/dom_storage_database.h
diff --git a/webkit/dom_storage/dom_storage_database.h b/webkit/dom_storage/dom_storage_database.h
index 5d0d9ed8d4431b9ddde9c0a1d1cb6dc46c6cc9f3..d0dbddf5a1daff2747a98eeb958938428d82481d 100644
--- a/webkit/dom_storage/dom_storage_database.h
+++ b/webkit/dom_storage/dom_storage_database.h
@@ -13,39 +13,81 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/nullable_string16.h"
 #include "base/string16.h"
+#include "googleurl/src/gurl.h"
 #include "sql/connection.h"
 #include "webkit/dom_storage/dom_storage_types.h"
 
 namespace dom_storage {
 
-// Represents a SQLite based backing for DOM storage data. This
-// class is designed to be used on a single thread.
 class DomStorageDatabase {
  public:
+  DomStorageDatabase(const FilePath& file_path);
+  virtual ~DomStorageDatabase();
+
+  // Reads all the key, value pairs stored for |namespace_id| and |origin| in
+  // the database and returns them. |result| is assumed to be empty and any
+  // duplicate keys will be overwritten. If the database exists on disk then it
+  // will be opened. If it does not exist then it will not be created and
+  // |result| will be unmodified.
+  virtual void ReadAllValues(int64 namespace_id,

[michaeln, 2012/04/12 01:48:46]

I think this existing class for localstorage backed by a per-origin sqlitedb is
fine as it is. The namespace_id and origin parameters are meaningless to this
class. The problem is that this interface is not sufficient for session storage,
and not sufficient for localstorage that where multiple origins are stored in
the same db.

Instead of forcing the existing class to have the multiplexing interface. Why
dont we do something like this...

// Interface used by DomStorageArea
class DomStorageDatabaseAdapter {
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
};

// Impl that utilizes old school per-origin DomStorageDatabase
class LocalStorageDatabaseAdapter {
  LocalStorageDatabaseAdapter(DomStorageDatabase* db);
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
 private:
  scoped_ptr<DomStorageDatabase> db_;
};


// Impl that utilizes shiny new multiplexing SessionStorageDatabase
class SessionStorageDatabaseAdapter {
  SessionStorageDatabaseAdapter(SessionStorageDatabase* db,
                                int64 namespace_id,
                                GURL origin);
  virtual void ReadAllValues(ValuesMap* result) = 0;
  virtual bool CommitChanges(
      bool clear_all_first, const ValuesMap& changes) = 0;
 private:
  scoped_refptr<SessionStorageDatabase> db_;
};

Where the namespace creates the appropriate adapter. This might make unit tests
easier to compose (and faster to run) too since areas could be given a mock
adapter to work with.

There are odd difference around 'DeleteOrigin' that could be abstracted out into
the adapter too.

It'd be great if the next change to DomStorageDatabase.cc or .h was an svn
delete.

[marja, 2012/04/19 10:20:50]

Done. Though, the DomStorageArea still has 2 pointers to adapters, one generic
one which can be either session storage or local storage, and one which is
always session storage. This is because it needs to do different operations
(shallow copy, the shortcut for "deep copy + clear", etc.) for session storage.
Should I also put some parts of this logic into the adapter?

I also added DeleteFiles() and Reset() to the adapter, to be able to do
DeleteOrigin and PurgeMemory. Atm only LocalStorageDatabaseAdapter implements
them.

I also tried out having the Namespace create the Adapter, but it's somewhat
messy. DomStorageArea still needs the raw SessionStorageDatabase so that it can
call the special functions, so, it would make sense to have it create the
Adapter around it, too. Wdyt?

+                             const GURL& origin,
+                             ValuesMap* result) = 0;
+
+  // Updates the backing database. Will remove all keys before updating
+  // the database if |clear_all_first| is set. Then all entries in
+  // |changes| will be examined - keys mapped to a null NullableString16
+  // will be removed and all others will be inserted/updated as appropriate.
+  virtual bool CommitChanges(int64 namespace_id,
+                             const GURL& origin,
+                             bool clear_all_first,
+                             const ValuesMap& changes) = 0;
+
+  // Simple getter for the path we were constructed with.
+  const FilePath& file_path() const { return file_path_; }
+
+ protected:
+  // Open the database at file_path_ if it exists already and creates it if
+  // |create_if_needed| is true.
+  // Ensures we are at the correct database version and creates or updates
+  // tables as necessary. Returns false on failure.
+  virtual bool LazyOpen(bool create_if_needed) = 0;
+
+  virtual bool IsOpen() const = 0;
+
+  FilePath file_path_;
+  bool failed_to_open_;
+};
+
+// Represents a SQLite based backing for localStorage data. This
+// class is designed to be used on a single thread.
+class LocalStorageDatabase : public DomStorageDatabase {
+ public:
   static FilePath GetJournalFilePath(const FilePath& database_path);
 
-  explicit DomStorageDatabase(const FilePath& file_path);
-  virtual ~DomStorageDatabase();  // virtual for unit testing
+  explicit LocalStorageDatabase(const FilePath& file_path);
+  virtual ~LocalStorageDatabase();
 
   // Reads all the key, value pairs stored in the database and returns
   // them. |result| is assumed to be empty and any duplicate keys will
   // be overwritten. If the database exists on disk then it will be
   // opened. If it does not exist then it will not be created and
   // |result| will be unmodified.
-  void ReadAllValues(ValuesMap* result);
+  virtual void ReadAllValues(int64 namespace_id,
+                             const GURL& origin,
+                             ValuesMap* result) OVERRIDE;
 
   // Updates the backing database. Will remove all keys before updating
   // the database if |clear_all_first| is set. Then all entries in
   // |changes| will be examined - keys mapped to a null NullableString16
   // will be removed and all others will be inserted/updated as appropriate.
-  bool CommitChanges(bool clear_all_first, const ValuesMap& changes);
-
-  // Simple getter for the path we were constructed with.
-  const FilePath& file_path() const { return file_path_; }
+  virtual bool CommitChanges(int64 namespace_id,
+                             const GURL& origin,
+                             bool clear_all_first,
+                             const ValuesMap& changes) OVERRIDE;
 
  protected:
   // Constructor that uses an in-memory sqlite database, for testing.
-  DomStorageDatabase();
+  LocalStorageDatabase();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(DomStorageDatabaseTest, SimpleOpenAndClose);
@@ -71,12 +113,11 @@ class DomStorageDatabase {
     V1,
     V2
   };
-
   // Open the database at file_path_ if it exists already and creates it if
   // |create_if_needed| is true.
   // Ensures we are at the correct database version and creates or updates
   // tables as necessary. Returns false on failure.
-  bool LazyOpen(bool create_if_needed);
+  virtual bool LazyOpen(bool create_if_needed) OVERRIDE;
 
   // Analyses the database to verify that the connection that is open is indeed
   // a valid database and works out the schema version.
@@ -100,15 +141,12 @@ class DomStorageDatabase {
   bool UpgradeVersion1To2();
 
   void Close();
-  bool IsOpen() const { return db_.get() ? db_->is_open() : false; }
+  virtual bool IsOpen() const;
 
   // Initialization code shared between the two constructors of this class.
   void Init();
 
-  // Path to the database on disk.
-  const FilePath file_path_;
   scoped_ptr<sql::Connection> db_;
-  bool failed_to_open_;
   bool tried_to_recreate_;
   bool known_to_be_empty_;
 };