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

components/sync/engine/browser_thread_model_worker.cc

 // Copyright 2012 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/sync/engine/browser_thread_model_worker.h"
 
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/synchronization/waitable_event.h"
-
-using base::SingleThreadTaskRunner;
+#include <utility>
 
 namespace syncer {
 
 BrowserThreadModelWorker::BrowserThreadModelWorker(
-    const scoped_refptr<SingleThreadTaskRunner>& runner,
+    const scoped_refptr<base::SingleThreadTaskRunner>& runner,
     ModelSafeGroup group)
     : runner_(runner), group_(group) {}
 
-SyncerError BrowserThreadModelWorker::DoWorkAndWaitUntilDoneImpl(
-    const WorkCallback& work) {
-  SyncerError error = UNSET;
+void BrowserThreadModelWorker::ScheduleWork(base::Closure work) {
   if (runner_->BelongsToCurrentThread()) {
     DLOG(WARNING) << "Already on thread " << runner_;
-    return work.Run();
+    work.Run();
+    return;

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

What do you think of an else clause instead of having an early return?

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

Done.

   }
 
-  // Signaled when the task is deleted, i.e. after it runs or when it is
-  // abandoned.
-  base::WaitableEvent work_done_or_abandoned(
-      base::WaitableEvent::ResetPolicy::AUTOMATIC,
-      base::WaitableEvent::InitialState::NOT_SIGNALED);
-
-  if (!runner_->PostTask(
-          FROM_HERE,
-          base::Bind(
-              &BrowserThreadModelWorker::CallDoWorkAndSignalTask, this, work,
-              base::Passed(syncer::ScopedEventSignal(&work_done_or_abandoned)),
-              &error))) {
-    DLOG(WARNING) << "Failed to post task to runner " << runner_;
-    error = CANNOT_DO_WORK;
-    return error;
-  }
-  work_done_or_abandoned.Wait();
-  return error;
+  runner_->PostTask(FROM_HERE, std::move(work));

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

You're very consistent with your std::move(work). This would compile without the
moves? Is there a reason you're not passing OnceCallbacks here?

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

std::move() avoids ref count churn (see previous comment).

I wasn't passing OnceCalbacks because TaskRunner::PostTask didn't accept it, but
it does now.

 }
 
 ModelSafeGroup BrowserThreadModelWorker::GetModelSafeGroup() {
   return group_;
 }
 
 bool BrowserThreadModelWorker::IsOnModelThread() {
   return runner_->BelongsToCurrentThread();
 }
 
 BrowserThreadModelWorker::~BrowserThreadModelWorker() {}
 
-void BrowserThreadModelWorker::CallDoWorkAndSignalTask(
-    const WorkCallback& work,
-    syncer::ScopedEventSignal scoped_event_signal,
-    SyncerError* error) {
-  DCHECK(runner_->BelongsToCurrentThread());
-  if (!IsStopped())
-    *error = work.Run();
-  // The event in |scoped_event_signal| is signaled at the end of this scope.
-}
-
 }  // namespace syncer