Remove more mojom dependencies from DOM Storage

Eliminates use of the BatchedOperation mojom struct from
LocalStorageContextMojo.

Bug: 1000959
Change-Id: I09b849a3a53379735535dc7d00db19d2c745bfe2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1863253
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Chris Mumford <cmumford@google.com>
Cr-Commit-Position: refs/heads/master@{#706633}

content/browser/dom_storage/local_storage_context_mojo.h

@@ -156,7 +156,7 @@ class CONTENT_EXPORT LocalStorageContextMojo
                      std::vector<leveldb::mojom::KeyValuePtr> data);
 
   void OnGotStorageUsageForShutdown(std::vector<StorageUsageInfo> usage);
-  void OnShutdownComplete(leveldb::mojom::DatabaseError error);
+  void OnShutdownComplete(leveldb::Status status);
 
   void GetStatistics(size_t* total_cache_size, size_t* unused_area_count);
   void OnCommitResult(leveldb::mojom::DatabaseError error);

content/browser/dom_storage/session_storage_data_map.cc

@@ -7,6 +7,7 @@
 #include "base/system/sys_info.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/services/leveldb/public/cpp/util.h"
 #include "content/browser/dom_storage/dom_storage_types.h"
 
 namespace content {
@@ -38,13 +39,8 @@ scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::CreateClone(
       listener, std::move(map_data), std::move(clone_from)));
 }
 
