Re-work the thread restrictions on the DOM distiller database

components/dom_distiller/core/dom_distiller_database.cc

Index: components/dom_distiller/core/dom_distiller_database.cc
diff --git a/components/dom_distiller/core/dom_distiller_database.cc b/components/dom_distiller/core/dom_distiller_database.cc
index a4d599c3401be6d189bb1527d99a42bfe844beb4..27c327adf87b726b0fbdee5b354deaf89b9465eb 100644
--- a/components/dom_distiller/core/dom_distiller_database.cc
+++ b/components/dom_distiller/core/dom_distiller_database.cc
@@ -23,11 +23,15 @@ using base::SequencedTaskRunner;
 
 namespace dom_distiller {
 
-DomDistillerDatabase::LevelDB::LevelDB() {}
+DomDistillerDatabase::LevelDB::LevelDB() {
+  thread_checker_.DetachFromThread();
+}
 
 DomDistillerDatabase::LevelDB::~LevelDB() {}

[Nico, 2013/11/03 23:29:52]

should the destructor check too? (the class has a scoped_ptr, so it seems to
matter)

[cjhopman, 2013/11/04 18:40:59]

Done.

 
 bool DomDistillerDatabase::LevelDB::Init(const base::FilePath& database_dir) {
+  thread_checker_.CalledOnValidThread();

[Nico, 2013/11/03 23:29:52]

This does nothing as is. The function just returns a bool. You want to DCHECK()
the result.

(I suppose the function should be WARN_UNUSED_RESULT. Filed
http://crbug.com/314505 for that – maybe you're looking for commons work to
do… ;-) )

[cjhopman, 2013/11/04 18:40:59]

Ha, that's amusing. Done.

+
   leveldb::Options options;
   options.create_if_missing = true;
   options.max_open_files = 0;  // Use minimum.
@@ -54,6 +58,8 @@ bool DomDistillerDatabase::LevelDB::Init(const base::FilePath& database_dir) {
 }
 
 bool DomDistillerDatabase::LevelDB::Save(const EntryVector& entries) {
+  thread_checker_.CalledOnValidThread();
+
   leveldb::WriteBatch updates;
   for (EntryVector::const_iterator it = entries.begin(); it != entries.end();
        ++it) {
@@ -72,6 +78,8 @@ bool DomDistillerDatabase::LevelDB::Save(const EntryVector& entries) {
 }
 
 bool DomDistillerDatabase::LevelDB::Load(EntryVector* entries) {
+  thread_checker_.CalledOnValidThread();
+
   leveldb::ReadOptions options;
   scoped_ptr<leveldb::Iterator> db_iterator(db_->NewIterator(options));
   for (db_iterator->SeekToFirst(); db_iterator->Valid(); db_iterator->Next()) {
@@ -88,26 +96,6 @@ bool DomDistillerDatabase::LevelDB::Load(EntryVector* entries) {
   return true;
 }
 
-DomDistillerDatabase::DomDistillerDatabase(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : task_runner_(task_runner), weak_ptr_factory_(this) {
-  main_loop_ = MessageLoop::current();
-}
-
-void DomDistillerDatabase::Destroy() {
-  DCHECK(IsRunOnMainLoop());
-  weak_ptr_factory_.InvalidateWeakPtrs();
-  task_runner_->PostNonNestableTask(
-      FROM_HERE,
-      base::Bind(&DomDistillerDatabase::DestroyFromTaskRunner,
-                 base::Unretained(this)));
-}
-
-void DomDistillerDatabase::Init(const base::FilePath& database_dir,
-                                InitCallback callback) {
-  InitWithDatabase(scoped_ptr<Database>(new LevelDB()), database_dir, callback);
-}
-
 namespace {
 
 void RunInitCallback(DomDistillerDatabaseInterface::InitCallback callback,
@@ -126,20 +114,59 @@ void RunLoadCallback(DomDistillerDatabaseInterface::LoadCallback callback,
   callback.Run(*success, entries.Pass());
 }
 
+void InitFromTaskRunner(DomDistillerDatabase::Database* database,
+                        const base::FilePath& database_dir,
+                        bool* success) {
+  DCHECK(success);
+
+  VLOG(1) << "Opening " << database_dir.value();

[Nico, 2013/11/03 23:29:52]

Is this useful? In general, stuff shouldn't log stuff unless necessary. Chrome
outputs way too much logspam.

(also for the other vlogs. also for the log(warning))

[cjhopman, 2013/11/04 18:40:59]

I've gone over the LOG/VLOG and removed/quieted them a bit.

+
+  // TODO(cjhopman): Histogram for database size.
+  *success = database->Init(database_dir);
+}
+
+void SaveEntriesFromTaskRunner(DomDistillerDatabase::Database* database,
+                               scoped_ptr<EntryVector> entries,
+                               bool* success) {
+  DCHECK(success);
+  VLOG(1) << "Saving " << entries->size() << " entry(ies) to database ";
+  *success = database->Save(*entries);
+}
+
+void LoadEntriesFromTaskRunner(DomDistillerDatabase::Database* database,
+                               EntryVector* entries,
+                               bool* success) {
+  DCHECK(success);
+  DCHECK(entries);
+
+  entries->clear();
+  *success = database->Load(entries);
+}
+
 }  // namespace
 
+DomDistillerDatabase::DomDistillerDatabase(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(task_runner) {
+}
+
+void DomDistillerDatabase::Init(const base::FilePath& database_dir,
+                                InitCallback callback) {
+  thread_checker_.CalledOnValidThread();
+  InitWithDatabase(scoped_ptr<Database>(new LevelDB()), database_dir, callback);
+}
+
 void DomDistillerDatabase::InitWithDatabase(scoped_ptr<Database> database,
                                             const base::FilePath& database_dir,
                                             InitCallback callback) {

[Nico, 2013/11/03 23:29:52]

Does this intentionally not use thread_checker_ ?

[cjhopman, 2013/11/04 18:40:59]

Hm. I was using thread_checker_ to enforce that users are using this correctly.
Since this isn't part of the public interface, it doesn't need to be checked.
That being said, it looks inconsistent being the only thing that doesn't check
it.

-  DCHECK(IsRunOnMainLoop());
   DCHECK(!db_);
   DCHECK(database);
   db_.reset(database.release());
   bool* success = new bool(false);
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::InitFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(InitFromTaskRunner,
+                 base::Unretained(db_.get()),
                  database_dir,
                  success),
       base::Bind(RunInitCallback, callback, base::Owned(success)));
@@ -147,20 +174,19 @@ void DomDistillerDatabase::InitWithDatabase(scoped_ptr<Database> database,
 
 void DomDistillerDatabase::SaveEntries(scoped_ptr<EntryVector> entries,
                                        SaveCallback callback) {
-  DCHECK(IsRunOnMainLoop());
+  thread_checker_.CalledOnValidThread();
   bool* success = new bool(false);
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::SaveEntriesFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(SaveEntriesFromTaskRunner,
+                 base::Unretained(db_.get()),
                  base::Passed(&entries),
                  success),
       base::Bind(RunSaveCallback, callback, base::Owned(success)));
 }
 
 void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
-  DCHECK(IsRunOnMainLoop());
-
+  thread_checker_.CalledOnValidThread();
   bool* success = new bool(false);
 
   scoped_ptr<EntryVector> entries(new EntryVector());
@@ -169,8 +195,8 @@ void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
 
   task_runner_->PostTaskAndReply(
       FROM_HERE,
-      base::Bind(&DomDistillerDatabase::LoadEntriesFromTaskRunner,
-                 base::Unretained(this),
+      base::Bind(LoadEntriesFromTaskRunner,
+                 base::Unretained(db_.get()),
                  entries_ptr,
                  success),
       base::Bind(RunLoadCallback,
@@ -179,50 +205,10 @@ void DomDistillerDatabase::LoadEntries(LoadCallback callback) {
                  base::Passed(&entries)));
 }
 
-DomDistillerDatabase::~DomDistillerDatabase() { DCHECK(IsRunByTaskRunner()); }
-
-bool DomDistillerDatabase::IsRunByTaskRunner() const {
-  return task_runner_->RunsTasksOnCurrentThread();
-}
-
-bool DomDistillerDatabase::IsRunOnMainLoop() const {
-  return MessageLoop::current() == main_loop_;
-}
-
-void DomDistillerDatabase::DestroyFromTaskRunner() {
-  DCHECK(IsRunByTaskRunner());
-  delete this;
-}
-
-void DomDistillerDatabase::InitFromTaskRunner(
-    const base::FilePath& database_dir,
-    bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-
-  VLOG(1) << "Opening " << database_dir.value();
-
-  // TODO(cjhopman): Histogram for database size.
-  *success = db_->Init(database_dir);
-}
-
-void DomDistillerDatabase::SaveEntriesFromTaskRunner(
-    scoped_ptr<EntryVector> entries,
-    bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-  VLOG(1) << "Saving " << entries->size() << " entry(ies) to database ";
-  *success = db_->Save(*entries);
-}
-
-void DomDistillerDatabase::LoadEntriesFromTaskRunner(EntryVector* entries,
-                                                     bool* success) {
-  DCHECK(IsRunByTaskRunner());
-  DCHECK(success);
-  DCHECK(entries);
-
-  entries->clear();
-  *success = db_->Load(entries);
+DomDistillerDatabase::~DomDistillerDatabase() {
+  if (!task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
+    LOG(WARNING) << "DOM distiller database will not be deleted.";
+  }
 }
 
 }  // namespace dom_distiller