Add callback to LevelDBWrapperImpl to allow extra operations on commit.

Initially this callback is used to delay writing the schema version until data
actually gets committed. In follow-up CLs this will also be used to store
per-origin metadata.

BUG=586194

Review-Url: https://codereview.chromium.org/2597353002
Cr-Commit-Position: refs/heads/master@{#440861}

content/browser/dom_storage/local_storage_context_mojo.cc

@@ -31,8 +31,6 @@ const char kOriginSeparator = '\x00';
 const char kDataPrefix[] = "_";
 const int64_t kMinSchemaVersion = 1;
 const int64_t kCurrentSchemaVersion = 1;
-
-void NoOpResult(leveldb::mojom::DatabaseError status) {}
 }
 
 LocalStorageContextMojo::LocalStorageContextMojo(
@@ -79,9 +77,9 @@ void LocalStorageContextMojo::OpenLocalStorage(
 
   if (connection_state_ == CONNECTION_IN_PROGRESS) {
     // Queue this OpenLocalStorage call for when we have a level db pointer.
-    on_database_opened_callbacks_.push_back(base::Bind(
+    on_database_opened_callbacks_.push_back(base::BindOnce(
         &LocalStorageContextMojo::BindLocalStorage,
-        weak_ptr_factory_.GetWeakPtr(), origin, base::Passed(&request)));
+        weak_ptr_factory_.GetWeakPtr(), origin, std::move(request)));
     return;
   }
 
@@ -102,6 +100,24 @@ void LocalStorageContextMojo::OnLevelDBWrapperHasNoBindings(
   level_db_wrappers_.erase(origin);
 }
 
+std::vector<leveldb::mojom::BatchedOperationPtr>
+LocalStorageContextMojo::OnLevelDBWrapperPrepareToCommit() {
+  std::vector<leveldb::mojom::BatchedOperationPtr> operations;
+
+  // Write schema version if not already done so before.
+  if (!database_initialized_) {
+    leveldb::mojom::BatchedOperationPtr item =
+        leveldb::mojom::BatchedOperation::New();
+    item->key = leveldb::StdStringToUint8Vector(kVersionKey);
+    item->value = leveldb::StdStringToUint8Vector(
+        base::Int64ToString(kCurrentSchemaVersion));
+    operations.push_back(std::move(item));
+    database_initialized_ = true;
+  }
+
+  return operations;
+}
+
 void LocalStorageContextMojo::OnUserServiceConnectionComplete() {
   CHECK_EQ(service_manager::mojom::ConnectResult::SUCCEEDED,
            file_service_connection_->GetResult());
@@ -161,13 +177,8 @@ void LocalStorageContextMojo::OnGotDatabaseVersion(
     leveldb::mojom::DatabaseError status,
     const std::vector<uint8_t>& value) {
   if (status == leveldb::mojom::DatabaseError::NOT_FOUND) {
-    // New database, write current version and continue.
-    // TODO(mek): Delay writing version until first actual data gets committed.
-    database_->Put(leveldb::StdStringToUint8Vector(kVersionKey),
-                   leveldb::StdStringToUint8Vector(
-                       base::Int64ToString(kCurrentSchemaVersion)),
-                   base::Bind(&NoOpResult));
-    // new database
+    // New database, nothing more to do. Current version will get written
+    // when first data is committed.
   } else if (status == leveldb::mojom::DatabaseError::OK) {
     // Existing database, check if version number matches current schema
     // version.
@@ -178,6 +189,8 @@ void LocalStorageContextMojo::OnGotDatabaseVersion(
       // TODO(mek): delete and recreate database, rather than failing outright.
       database_ = nullptr;
     }
+
+    database_initialized_ = true;
   } else {
     // Other read error. Possibly database corruption.
     // TODO(mek): delete and recreate database, rather than failing outright.
@@ -188,7 +201,7 @@ void LocalStorageContextMojo::OnGotDatabaseVersion(
   // delayed bindings.
   connection_state_ = CONNECTION_FINISHED;
   for (size_t i = 0; i < on_database_opened_callbacks_.size(); ++i)
-    on_database_opened_callbacks_[i].Run();
+    std::move(on_database_opened_callbacks_[i]).Run();
   on_database_opened_callbacks_.clear();
 }
 
@@ -214,7 +227,9 @@ void LocalStorageContextMojo::BindLocalStorage(
         base::TimeDelta::FromSeconds(kCommitDefaultDelaySecs), kMaxBytesPerHour,
         kMaxCommitsPerHour,
         base::Bind(&LocalStorageContextMojo::OnLevelDBWrapperHasNoBindings,
-                   base::Unretained(this), origin));
+                   base::Unretained(this), origin),
+        base::Bind(&LocalStorageContextMojo::OnLevelDBWrapperPrepareToCommit,
+                   base::Unretained(this)));
     found = level_db_wrappers_.find(origin);
   }
 

content/browser/dom_storage/local_storage_context_mojo.h

@@ -37,6 +37,8 @@ class CONTENT_EXPORT LocalStorageContextMojo {
 
  private:
   void OnLevelDBWrapperHasNoBindings(const url::Origin& origin);
+  std::vector<leveldb::mojom::BatchedOperationPtr>
+  OnLevelDBWrapperPrepareToCommit();
   void OnUserServiceConnectionComplete();
   void OnUserServiceConnectionError();
 
@@ -59,6 +61,7 @@ class CONTENT_EXPORT LocalStorageContextMojo {
     CONNECTION_IN_PROGRESS,
     CONNECTION_FINISHED
   } connection_state_ = NO_CONNECTION;
+  bool database_initialized_ = false;
 
   std::unique_ptr<service_manager::Connection> file_service_connection_;
 
@@ -68,7 +71,7 @@ class CONTENT_EXPORT LocalStorageContextMojo {
   leveldb::mojom::LevelDBServicePtr leveldb_service_;
   leveldb::mojom::LevelDBDatabasePtr database_;
 
-  std::vector<base::Closure> on_database_opened_callbacks_;
+  std::vector<base::OnceClosure> on_database_opened_callbacks_;
 
   // Maps between an origin and its prefixed LevelDB view.
   std::map<url::Origin, std::unique_ptr<LevelDBWrapperImpl>> level_db_wrappers_;

content/browser/dom_storage/local_storage_context_mojo_unittest.cc

@@ -72,6 +72,7 @@ TEST_F(LocalStorageContextMojoTest, Basic) {
   wrapper.reset();
 
   base::RunLoop().RunUntilIdle();
+
   ASSERT_EQ(2u, mock_data().size());
   EXPECT_EQ(value, mock_data().rbegin()->second);
 }
@@ -134,4 +135,23 @@ TEST_F(LocalStorageContextMojoTest, InvalidVersion) {
   EXPECT_FALSE(success);
 }
 
+TEST_F(LocalStorageContextMojoTest, VersionOnlyWrittenOnCommit) {
+  mojom::LevelDBWrapperPtr wrapper;
+  context()->OpenLocalStorage(url::Origin(GURL("http://foobar.com")),
+                              MakeRequest(&wrapper));
+
+  base::RunLoop run_loop;
+  bool success = false;
+  std::vector<uint8_t> result;
+  wrapper->Get(
+      StdStringToUint8Vector("key"),
+      base::Bind(&GetCallback, run_loop.QuitClosure(), &success, &result));
+  run_loop.Run();
+  EXPECT_FALSE(success);
+  wrapper.reset();
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(mock_data().empty());
+}
+
 }  // namespace content

content/browser/leveldb_wrapper_impl.cc

@@ -41,9 +41,11 @@ LevelDBWrapperImpl::LevelDBWrapperImpl(
     base::TimeDelta default_commit_delay,
     int max_bytes_per_hour,
     int max_commits_per_hour,
-    const base::Closure& no_bindings_callback)
+    const base::Closure& no_bindings_callback,
+    const PrepareToCommitCallback& prepare_to_commit_callback)
     : prefix_(leveldb::StdStringToUint8Vector(prefix)),
       no_bindings_callback_(no_bindings_callback),
+      prepare_to_commit_callback_(prepare_to_commit_callback),
       database_(database),
       bytes_used_(0),
       max_size_(max_size),
@@ -334,7 +336,8 @@ void LevelDBWrapperImpl::CommitChanges() {
   commit_rate_limiter_.add_samples(1);
 
   // Commit all our changes in a single batch.
-  std::vector<leveldb::mojom::BatchedOperationPtr> operations;
+  std::vector<leveldb::mojom::BatchedOperationPtr> operations =
+      prepare_to_commit_callback_.Run();
   if (commit_batch_->clear_all_first) {
     leveldb::mojom::BatchedOperationPtr item =
         leveldb::mojom::BatchedOperation::New();

content/browser/leveldb_wrapper_impl.h

@@ -32,6 +32,9 @@ namespace content {
 // 4) Throttles requests to avoid overwhelming the disk.
 class CONTENT_EXPORT LevelDBWrapperImpl : public mojom::LevelDBWrapper {
  public:
+  using PrepareToCommitCallback =
+      base::Callback<std::vector<leveldb::mojom::BatchedOperationPtr>()>;
+
   // |no_bindings_callback| will be called when this object has no more
   // bindings and all pending modifications have been processed.
   LevelDBWrapperImpl(leveldb::mojom::LevelDBDatabase* database,
@@ -40,7 +43,8 @@ class CONTENT_EXPORT LevelDBWrapperImpl : public mojom::LevelDBWrapper {
                      base::TimeDelta default_commit_delay,
                      int max_bytes_per_hour,
                      int max_commits_per_hour,
-                     const base::Closure& no_bindings_callback);
+                     const base::Closure& no_bindings_callback,
+                     const PrepareToCommitCallback& prepare_to_commit_callback);
   ~LevelDBWrapperImpl() override;
 
   void Bind(mojom::LevelDBWrapperRequest request);
@@ -118,6 +122,7 @@ class CONTENT_EXPORT LevelDBWrapperImpl : public mojom::LevelDBWrapper {
   mojo::BindingSet<mojom::LevelDBWrapper> bindings_;
   mojo::AssociatedInterfacePtrSet<mojom::LevelDBObserver> observers_;
   base::Closure no_bindings_callback_;
+  PrepareToCommitCallback prepare_to_commit_callback_;
   leveldb::mojom::LevelDBDatabase* database_;
   std::unique_ptr<ValueMap> map_;
   std::vector<base::Closure> on_load_complete_tasks_;

content/browser/leveldb_wrapper_impl_unittest.cc

@@ -53,6 +53,9 @@ class GetAllCallback : public mojom::LevelDBWrapperGetAllCallback {
 };
 
 void NoOp() {}
+std::vector<leveldb::mojom::BatchedOperationPtr> PrepareToCommitNoOp() {
+  return std::vector<leveldb::mojom::BatchedOperationPtr>();
+}
 
 void GetCallback(const base::Closure& callback,
                  bool* success_out,
@@ -92,7 +95,8 @@ class LevelDBWrapperImplTest : public testing::Test,
                           base::TimeDelta::FromSeconds(5),
                           10 * 1024 * 1024 /* max_bytes_per_hour */,
                           60 /* max_commits_per_hour */,
-                          base::Bind(&NoOp)),
+                          base::Bind(&NoOp),
+                          base::Bind(&PrepareToCommitNoOp)),
         observer_binding_(this) {
     set_mock_data(std::string(kTestPrefix) + "def", "defdata");
     set_mock_data(std::string(kTestPrefix) + "123", "123data");