Simplify file_system_provider::Queue.

IsAborted() and Remove() were removed, and semantics of Abort() simplified.

TEST=unit_tests: *FileSystemProvider*
BUG=463344

Review URL: https://codereview.chromium.org/971303003

Cr-Commit-Position: refs/heads/master@{#319041}

chrome/browser/chromeos/file_system_provider/provided_file_system.cc

@@ -654,7 +654,6 @@ void ProvidedFileSystem::OnAddWatcherInQueueCompleted(
   if (result != base::File::FILE_OK) {
     callback.Run(result);
     watcher_queue_.Complete(token);
-    watcher_queue_.Remove(token);
     return;
   }
 
@@ -663,7 +662,6 @@ void ProvidedFileSystem::OnAddWatcherInQueueCompleted(
   if (it != watchers_.end()) {
     callback.Run(base::File::FILE_OK);
     watcher_queue_.Complete(token);
-    watcher_queue_.Remove(token);
     return;
   }
 
@@ -678,7 +676,6 @@ void ProvidedFileSystem::OnAddWatcherInQueueCompleted(
 
   callback.Run(base::File::FILE_OK);
   watcher_queue_.Complete(token);
-  watcher_queue_.Remove(token);
 }
 
 void ProvidedFileSystem::OnRemoveWatcherInQueueCompleted(
@@ -690,7 +687,6 @@ void ProvidedFileSystem::OnRemoveWatcherInQueueCompleted(
     base::File::Error result) {
   if (!extension_response && result != base::File::FILE_OK) {
     watcher_queue_.Complete(token);
-    watcher_queue_.Remove(token);
     callback.Run(result);
     return;
   }
@@ -712,7 +708,6 @@ void ProvidedFileSystem::OnRemoveWatcherInQueueCompleted(
 
   callback.Run(base::File::FILE_OK);
   watcher_queue_.Complete(token);
-  watcher_queue_.Remove(token);
 }
 
 void ProvidedFileSystem::OnNotifyInQueueCompleted(
@@ -721,7 +716,6 @@ void ProvidedFileSystem::OnNotifyInQueueCompleted(
   if (result != base::File::FILE_OK) {
     args->callback.Run(result);
     watcher_queue_.Complete(args->token);
-    watcher_queue_.Remove(args->token);
     return;
   }
 
@@ -731,7 +725,6 @@ void ProvidedFileSystem::OnNotifyInQueueCompleted(
   if (it == watchers_.end()) {
     args->callback.Run(base::File::FILE_ERROR_NOT_FOUND);
     watcher_queue_.Complete(args->token);
-    watcher_queue_.Remove(args->token);
     return;
   }
 
@@ -754,7 +747,6 @@ void ProvidedFileSystem::OnNotifyInQueueCompleted(
 
   args->callback.Run(base::File::FILE_OK);
   watcher_queue_.Complete(args->token);
-  watcher_queue_.Remove(args->token);
 }
 
 void ProvidedFileSystem::OnOpenFileCompleted(const base::FilePath& file_path,

chrome/browser/chromeos/file_system_provider/queue.cc

@@ -50,37 +50,17 @@ void Queue::Enqueue(size_t token, const AbortableCallback& callback) {
 
 void Queue::Complete(size_t token) {
   const auto it = executed_.find(token);
-  CHECK(it != executed_.end());
-  completed_[token] = it->second;
+  DCHECK(it != executed_.end());
   executed_.erase(it);
-}
-
-void Queue::Remove(size_t token) {
-  const auto it = completed_.find(token);
-  if (it != completed_.end()) {
-    completed_.erase(it);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&Queue::MaybeRun, weak_ptr_factory_.GetWeakPtr()));
-    return;
-  }
-
-  // If not completed, then it must have been aborted.
-  const auto aborted_it = aborted_.find(token);
-  CHECK(aborted_it != aborted_.end());
-  aborted_.erase(aborted_it);
-
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&Queue::MaybeRun, weak_ptr_factory_.GetWeakPtr()));
 }
 
 void Queue::MaybeRun() {
-  if (executed_.size() + completed_.size() == max_in_parallel_ ||
-      !pending_.size()) {
+  if (executed_.size() == max_in_parallel_ || !pending_.size())
     return;
-  }
 
-  CHECK_GT(max_in_parallel_, executed_.size() + completed_.size());
+  CHECK_GT(max_in_parallel_, executed_.size());
   Task task = pending_.front();
   pending_.pop_front();
 
@@ -95,24 +75,20 @@ void Queue::MaybeRun() {
 }
 
 void Queue::Abort(size_t token) {
-  // Check if it's running.
+  // Check if it's running. If so, then abort and expect a Complete() call soon.
   const auto it = executed_.find(token);
   if (it != executed_.end()) {
-    Task task = it->second;
-    aborted_[token] = task;
-    executed_.erase(it);
-    CHECK(!task.abort_callback.is_null());
-    task.abort_callback.Run();
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&Queue::MaybeRun, weak_ptr_factory_.GetWeakPtr()));
+    Task& task = it->second;
+    AbortCallback abort_callback = task.abort_callback;
+    task.abort_callback = AbortCallback();
+    DCHECK(!abort_callback.is_null());
+    abort_callback.Run();
     return;
   }
 
   // Aborting not running tasks is linear. TODO(mtomasz): Optimize if feasible.
   for (auto it = pending_.begin(); it != pending_.end(); ++it) {
     if (token == it->token) {
-      aborted_[token] = *it;
       pending_.erase(it);
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
@@ -125,21 +101,5 @@ void Queue::Abort(size_t token) {
   NOTREACHED();
 }
 
-bool Queue::IsAborted(size_t token) {
-#if !NDEBUG
-  bool in_queue = executed_.find(token) != executed_.end() ||
-                  completed_.find(token) != completed_.end() ||
-                  aborted_.find(token) != aborted_.end();
-  for (auto& task : pending_) {
-    if (token == task.token) {
-      in_queue = true;
-      break;
-    }
-  }
-  DCHECK(in_queue);
-#endif
-  return aborted_.find(token) != aborted_.end();
-}
-
 }  // namespace file_system_provider
 }  // namespace chromeos

chrome/browser/chromeos/file_system_provider/queue.h

@@ -24,19 +24,14 @@ namespace file_system_provider {
 // 1. Call NewToken() to obtain the token used bo all other methods.
 // 2. Call Enqueue() to enqueue the task.
 // 3. Call Complete() when the task is completed.
-// 4. Call Remove() to remove a completed task from the queue and run other
-//    enqueued tasks.
 //
-// Enqueued tasks can be aborted with Abort() at any time until they are marked
-// as completed or removed from the queue, as long as the task supports aborting
-// (it's abort callback is not NULL). Aorting does not remove the task from the
-// queue.
+// If the task supports aborting (it's abort callback is not NULL), then an
+// enqueued task can be aborted with Abort() at any time as long as the task is
+// not completed.
 //
-// In most cases you'll want to call Remove() and Complete() one after the
-// other. However, in some cases you may want to separate it. Eg. for limiting
-// number of opened files, you may want to call Complete() after opening is
-// completed, but Remove() after the file is closed. Note, that they can be
-// called at most once.
+// Once a task is executed, it must be marked as completed with Complete(). If
+// it's aborted before executing, no call to Complete() can happen. Simply
+// saying, just call Complete() from the completion callback of the task.
 class Queue {
  public:
   typedef base::Callback<AbortCallback(void)> AbortableCallback;
@@ -51,33 +46,20 @@ class Queue {
 
   // Enqueues a task using a token generated with NewToken(). The task will be
   // executed if there is space in the internal queue, otherwise it will wait
-  // until another task is finished. Once the task is finished, Complete() and
-  // Remove() must be called. The callback's abort callback may be NULL. In
-  // such case, Abort() must not be called.
+  // until another task is finished. Once the task is finished, Complete() must
+  // be called. The callback's abort callback may be NULL. In such case, Abort()
+  // must not be called.
   void Enqueue(size_t token, const AbortableCallback& callback);
 
   // Forcibly aborts a previously enqueued task. May be called at any time as
   // long as the task is still in the queue and is not marked as completed.
-  // Note, that Remove() must be called in order to remove the task from the
-  // queue. Must not be called if the task doesn't support aborting (it's
-  // abort callback is NULL).
   void Abort(size_t token);
 
-  // Returns true if the task which is in the queue with |token| has been
-  // aborted. This method must not be called for tasks which are not in the
-  // queue.
-  bool IsAborted(size_t token);
-
-  // Marks the previously enqueued task as complete. Must be called for each
-  // enqueued task (unless aborted). Note, that Remove() must be called in order
-  // to remove the task from the queue if it hasn't been aborted earlier.
-  // It must not be called more than one, nor for aborted tasks.
+  // Marks an executed task with |token| as completed. Must be called once the
+  // task is executed. Simply saying, in most cases it should be just called
+  // from the task's completion callback.
   void Complete(size_t token);
 
-  // Removes the previously enqueued and completed or aborted task from the
-  // queue. Must not be called more than once.
-  void Remove(size_t token);
-
  private:
   // Information about an enqueued task which hasn't been removed, nor aborted.
   struct Task {
@@ -98,8 +80,6 @@ class Queue {
   size_t next_token_;
   std::deque<Task> pending_;
   std::map<int, Task> executed_;
-  std::map<int, Task> completed_;
-  std::map<int, Task> aborted_;
 
   base::WeakPtrFactory<Queue> weak_ptr_factory_;
   DISALLOW_COPY_AND_ASSIGN(Queue);

chrome/browser/chromeos/file_system_provider/queue_unittest.cc

@@ -70,19 +70,11 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_OneAtOnce) {
   EXPECT_EQ(0, second_counter);
   EXPECT_EQ(0, second_abort_counter);
 
-  // Complete the first task, which should not run the second one, yet.
+  // Complete the first task from the queue should run the second task.
   queue.Complete(first_token);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, first_counter);
   EXPECT_EQ(0, first_abort_counter);
-  EXPECT_EQ(0, second_counter);
-  EXPECT_EQ(0, second_abort_counter);
-
-  // Removing the first task from the queue should run the second task.
-  queue.Remove(first_token);
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(1, first_counter);
-  EXPECT_EQ(0, first_abort_counter);
   EXPECT_EQ(1, second_counter);
   EXPECT_EQ(0, second_abort_counter);
 
@@ -103,7 +95,7 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_OneAtOnce) {
 
   // After aborting the second task, the third should run.
   queue.Abort(second_token);
-  queue.Remove(second_token);
+  queue.Complete(second_token);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, first_counter);
   EXPECT_EQ(0, first_abort_counter);
@@ -113,30 +105,6 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_OneAtOnce) {
   EXPECT_EQ(0, third_abort_counter);
 }
 
-TEST_F(FileSystemProviderQueueTest, Enqueue_WhilePreviousNotRemoved) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-  queue.Complete(first_token);
-
-  // Enqueuing a new task must not start it, once the queue is filled with a
-  // completed task.
-  const size_t second_token = queue.NewToken();
-  int second_counter = 0;
-  int second_abort_counter = 0;
-  queue.Enqueue(second_token,
-                base::Bind(&OnRun, &second_counter, &second_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-  EXPECT_EQ(0, second_counter);
-  EXPECT_EQ(0, second_abort_counter);
-}
-
 TEST_F(FileSystemProviderQueueTest, Enqueue_MultipleAtOnce) {
   Queue queue(2);
   const size_t first_token = queue.NewToken();
@@ -167,7 +135,6 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_MultipleAtOnce) {
 
   // Completing and removing the second task, should start the last one.
   queue.Complete(second_token);
-  queue.Remove(second_token);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, first_counter);
   EXPECT_EQ(0, first_abort_counter);
@@ -206,24 +173,10 @@ TEST_F(FileSystemProviderQueueTest, InvalidUsage_CompleteNotStarted) {
   // Completing and removing the first task, which however hasn't started.
   // That should not invoke the second task.
   EXPECT_DEATH(queue.Complete(first_token), "");
-  EXPECT_DEATH(queue.Remove(first_token), "");
-}
-
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_RemoveNotCompletedNorAborted) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-
-  // Remove before completing.
-  EXPECT_DEATH(queue.Remove(first_token), "");
 }
 
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_CompleteAfterAborting) {
+TEST_F(FileSystemProviderQueueTest,
+       InvalidUsage_CompleteAfterAbortingNonExecutedTask) {
   Queue queue(1);
   const size_t first_token = queue.NewToken();
   int first_counter = 0;
@@ -231,9 +184,6 @@ TEST_F(FileSystemProviderQueueTest, InvalidUsage_CompleteAfterAborting) {
   queue.Enqueue(first_token,
                 base::Bind(&OnRun, &first_counter, &first_abort_counter));
 
-  base::RunLoop().RunUntilIdle();
-
-  // Run, then abort.
   std::vector<base::File::Error> first_abort_callback_log;
   queue.Abort(first_token);
 
@@ -282,71 +232,6 @@ TEST_F(FileSystemProviderQueueTest, InvalidUsage_AbortTwice) {
   EXPECT_DEATH(queue.Abort(first_token), "");
 }
 
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_IsAbortedWhileNotInQueue) {
-  Queue queue(1);
-  EXPECT_DEATH(queue.IsAborted(1234), "");
-}
-
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_IsAbortedAfterRemoved) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-
-  queue.Abort(first_token);
-  queue.Remove(first_token);
-  EXPECT_DEATH(queue.IsAborted(first_token), "");
-}
-
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_RemoveTwice) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-
-  queue.Complete(first_token);
-  queue.Remove(first_token);
-  EXPECT_DEATH(queue.Complete(first_token), "");
-}
-
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_AbortAfterRemoving) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-
-  queue.Complete(first_token);
-  queue.Remove(first_token);
-  EXPECT_DEATH(queue.Abort(first_token), "");
-}
-
-TEST_F(FileSystemProviderQueueTest, InvalidUsage_CompleteAfterRemoving) {
-  Queue queue(1);
-  const size_t first_token = queue.NewToken();
-  int first_counter = 0;
-  int first_abort_counter = 0;
-  queue.Enqueue(first_token,
-                base::Bind(&OnRun, &first_counter, &first_abort_counter));
-
-  base::RunLoop().RunUntilIdle();
-
-  queue.Complete(first_token);
-  queue.Remove(first_token);
-  EXPECT_DEATH(queue.Complete(first_token), "");
-}
-
 TEST_F(FileSystemProviderQueueTest, InvalidUsage_AbortNonAbortable) {
   Queue queue(1);
   const size_t first_token = queue.NewToken();
@@ -383,17 +268,12 @@ TEST_F(FileSystemProviderQueueTest, Enqueue_Abort) {
   EXPECT_EQ(0, second_abort_counter);
 
   // Abort the first task while it's being executed.
-  EXPECT_FALSE(queue.IsAborted(first_token));
   queue.Abort(first_token);
-  EXPECT_TRUE(queue.IsAborted(first_token));
-  queue.Remove(first_token);
+  queue.Complete(first_token);
 
   // Abort the second task, before it's started.
   EXPECT_EQ(0, second_counter);
-  EXPECT_FALSE(queue.IsAborted(second_token));
   queue.Abort(second_token);
-  EXPECT_TRUE(queue.IsAborted(second_token));
-  queue.Remove(second_token);
 
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, first_counter);

chrome/browser/chromeos/file_system_provider/throttled_file_system.cc

@@ -195,19 +195,12 @@ void ThrottledFileSystem::OnOpenFileCompleted(int queue_token,
                                               const OpenFileCallback& callback,
                                               int file_handle,
                                               base::File::Error result) {
-  // The task may be aborted either via the callback, or by the operation, eg.
-  // because of destroying the request manager or unmounting the file system
-  // during the operation. Mark the task as completed only if it hasn't been
-  // aborted before.
-  if (!open_queue_->IsAborted(queue_token))
-    open_queue_->Complete(queue_token);
-
   // If the file is opened successfully then hold the queue token until the file
   // is closed.
   if (result == base::File::FILE_OK)
     opened_files_[file_handle] = queue_token;
   else
-    open_queue_->Remove(queue_token);
+    open_queue_->Complete(queue_token);
 
   callback.Run(file_handle, result);
 }
@@ -220,10 +213,10 @@ void ThrottledFileSystem::OnCloseFileCompleted(
   // closed on the C++ side. Release the task from the queue, so other files
   // which are enqueued can be opened.
   const auto it = opened_files_.find(file_handle);
-  CHECK(it != opened_files_.end());
+  DCHECK(it != opened_files_.end());
 
   const int queue_token = it->second;
-  open_queue_->Remove(queue_token);
+  open_queue_->Complete(queue_token);
   opened_files_.erase(file_handle);
 
   callback.Run(result);