-std::vector<leveldb::mojom::BatchedOperationPtr>
-SessionStorageDataMap::PrepareToCommit() {
-  return std::vector<leveldb::mojom::BatchedOperationPtr>();
-}
-
-void SessionStorageDataMap::DidCommit(leveldb::mojom::DatabaseError error) {
-  listener_->OnCommitResult(error);
+void SessionStorageDataMap::DidCommit(leveldb::Status status) {
+  listener_->OnCommitResult(leveldb::LeveldbStatusToError(status));
 }
 
 SessionStorageDataMap::SessionStorageDataMap(

content/browser/dom_storage/session_storage_data_map.h

@@ -77,9 +77,7 @@ class CONTENT_EXPORT SessionStorageDataMap final
   // Note: this is irrelevant, as the parent area is handling binding.
   void OnNoBindings() override {}
 
-  std::vector<leveldb::mojom::BatchedOperationPtr> PrepareToCommit() override;
-
-  void DidCommit(leveldb::mojom::DatabaseError error) override;
+  void DidCommit(leveldb::Status status) override;
 
  private:
   friend class base::RefCounted<SessionStorageDataMap>;

content/browser/dom_storage/storage_area_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/containers/span.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
@@ -14,16 +15,21 @@
 #include "components/services/leveldb/public/cpp/util.h"
 #include "content/public/browser/browser_thread.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
+#include "third_party/leveldatabase/env_chromium.h"
 
 namespace content {
+
 namespace {
-using leveldb::mojom::BatchedOperation;
-using leveldb::mojom::BatchedOperationPtr;
 using leveldb::mojom::DatabaseError;
 }  // namespace
 
 StorageAreaImpl::Delegate::~Delegate() {}
 
+void StorageAreaImpl::Delegate::PrepareToCommit(
+    std::vector<storage::DomStorageDatabase::KeyValuePair>*
+        extra_entries_to_add,
+    std::vector<storage::DomStorageDatabase::Key>* extra_keys_to_delete) {}
+
 void StorageAreaImpl::Delegate::MigrateData(
     base::OnceCallback<void(std::unique_ptr<ValueMap>)> callback) {
   std::move(callback).Run(nullptr);
@@ -756,56 +762,60 @@ void StorageAreaImpl::CommitChanges() {
   commit_rate_limiter_.add_samples(1);
 
   // Commit all our changes in a single batch.
-  std::vector<BatchedOperationPtr> operations = delegate_->PrepareToCommit();
-  bool has_changes = !operations.empty() ||
-                     !commit_batch_->changed_values.empty() ||
-                     !commit_batch_->changed_keys.empty();
-  if (commit_batch_->clear_all_first) {
-    BatchedOperationPtr item = BatchedOperation::New();
-    item->type = leveldb::mojom::BatchOperationType::DELETE_PREFIXED_KEY;
-    item->key = prefix_;
-    operations.push_back(std::move(item));
-  }
+  struct Commit {
+    storage::DomStorageDatabase::Key prefix;
+    bool clear_all_first;
+    std::vector<storage::DomStorageDatabase::KeyValuePair> entries_to_add;
+    std::vector<storage::DomStorageDatabase::Key> keys_to_delete;
+    base::Optional<storage::DomStorageDatabase::Key> copy_to_prefix;
+  };
+
+  Commit commit;
+  commit.prefix = prefix_;
+  commit.clear_all_first = commit_batch_->clear_all_first;
+  delegate_->PrepareToCommit(&commit.entries_to_add, &commit.keys_to_delete);
+
+  const bool has_changes = !commit.entries_to_add.empty() ||
+                           !commit.keys_to_delete.empty() ||
+                           !commit_batch_->changed_values.empty() ||
+                           !commit_batch_->changed_keys.empty();
   size_t data_size = 0;
   if (map_state_ == MapState::LOADED_KEYS_AND_VALUES) {
     DCHECK(commit_batch_->changed_values.empty())
         << "Map state and commit state out of sync.";
     for (const auto& key : commit_batch_->changed_keys) {
       data_size += key.size();
-      BatchedOperationPtr item = BatchedOperation::New();
-      item->key.reserve(prefix_.size() + key.size());
-      item->key.insert(item->key.end(), prefix_.begin(), prefix_.end());
-      item->key.insert(item->key.end(), key.begin(), key.end());
-      auto kv_it = keys_values_map_.find(key);
-      if (kv_it != keys_values_map_.end()) {
-        item->type = leveldb::mojom::BatchOperationType::PUT_KEY;
-        data_size += kv_it->second.size();
-        item->value = kv_it->second;
+      storage::DomStorageDatabase::Key prefixed_key;
+      prefixed_key.reserve(prefix_.size() + key.size());
+      prefixed_key.insert(prefixed_key.end(), prefix_.begin(), prefix_.end());
+      prefixed_key.insert(prefixed_key.end(), key.begin(), key.end());
+      auto it = keys_values_map_.find(key);
+      if (it != keys_values_map_.end()) {
+        data_size += it->second.size();
+        commit.entries_to_add.emplace_back(std::move(prefixed_key), it->second);
       } else {
-        item->type = leveldb::mojom::BatchOperationType::DELETE_KEY;
+        commit.keys_to_delete.push_back(std::move(prefixed_key));
       }
-      operations.push_back(std::move(item));
     }
   } else {
     DCHECK(commit_batch_->changed_keys.empty())
         << "Map state and commit state out of sync.";
     DCHECK_EQ(map_state_, MapState::LOADED_KEYS_ONLY);
-    for (auto& it : commit_batch_->changed_values) {
-      const auto& key = it.first;
+    for (auto& entry : commit_batch_->changed_values) {
+      const auto& key = entry.first;
       data_size += key.size();
-      BatchedOperationPtr item = BatchedOperation::New();
-      item->key.reserve(prefix_.size() + key.size());
-      item->key.insert(item->key.end(), prefix_.begin(), prefix_.end());
-      item->key.insert(item->key.end(), key.begin(), key.end());
-      auto kv_it = keys_only_map_.find(key);
-      if (kv_it != keys_only_map_.end()) {
-        item->type = leveldb::mojom::BatchOperationType::PUT_KEY;
-        data_size += it.second.size();
-        item->value = std::move(it.second);
+      storage::DomStorageDatabase::Key prefixed_key;
+      prefixed_key.reserve(prefix_.size() + key.size());
+      prefixed_key.insert(prefixed_key.end(), prefix_.begin(), prefix_.end());
+      prefixed_key.insert(prefixed_key.end(), key.begin(), key.end());
+      auto it = keys_only_map_.find(key);
+      if (it != keys_only_map_.end()) {
+        data_size += entry.second.size();
+        commit.entries_to_add.emplace_back(std::move(prefixed_key),
+                                           std::move(entry.second));
       } else {
-        item->type = leveldb::mojom::BatchOperationType::DELETE_KEY;
+        commit.keys_to_delete.push_back(std::move(prefixed_key));
       }
-      operations.push_back(std::move(item));
     }
   }
   // Schedule the copy, and ignore if |clear_all_first| is specified and there
@@ -813,11 +823,7 @@ void StorageAreaImpl::CommitChanges() {
   if (commit_batch_->copy_to_prefix) {
     DCHECK(!has_changes);
     DCHECK(!commit_batch_->clear_all_first);
-    BatchedOperationPtr item = BatchedOperation::New();
-    item->type = leveldb::mojom::BatchOperationType::COPY_PREFIXED_KEY;
-    item->key = prefix_;
-    item->value = std::move(commit_batch_->copy_to_prefix.value());
-    operations.push_back(std::move(item));
+    commit.copy_to_prefix = std::move(commit_batch_->copy_to_prefix);
   }
   commit_batch_.reset();
 
@@ -825,26 +831,41 @@ void StorageAreaImpl::CommitChanges() {
 
   ++commit_batches_in_flight_;
 
-  // TODO(michaeln): Currently there is no guarantee LevelDBDatabaseImpl::Write
-  // will run during a clean shutdown. We need that to avoid dataloss.
-  database_->Write(std::move(operations),
-                   base::BindOnce(&StorageAreaImpl::OnCommitComplete,
-                                  weak_ptr_factory_.GetWeakPtr()));
-}
-
-void StorageAreaImpl::OnCommitComplete(DatabaseError error) {
+  database_->RunDatabaseTask(
+      base::BindOnce(
+          [](Commit commit, const storage::DomStorageDatabase& db) {
+            leveldb::WriteBatch batch;
+            if (commit.clear_all_first)
+              db.DeletePrefixed(commit.prefix, &batch);
+            for (const auto& entry : commit.entries_to_add) {
+              batch.Put(leveldb_env::MakeSlice(entry.key),
+                        leveldb_env::MakeSlice(entry.value));
+            }
+            for (const auto& key : commit.keys_to_delete)
+              batch.Delete(leveldb_env::MakeSlice(key));
+            if (commit.copy_to_prefix) {
+              db.CopyPrefixed(commit.prefix, commit.copy_to_prefix.value(),
+                              &batch);
+            }
+            return db.Commit(&batch);
+          },
+          std::move(commit)),
+      base::BindOnce(&StorageAreaImpl::OnCommitComplete,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void StorageAreaImpl::OnCommitComplete(leveldb::Status status) {
   has_committed_data_ = true;
   --commit_batches_in_flight_;
   StartCommitTimer();
 
-  if (error != DatabaseError::OK) {
+  if (!status.ok())
     SetCacheMode(CacheMode::KEYS_AND_VALUES);
-  }
 
   // Call before |DidCommit| as delegate can destroy this object.
   UnloadMapIfPossible();
 
-  delegate_->DidCommit(error);
+  delegate_->DidCommit(status);
 }
 
 void StorageAreaImpl::UnloadMapIfPossible() {

content/browser/dom_storage/storage_area_impl.h

@@ -16,6 +16,7 @@
 #include "base/optional.h"
 #include "base/time/time.h"
 #include "components/services/leveldb/public/mojom/leveldb.mojom.h"
+#include "components/services/storage/dom_storage/dom_storage_database.h"
 #include "content/common/content_export.h"
 #include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -58,9 +59,11 @@ class CONTENT_EXPORT StorageAreaImpl : public blink::mojom::StorageArea {
    public:
     virtual ~Delegate();
     virtual void OnNoBindings() = 0;
-    virtual std::vector<leveldb::mojom::BatchedOperationPtr>
-    PrepareToCommit() = 0;
-    virtual void DidCommit(leveldb::mojom::DatabaseError error) = 0;
+    virtual void PrepareToCommit(
+        std::vector<storage::DomStorageDatabase::KeyValuePair>*
+            extra_entries_to_add,
+        std::vector<storage::DomStorageDatabase::Key>* extra_keys_to_delete);
+    virtual void DidCommit(leveldb::Status error) = 0;
     // Called during loading if no data was found. Needs to call |callback|.
     virtual void MigrateData(ValueMapCallback callback);
     // Called during loading to give delegate a chance to modify the data as
@@ -296,7 +299,7 @@ class CONTENT_EXPORT StorageAreaImpl : public blink::mojom::StorageArea {
   base::TimeDelta ComputeCommitDelay() const;
 
   void CommitChanges();
-  void OnCommitComplete(leveldb::mojom::DatabaseError error);
+  void OnCommitComplete(leveldb::Status status);
 
   void UnloadMapIfPossible();
 

content/browser/dom_storage/storage_area_impl_unittest.cc

@@ -53,11 +53,8 @@ class MockDelegate : public StorageAreaImpl::Delegate {
   ~MockDelegate() override {}
 
   void OnNoBindings() override {}
-  std::vector<leveldb::mojom::BatchedOperationPtr> PrepareToCommit() override {
-    return std::vector<leveldb::mojom::BatchedOperationPtr>();
-  }
-  void DidCommit(DatabaseError error) override {
-    if (error != DatabaseError::OK)
+  void DidCommit(leveldb::Status status) override {
+    if (!status.ok())
       LOG(ERROR) << "error committing!";
     if (committed_)
       std::move(committed_).Run();

third_party/leveldatabase/env_chromium.cc

@@ -1699,6 +1699,11 @@ leveldb::Slice MakeSlice(const base::StringPiece& s) {
   return leveldb::Slice(s.begin(), s.size());
 }
 
+leveldb::Slice MakeSlice(base::span<const uint8_t> s) {
+  return MakeSlice(
+      base::StringPiece(reinterpret_cast<const char*>(s.data()), s.size()));
+}
+
 }  // namespace leveldb_env
 
 namespace leveldb {

third_party/leveldatabase/env_chromium.h

@@ -372,6 +372,7 @@ LEVELDB_EXPORT leveldb::Status RewriteDB(const leveldb_env::Options& options,
 
 LEVELDB_EXPORT base::StringPiece MakeStringPiece(const leveldb::Slice& s);
 LEVELDB_EXPORT leveldb::Slice MakeSlice(const base::StringPiece& s);
+LEVELDB_EXPORT leveldb::Slice MakeSlice(base::span<const uint8_t> s);
 
 }  // namespace leveldb_env