[Sync] Refactor ModelSafeWorker::DoWorkAndWaitUntilDone() to avoid code duplication.

components/history/core/browser/history_model_worker.cc

Index: components/history/core/browser/history_model_worker.cc
diff --git a/components/history/core/browser/history_model_worker.cc b/components/history/core/browser/history_model_worker.cc
index 72b843dc9ffc765938f73116d1f5a49c1f27f4cd..5956048a3e2cbb87f1c1e39ebc43cc8b31431b0a 100644
--- a/components/history/core/browser/history_model_worker.cc
+++ b/components/history/core/browser/history_model_worker.cc
@@ -6,30 +6,19 @@
 
 #include <utility>
 
-#include "base/synchronization/waitable_event.h"
+#include "base/memory/ptr_util.h"
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service.h"
-#include "components/sync/base/scoped_event_signal.h"
 
 namespace browser_sync {
 
 class WorkerTask : public history::HistoryDBTask {
  public:
-  WorkerTask(const syncer::WorkCallback& work,
-             syncer::ScopedEventSignal scoped_event_signal,
-             syncer::SyncerError* error)
-      : work_(work),
-        scoped_event_signal_(std::move(scoped_event_signal)),
-        error_(error) {}
+  WorkerTask(base::OnceClosure work) : work_(std::move(work)) {}
 
   bool RunOnDBThread(history::HistoryBackend* backend,
                      history::HistoryDatabase* db) override {
-    // Signal the completion event at the end of this scope.
-    auto scoped_event_signal = std::move(scoped_event_signal_);
-
-    // Run the task.
-    *error_ = work_.Run();
-
+    std::move(work_).Run();
     return true;
   }
 
@@ -37,34 +26,12 @@ class WorkerTask : public history::HistoryDBTask {
   // any code asynchronously on the main thread after completion.
   void DoneRunOnMainThread() override {}
 
- protected:
-  ~WorkerTask() override {
-    // The event in |scoped_event_signal_| is signaled at the end of this
-    // scope if this is destroyed before RunOnDBThread runs.
-  }
-
-  syncer::WorkCallback work_;
-  syncer::ScopedEventSignal scoped_event_signal_;
-  syncer::SyncerError* error_;
-};
-
-class AddDBThreadObserverTask : public history::HistoryDBTask {
- public:
-  explicit AddDBThreadObserverTask(base::Closure register_callback)
-     : register_callback_(register_callback) {}
-
-  bool RunOnDBThread(history::HistoryBackend* backend,
-                     history::HistoryDatabase* db) override {
-    register_callback_.Run();
-    return true;
-  }
-
-  void DoneRunOnMainThread() override {}
-
  private:
-  ~AddDBThreadObserverTask() override {}
+  // A OnceClosure is deleted right after it runs. This is important to unblock
+  // DoWorkAndWaitUntilDone() right after the task runs.
+  base::OnceClosure work_;
 
-  base::Closure register_callback_;
+  DISALLOW_COPY_AND_ASSIGN(WorkerTask);
 };
 
 namespace {
@@ -73,18 +40,11 @@ namespace {
 // thread.
 void PostWorkerTask(
     const base::WeakPtr<history::HistoryService>& history_service,
-    const syncer::WorkCallback& work,
-    syncer::ScopedEventSignal scoped_event_signal,
-    base::CancelableTaskTracker* cancelable_tracker,
-    syncer::SyncerError* error) {
+    base::OnceClosure work,
+    base::CancelableTaskTracker* cancelable_tracker) {
   if (history_service.get()) {
-    std::unique_ptr<history::HistoryDBTask> task(
-        new WorkerTask(work, std::move(scoped_event_signal), error));
-    history_service->ScheduleDBTask(std::move(task), cancelable_tracker);
-  } else {
-    *error = syncer::CANNOT_DO_WORK;
-    // The event in |scoped_event_signal| is signaled at the end of this
-    // scope.
+    history_service->ScheduleDBTask(
+        base::MakeUnique<WorkerTask>(std::move(work)), cancelable_tracker);
   }
 }
 
@@ -99,27 +59,6 @@ HistoryModelWorker::HistoryModelWorker(
   cancelable_tracker_.reset(new base::CancelableTaskTracker);
 }
 
-syncer::SyncerError HistoryModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const syncer::WorkCallback& work) {
-  syncer::SyncerError error = syncer::UNSET;
-
-  // Signaled after the task runs or when it is abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  if (ui_thread_->PostTask(FROM_HERE,
-                           base::Bind(&PostWorkerTask, history_service_, work,
-                                      base::Passed(syncer::ScopedEventSignal(
-                                          &work_done_or_abandoned)),
-                                      cancelable_tracker_.get(), &error))) {
-    work_done_or_abandoned.Wait();
-  } else {
-    error = syncer::CANNOT_DO_WORK;
-  }
-  return error;
-}
-
 syncer::ModelSafeGroup HistoryModelWorker::GetModelSafeGroup() {
   return syncer::GROUP_HISTORY;
 }
@@ -138,4 +77,10 @@ HistoryModelWorker::~HistoryModelWorker() {
   ui_thread_->DeleteSoon(FROM_HERE, cancelable_tracker_.release());
 }
 
+void HistoryModelWorker::ScheduleWork(base::Closure work) {
+  ui_thread_->PostTask(
+      FROM_HERE, base::Bind(&PostWorkerTask, history_service_, std::move(work),

[skym, 2017/04/06 00:34:54]

Is the Closure implicitly converted to a OnceClosure here? How does that work?

[fdoray, 2017/04/06 18:10:55]

Yes, a Closure can be implicitly converted to OnceClosure, but not the reverse.
A OnceClosure DCHECKs if run more than once.

Before, I could not use OnceClosure as argument to ScheduleWork() because
TaskRunner::PostTask() took a Closure as argument. Thanks to
https://codereview.chromium.org/2637843002, I can now use OnceClosure
everywhere.

+                            cancelable_tracker_.get()));
+}
+
 }  // namespace browser_sync