Make SyncTaskManager's regular Task cancellable (like SyncTask)

A small fix so that modifying the existing code becomes safer.

BUG=none
TEST=SyncTaskManagerTest.ScheduleAndCancelTask

Review URL: https://chromiumcodereview.appspot.com/23513021

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@222214 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync_file_system/sync_task_manager.cc

@@ -38,7 +38,6 @@ class SyncTaskManager::TaskToken {
       // Reinitializes the token.
       manager_->NotifyTaskDone(
           make_scoped_ptr(new TaskToken(manager_)),
-          SyncStatusCallback(),
           SYNC_STATUS_OK);
     }
   }
@@ -64,7 +63,6 @@ SyncTaskManager::~SyncTaskManager() {
 void SyncTaskManager::Initialize(SyncStatusCode status) {
   DCHECK(!token_);
   NotifyTaskDone(make_scoped_ptr(new TaskToken(AsWeakPtr())),
-                 SyncStatusCallback(),
                  status);
 }
 
@@ -114,7 +112,6 @@ void SyncTaskManager::ScheduleSyncTaskIfIdle(scoped_ptr<SyncTask> task) {
 
 void SyncTaskManager::NotifyTaskDone(
     scoped_ptr<TaskToken> token,
-    const SyncStatusCallback& callback,
     SyncStatusCode status) {
   DCHECK(token);
   last_operation_status_ = status;
@@ -129,8 +126,9 @@ void SyncTaskManager::NotifyTaskDone(
   if (client_)
     client_->NotifyLastOperationStatus(last_operation_status_);
 
-  if (!callback.is_null())
-    callback.Run(status);
+  if (!current_callback_.is_null())
+    current_callback_.Run(status);
+  current_callback_.Reset();
 
   if (!pending_tasks_.empty()) {
     base::Closure closure = pending_tasks_.front();
@@ -157,8 +155,10 @@ SyncStatusCallback SyncTaskManager::CreateCompletionCallback(
     scoped_ptr<TaskToken> token,
     const SyncStatusCallback& callback) {
   DCHECK(token);
+  DCHECK(current_callback_.is_null());
+  current_callback_ = callback;
   return base::Bind(&SyncTaskManager::NotifyTaskDone,
-                    AsWeakPtr(), base::Passed(&token), callback);
+                    AsWeakPtr(), base::Passed(&token));
 }
 
 }  // namespace sync_file_system

chrome/browser/sync_file_system/sync_task_manager.h

@@ -48,8 +48,6 @@ class SyncTaskManager
   // service status. This should not be called more than once.
   void Initialize(SyncStatusCode status);
 
-  // Note that the argument of |task|'s parameter owns |callback|.
-  // The reference from |callback| to |task| causes circular dependency.
   void ScheduleTask(const Task& task,
                     const SyncStatusCallback& callback);
   void ScheduleSyncTask(scoped_ptr<SyncTask> task,
@@ -60,7 +58,6 @@ class SyncTaskManager
   void ScheduleSyncTaskIfIdle(scoped_ptr<SyncTask> task);
 
   void NotifyTaskDone(scoped_ptr<TaskToken> token,
-                      const SyncStatusCallback& callback,
                       SyncStatusCode status);
 
  private:
@@ -78,6 +75,8 @@ class SyncTaskManager
   SyncStatusCode last_operation_status_;
   scoped_ptr<SyncTask> running_task_;
   std::deque<base::Closure> pending_tasks_;
+  SyncStatusCallback current_callback_;
+
 
   // Absence of |token_| implies a task is running. Incoming tasks should
   // wait for the task to finish in |pending_tasks_| if |token_| is null.

chrome/browser/sync_file_system/sync_task_manager_unittest.cc

@@ -20,6 +20,15 @@ void IncrementAndAssign(int* counter,
   *status_out = status;
 }
 
+template <typename T>
+void IncrementAndAssignWithOwnedPointer(T* object,
+                                        int* counter,
+                                        SyncStatusCode* status_out,
+                                        SyncStatusCode status) {
+  ++(*counter);
+  *status_out = status;
+}
+
 class TaskManagerClient
     : public SyncTaskManager::Client,
       public base::SupportsWeakPtr<TaskManagerClient> {
@@ -235,4 +244,31 @@ TEST(SyncTaskManagerTest, ScheduleAndCancelSyncTask) {
   EXPECT_FALSE(task_completed);
 }
 
+TEST(SyncTaskManagerTest, ScheduleAndCancelTask) {
+  base::MessageLoop message_loop;
+
+  int callback_count = 0;
+  SyncStatusCode status = SYNC_STATUS_UNKNOWN;
+
+  bool task_started = false;
+  bool task_completed = false;
+
+  {
+    SyncTaskManager task_manager((base::WeakPtr<SyncTaskManager::Client>()));
+    task_manager.Initialize(SYNC_STATUS_OK);
+    MultihopSyncTask* task = new MultihopSyncTask(
+        &task_started, &task_completed);
+    task_manager.ScheduleTask(
+        base::Bind(&MultihopSyncTask::Run, base::Unretained(task)),
+        base::Bind(&IncrementAndAssignWithOwnedPointer<MultihopSyncTask>,
+                   base::Owned(task), &callback_count, &status));
+  }
+
+  message_loop.RunUntilIdle();
+  EXPECT_EQ(0, callback_count);
+  EXPECT_EQ(SYNC_STATUS_UNKNOWN, status);
+  EXPECT_TRUE(task_started);
+  EXPECT_FALSE(task_completed);
+}
+
 }  // namespace sync_file_system