Start adding tests for LevelDBWrapper, and fix a bunch of bugs.

In particular this fixes:
- loading data from leveldb should strip the prefix from the keys
- writing data to leveldb should preprend the prefix to the keys
- CommitChanges should actually commit changes

BUG=586194

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

content/browser/dom_storage/dom_storage_context_wrapper.cc

@@ -124,9 +124,6 @@ class DOMStorageContextWrapper::MojoState {
                         mojom::LevelDBObserverPtr observer,
                         mojom::LevelDBWrapperRequest request);
 
-  // Maps between an origin and its prefixed LevelDB view.
-  std::map<url::Origin, std::unique_ptr<LevelDBWrapperImpl>> level_db_wrappers_;
-
   service_manager::Connector* const connector_;
   const base::FilePath subdirectory_;
 
@@ -146,6 +143,9 @@ class DOMStorageContextWrapper::MojoState {
 
   std::vector<base::Closure> on_database_opened_callbacks_;
 
+  // Maps between an origin and its prefixed LevelDB view.
+  std::map<url::Origin, std::unique_ptr<LevelDBWrapperImpl>> level_db_wrappers_;
+
   base::WeakPtrFactory<MojoState> weak_ptr_factory_;
 };
 

content/browser/leveldb_wrapper_impl.cc

@@ -55,7 +55,7 @@ LevelDBWrapperImpl::LevelDBWrapperImpl(
     int max_bytes_per_hour,
     int max_commits_per_hour,
     const base::Closure& no_bindings_callback)
-    : prefix_(prefix),
+    : prefix_(leveldb::StdStringToUint8Vector(prefix)),
       no_bindings_callback_(no_bindings_callback),
       database_(database),
       bytes_used_(0),
@@ -249,7 +249,7 @@ void LevelDBWrapperImpl::LoadMap(const base::Closure& completion_callback) {
     return;
 
   // TODO(michaeln): Import from sqlite localstorage db.
-  database_->GetPrefixed(leveldb::StdStringToUint8Vector(prefix_),
+  database_->GetPrefixed(prefix_,
                          base::Bind(&LevelDBWrapperImpl::OnLoadComplete,
                                     weak_ptr_factory_.GetWeakPtr()));
 }
@@ -259,8 +259,11 @@ void LevelDBWrapperImpl::OnLoadComplete(
     std::vector<leveldb::mojom::KeyValuePtr> data) {
   DCHECK(!map_);
   map_.reset(new ValueMap);
-  for (auto& it : data)
-    (*map_)[it->key] = it->value;
+  for (auto& it : data) {
+    DCHECK_GE(it->key.size(), prefix_.size());
+    (*map_)[std::vector<uint8_t>(it->key.begin() + prefix_.size(),
+                                 it->key.end())] = it->value;
+  }
 
   // We proceed without using a backing store, nothing will be persisted but the
   // class is functional for the lifetime of the object.
@@ -316,7 +319,7 @@ base::TimeDelta LevelDBWrapperImpl::ComputeCommitDelay() const {
 
 void LevelDBWrapperImpl::CommitChanges() {
   DCHECK(database_);
-  if (commit_batch_)
+  if (!commit_batch_)
     return;
 
   commit_rate_limiter_.add_samples(1);
@@ -328,13 +331,15 @@ void LevelDBWrapperImpl::CommitChanges() {
     leveldb::mojom::BatchedOperationPtr item =
         leveldb::mojom::BatchedOperation::New();
     item->type = leveldb::mojom::BatchOperationType::DELETE_PREFIXED_KEY;
-    item->key = leveldb::StdStringToUint8Vector(prefix_);
+    item->key = prefix_;
     operations.push_back(std::move(item));
   }
   for (auto& it : commit_batch_->changed_values) {
     leveldb::mojom::BatchedOperationPtr item =
         leveldb::mojom::BatchedOperation::New();
-    item->key = std::move(it.first);
+    item->key.reserve(prefix_.size() + it.first.size());
+    item->key.insert(item->key.end(), prefix_.begin(), prefix_.end());
+    item->key.insert(item->key.end(), it.first.begin(), it.first.end());
     if (!it.second) {
       item->type = leveldb::mojom::BatchOperationType::DELETE_KEY;
     } else {

content/browser/leveldb_wrapper_impl.h

@@ -30,7 +30,7 @@ namespace content {
 // 2) Enforces a max_size constraint.
 // 3) Informs observers when values scoped by prefix are modified.
 // 4) Throttles requests to avoid overwhelming the disk.
-class LevelDBWrapperImpl : public mojom::LevelDBWrapper {
+class CONTENT_EXPORT LevelDBWrapperImpl : public mojom::LevelDBWrapper {
  public:
   // |no_bindings_callback| will be called when this object has no more
   // bindings.
@@ -53,6 +53,8 @@ class LevelDBWrapperImpl : public mojom::LevelDBWrapper {
   static void EnableAggressiveCommitDelay();
 
  private:
+  friend class LevelDBWrapperImplTest;
+
   using ValueMap = std::map<std::vector<uint8_t>, std::vector<uint8_t>>;
   using ChangedValueMap =
       std::map<std::vector<uint8_t>, base::Optional<std::vector<uint8_t>>>;
@@ -114,7 +116,7 @@ class LevelDBWrapperImpl : public mojom::LevelDBWrapper {
   void CommitChanges();
   void OnCommitComplete(leveldb::mojom::DatabaseError error);
 
-  std::string prefix_;
+  std::vector<uint8_t> prefix_;
   mojo::BindingSet<mojom::LevelDBWrapper> bindings_;
   mojo::InterfacePtrSet<mojom::LevelDBObserver> observers_;
   base::Closure no_bindings_callback_;

content/test/BUILD.gn

@@ -1097,6 +1097,7 @@ test("content_unittests") {
     "../browser/indexed_db/mock_indexed_db_database_callbacks.h",
     "../browser/indexed_db/mock_indexed_db_factory.cc",
     "../browser/indexed_db/mock_indexed_db_factory.h",
+    "../browser/leveldb_wrapper_impl_unittest.cc",
     "../browser/loader/async_resource_handler_unittest.cc",
     "../browser/loader/async_revalidation_driver_unittest.cc",
     "../browser/loader/async_revalidation_manager_unittest.cc",
@@ -1385,6 +1386,8 @@ test("content_unittests") {
     "//cc/ipc",
     "//cc/surfaces",
     "//components/display_compositor",
+    "//components/leveldb/public/cpp",
+    "//components/leveldb/public/interfaces",
     "//components/payments:payment_app",
     "//components/rappor:test_support",
     "//content:resources",