Refactor ProtoDatabase(Impl) to allow DBs with multiple proto types.

Currently, ProtoDatabase<T> only allows us to have a LevelDB that stores
a single proto type.

In preparation for a single unified proto DB, ProtoDatabase and
ProtoDatabaseImpl have been refactored. Much of ProtoDatabaseImpl's
logic has been moved into ProtoLevelDBWrapper, which contains template
functions instead of being a template class.

ProtoDatabase<T> provides the interface we have today, and we now
have a UniqueProtoDatabase<T> as a thin layer above ProtoDatabase so
that it can manage the unique_ptr to its own LevelDB.
ProtoDatabaseImpl<T> is kept around as an alias for UniqueProtoDatabase
for compatibility, and as a result no ProtoDatabase(Impl) users are
negatively impacted by this change.

The addition of the ProtoLevelDBWrapper provides a convenient place to
record metrics for the various database clients.

Bug: 870813
Change-Id: I6175a3cbea5dd312f09c1d88d5ad80f1f4b26006
Reviewed-on: https://chromium-review.googlesource.com/c/1170093
Commit-Queue: Troy Hildebrandt <thildebr@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598782}

components/leveldb_proto/BUILD.gn

@@ -9,6 +9,9 @@ static_library("leveldb_proto") {
     "proto_database.cc",
     "proto_database.h",
     "proto_database_impl.h",
+    "proto_leveldb_wrapper.cc",
+    "proto_leveldb_wrapper.h",
+    "unique_proto_database.h",
   ]
 
   public_deps = [
@@ -26,6 +29,7 @@ source_set("test_support") {
   public_deps = [
     ":leveldb_proto",
     "//base",
+    "//base/test:test_support",
     "//components/leveldb_proto/testing/proto",
   ]
 }
@@ -33,7 +37,7 @@ source_set("test_support") {
 source_set("unit_tests") {
   testonly = true
   sources = [
-    "proto_database_impl_unittest.cc",
+    "unique_proto_database_unittest.cc",
   ]
   deps = [
     ":test_support",

components/leveldb_proto/leveldb_database.h

@@ -10,10 +10,7 @@
 #include <string>
 #include <vector>
 
-#include "base/gtest_prod_util.h"
-#include "base/macros.h"
 #include "base/strings/string_split.h"
-#include "base/threading/thread_collision_warner.h"
 #include "third_party/leveldatabase/env_chromium.h"
 
 namespace base {
@@ -25,7 +22,6 @@ namespace leveldb {
 class Cache;
 class DB;
 class Env;
-class Status;
 }  // namespace leveldb
 
 namespace leveldb_proto {

components/leveldb_proto/proto_database.h

@@ -6,18 +6,12 @@
 #define COMPONENTS_LEVELDB_PROTO_PROTO_DATABASE_H_
 
 #include <map>
-#include <memory>
-#include <string>
-#include <utility>
 #include <vector>
 
-#include "base/callback.h"
+#include "base/sequenced_task_runner.h"
+#include "base/threading/thread_checker.h"
 #include "components/leveldb_proto/leveldb_database.h"
-#include "third_party/leveldatabase/env_chromium.h"
-
-namespace base {
-class FilePath;
-}
+#include "components/leveldb_proto/proto_leveldb_wrapper.h"
 
 namespace leveldb_proto {
 
@@ -43,6 +37,9 @@ class ProtoDatabase {
   // A list of key-value (string, T) tuples.
   using KeyEntryVector = std::vector<std::pair<std::string, T>>;
 
+  explicit ProtoDatabase(
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+      : db_wrapper_(std::make_unique<ProtoLevelDBWrapper>(task_runner)) {}
   virtual ~ProtoDatabase() {}
 
   // Asynchronously initializes the object with the specified |options|.
@@ -50,7 +47,15 @@ class ProtoDatabase {
   virtual void Init(const char* client_name,
                     const base::FilePath& database_dir,
                     const leveldb_env::Options& options,
-                    InitCallback callback) = 0;
+                    typename ProtoDatabase<T>::InitCallback callback) = 0;
+
+  virtual void InitWithDatabase(LevelDB* database,
+                                const base::FilePath& database_dir,
+                                const leveldb_env::Options& options,
+                                InitCallback callback) {
+    db_wrapper_->InitWithDatabase(database, database_dir, options,
+                                  std::move(callback));
+  }
 
   // Asynchronously saves |entries_to_save| and deletes entries from
   // |keys_to_remove| from the database. |callback| will be invoked on the
@@ -58,7 +63,11 @@ class ProtoDatabase {
   virtual void UpdateEntries(
       std::unique_ptr<KeyEntryVector> entries_to_save,
       std::unique_ptr<std::vector<std::string>> keys_to_remove,
-      UpdateCallback callback) = 0;
+      UpdateCallback callback) {
+    db_wrapper_->template UpdateEntries<T>(std::move(entries_to_save),
+                                           std::move(keys_to_remove),
+                                           std::move(callback));
+  }
 
   // Asynchronously saves |entries_to_save| and deletes entries that satisfies
   // the |delete_key_filter| from the database. |callback| will be invoked on
@@ -67,44 +76,76 @@ class ProtoDatabase {
   virtual void UpdateEntriesWithRemoveFilter(
       std::unique_ptr<KeyEntryVector> entries_to_save,
       const LevelDB::KeyFilter& delete_key_filter,
-      UpdateCallback callback) = 0;
+      UpdateCallback callback) {
+    db_wrapper_->template UpdateEntriesWithRemoveFilter<T>(
+        std::move(entries_to_save), delete_key_filter, std::move(callback));
+  }
 
   // Asynchronously loads all entries from the database and invokes |callback|
   // when complete.
-  virtual void LoadEntries(LoadCallback callback) = 0;
+  virtual void LoadEntries(LoadCallback callback) {
+    db_wrapper_->template LoadEntries<T>(std::move(callback));
+  }
 
   // Asynchronously loads entries that satisfies the |filter| from the database
   // and invokes |callback| when complete. The filter will be called on
   // ProtoDatabase's taskrunner.
   virtual void LoadEntriesWithFilter(const LevelDB::KeyFilter& filter,
-                                     LoadCallback callback) = 0;
-  virtual void LoadEntriesWithFilter(const LevelDB::KeyFilter& filter,
+                                     LoadCallback callback) {
+    db_wrapper_->template LoadEntriesWithFilter<T>(filter, std::move(callback));
+  }
+
+  virtual void LoadEntriesWithFilter(const LevelDB::KeyFilter& key_filter,
                                      const leveldb::ReadOptions& options,
                                      const std::string& target_prefix,
-                                     LoadCallback callback) = 0;
+                                     LoadCallback callback) {
+    db_wrapper_->template LoadEntriesWithFilter<T>(
+        key_filter, options, target_prefix, std::move(callback));
+  }
+
+  virtual void LoadKeysAndEntries(LoadKeysAndEntriesCallback callback) {
+    db_wrapper_->template LoadKeysAndEntries<T>(std::move(callback));
+  }
 
-  virtual void LoadKeysAndEntries(
-      typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) = 0;
   virtual void LoadKeysAndEntriesWithFilter(
       const LevelDB::KeyFilter& filter,
-      typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) = 0;
+      typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) {
+    db_wrapper_->template LoadKeysAndEntriesWithFilter<T>(filter,
+                                                          std::move(callback));
+  }
   virtual void LoadKeysAndEntriesWithFilter(
       const LevelDB::KeyFilter& filter,
       const leveldb::ReadOptions& options,
       const std::string& target_prefix,
-      typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) = 0;
+      typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) {
+    db_wrapper_->template LoadKeysAndEntriesWithFilter<T>(
+        filter, options, target_prefix, std::move(callback));
+  }
 
   // Asynchronously loads all keys from the database and invokes |callback| with
   // those keys when complete.
-  virtual void LoadKeys(LoadKeysCallback callback) = 0;
+  virtual void LoadKeys(LoadKeysCallback callback) {
+    db_wrapper_->LoadKeys(std::move(callback));
+  }
 
   // Asynchronously loads a single entry, identified by |key|, from the database
   // and invokes |callback| when complete. If no entry with |key| is found,
   // a nullptr is passed to the callback, but the success flag is still true.
-  virtual void GetEntry(const std::string& key, GetCallback callback) = 0;
+  virtual void GetEntry(const std::string& key, GetCallback callback) {
+    db_wrapper_->template GetEntry<T>(key, std::move(callback));
+  }
 
   // Asynchronously destroys the database.
-  virtual void Destroy(DestroyCallback callback) = 0;
+  virtual void Destroy(DestroyCallback callback) {
+    db_wrapper_->Destroy(std::move(callback));
+  }
+
+  bool GetApproximateMemoryUse(uint64_t* approx_mem_use) {
+    return db_wrapper_->GetApproximateMemoryUse(approx_mem_use);
+  }
+
+ protected:
+  std::unique_ptr<ProtoLevelDBWrapper> db_wrapper_;
 };
 
 // Return a new instance of Options, but with two additions:

components/leveldb_proto/proto_database_perftest.cc

@@ -24,8 +24,8 @@
 #include "base/threading/thread.h"
 #include "build/build_config.h"
 #include "components/leveldb_proto/leveldb_database.h"
-#include "components/leveldb_proto/proto_database_impl.h"
 #include "components/leveldb_proto/testing/proto/test_db.pb.h"
+#include "components/leveldb_proto/unique_proto_database.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/perf/perf_test.h"
@@ -85,7 +85,7 @@ class TestDatabase {
   TestDatabase(const std::string& name,
                scoped_refptr<base::SingleThreadTaskRunner> task_runner,
                const base::FilePath& path) {
-    db_.reset(new ProtoDatabaseImpl<TestProto>(task_runner));
+    db_.reset(new UniqueProtoDatabase<TestProto>(task_runner));
     leveldb_env::Options options = leveldb_proto::CreateSimpleOptions();
 
     base::RunLoop run_init_db;
@@ -102,11 +102,11 @@ class TestDatabase {
   }
 
   bool is_initialized() const { return is_initialized_; }
-  ProtoDatabaseImpl<TestProto>* proto_db() const { return db_.get(); }
+  UniqueProtoDatabase<TestProto>* proto_db() const { return db_.get(); }
 
  private:
   bool is_initialized_ = false;
-  std::unique_ptr<ProtoDatabaseImpl<TestProto>> db_;
+  std::unique_ptr<UniqueProtoDatabase<TestProto>> db_;
 };
 
 }  // namespace

components/leveldb_proto/proto_leveldb_wrapper.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/leveldb_proto/proto_leveldb_wrapper.h"
+
+namespace leveldb_proto {
+
+namespace {
+
+void RunInitCallback(typename ProtoLevelDBWrapper::InitCallback callback,
+                     const bool* success) {
+  std::move(callback).Run(*success);
+}
+
+inline void InitFromTaskRunner(LevelDB* database,
+                               const base::FilePath& database_dir,
+                               const leveldb_env::Options& options,
+                               bool* success) {
+  DCHECK(success);
+
+  // TODO(cjhopman): Histogram for database size.
+  *success = database->Init(database_dir, options);
+}
+
+void RunDestroyCallback(typename ProtoLevelDBWrapper::DestroyCallback callback,
+                        const bool* success) {
+  std::move(callback).Run(*success);
+}
+
+inline void DestroyFromTaskRunner(LevelDB* database, bool* success) {
+  CHECK(success);
+
+  *success = database->Destroy();
+}
+
+void RunLoadKeysCallback(
+    typename ProtoLevelDBWrapper::LoadKeysCallback callback,
+    std::unique_ptr<bool> success,
+    std::unique_ptr<std::vector<std::string>> keys) {
+  std::move(callback).Run(*success, std::move(keys));
+}
+
+inline void LoadKeysFromTaskRunner(LevelDB* database,
+                                   std::vector<std::string>* keys,
+                                   bool* success) {
+  DCHECK(success);
+  DCHECK(keys);
+  keys->clear();
+  *success = database->LoadKeys(keys);
+}
+
+}  // namespace
+
+ProtoLevelDBWrapper::ProtoLevelDBWrapper(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+    : task_runner_(task_runner) {}
+
+ProtoLevelDBWrapper::~ProtoLevelDBWrapper() = default;
+
+void ProtoLevelDBWrapper::InitWithDatabase(
+    LevelDB* database,
+    const base::FilePath& database_dir,
+    const leveldb_env::Options& options,
+    typename ProtoLevelDBWrapper::InitCallback callback) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(!db_);
+  DCHECK(database);
+  db_ = database;
+  bool* success = new bool(false);
+  task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(InitFromTaskRunner, base::Unretained(db_), database_dir,
+                     options, success),
+      base::BindOnce(RunInitCallback, std::move(callback),
+                     base::Owned(success)));
+}
+
+void ProtoLevelDBWrapper::Destroy(
+    typename ProtoLevelDBWrapper::DestroyCallback callback) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(db_);
+
+  bool* success = new bool(false);
+  task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(DestroyFromTaskRunner, base::Unretained(db_), success),
+      base::BindOnce(RunDestroyCallback, std::move(callback),
+                     base::Owned(success)));
+}
+
+void ProtoLevelDBWrapper::LoadKeys(
+    typename ProtoLevelDBWrapper::LoadKeysCallback callback) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  auto success = std::make_unique<bool>(false);
+  auto keys = std::make_unique<std::vector<std::string>>();
+  auto load_task = base::BindOnce(LoadKeysFromTaskRunner, base::Unretained(db_),
+                                  keys.get(), success.get());
+  task_runner_->PostTaskAndReply(
+      FROM_HERE, std::move(load_task),
+      base::BindOnce(RunLoadKeysCallback, std::move(callback),
+                     std::move(success), std::move(keys)));
+}
+
+bool ProtoLevelDBWrapper::GetApproximateMemoryUse(uint64_t* approx_mem_use) {
+  return db_->GetApproximateMemoryUse(approx_mem_use);
+}
+
+const scoped_refptr<base::SequencedTaskRunner>&
+ProtoLevelDBWrapper::task_runner() {
+  return task_runner_;
+}
+
+}  // namespace leveldb_proto
\ No newline at end of file

components/leveldb_proto/testing/fake_db.h

@@ -14,6 +14,8 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
+#include "base/task/post_task.h"
+#include "base/test/test_simple_task_runner.h"
 #include "components/leveldb_proto/proto_database.h"
 
 namespace leveldb_proto {
@@ -34,6 +36,11 @@ class FakeDB : public ProtoDatabase<T> {
             const base::FilePath& database_dir,
             const leveldb_env::Options& options,
             typename ProtoDatabase<T>::InitCallback callback) override;
+  void InitWithDatabase(
+      LevelDB* database,
+      const base::FilePath& database_dir,
+      const leveldb_env::Options& options,
+      typename ProtoLevelDBWrapper::InitCallback callback) override;
   void UpdateEntries(
       std::unique_ptr<typename ProtoDatabase<T>::KeyEntryVector>
           entries_to_save,
@@ -115,7 +122,9 @@ class FakeDB : public ProtoDatabase<T> {
 
 template <typename T>
 FakeDB<T>::FakeDB(EntryMap* db)
-    : db_(db) {}
+    : ProtoDatabase<T>(base::MakeRefCounted<base::TestSimpleTaskRunner>()) {
+  db_ = db;
+}
 
 template <typename T>
 FakeDB<T>::~FakeDB() {}
@@ -129,6 +138,15 @@ void FakeDB<T>::Init(const char* client_name,
   init_callback_ = std::move(callback);
 }
 
+template <typename T>
+void FakeDB<T>::InitWithDatabase(
+    LevelDB* database,
+    const base::FilePath& database_dir,
+    const leveldb_env::Options& options,
+    typename ProtoLevelDBWrapper::InitCallback callback) {
+  Init("", database_dir, options, std::move(callback));
+}
+
 template <typename T>
 void FakeDB<T>::UpdateEntries(
     std::unique_ptr<typename ProtoDatabase<T>::KeyEntryVector> entries_to_save,

components/leveldb_proto/unique_proto_database.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_LEVELDB_PROTO_UNIQUE_PROTO_DATABASE_H_
+#define COMPONENTS_LEVELDB_PROTO_UNIQUE_PROTO_DATABASE_H_
+
+#include "base/sequenced_task_runner.h"
+#include "base/threading/thread_checker.h"
+#include "components/leveldb_proto/leveldb_database.h"
+#include "components/leveldb_proto/proto_database.h"
+#include "components/leveldb_proto/proto_leveldb_wrapper.h"
+
+namespace leveldb_proto {
+
+// An implementation of ProtoDatabase<T> that manages the lifecycle of a unique
+// LevelDB instance.
+template <typename T>
+class UniqueProtoDatabase : public ProtoDatabase<T> {
+ public:
+  UniqueProtoDatabase(
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner)
+      : ProtoDatabase<T>(task_runner) {}
+
+  virtual void Init(const char* client_name,
+                    const base::FilePath& database_dir,
+                    const leveldb_env::Options& options,
+                    typename ProtoDatabase<T>::InitCallback callback) override {
+    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    db_ = std::make_unique<LevelDB>(client_name);
+    ProtoDatabase<T>::InitWithDatabase(db_.get(), database_dir, options,
+                                       std::move(callback));
+  }
+
+  virtual ~UniqueProtoDatabase() {
+    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    if (db_.get() &&
+        !this->db_wrapper_->task_runner()->DeleteSoon(FROM_HERE, db_.release()))
+      DLOG(WARNING) << "Proto database will not be deleted.";
+  }
+
+ private:
+  THREAD_CHECKER(thread_checker_);
+
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  std::unique_ptr<LevelDB> db_;
+};
+
+}  // namespace leveldb_proto
+
+#endif  // COMPONENTS_LEVELDB_PROTO_UNIQUE_PROTO_DATABASE_H_