[Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore

components/offline_pages/core/task_queue.h

+// Copyright 2016 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_TASK_QUEUE_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_TASK_QUEUE_H_
+
+#include <stdint.h>
+
+#include <memory>
+#include <queue>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+
+namespace offline_pages {
+
+// Class for coordinating sets of asynchronous operations, which switch threads
+// and put multiple tasks in a message loop, with the goal to only run a single
+// operation like that at once and postpone all the other work until the current
+// task is finished.
+//
+// Consumers of this class should create an instance of TaskQueue and implement
+// the tasks, that need to be run sequentially. New task will only be started,

[Pete Williamson, 2016/09/27 23:49:47]

nit - clearer without the comma after "started"

[fgorski, 2016/09/28 22:19:28]

Done.

+// when the previous one calls complete.
+//
+// If there is a chance that a task callback will come after the task is
+// destroyed, it is up to the task to actually implement mechanism to deal with
+// that, such as using a base::WeakPtrFactory.
+class TaskQueue {
+ public:
+  class Task;
+
+  typedef base::Callback<void(Task*)> CompletionCallback;
+
+  // Task interface for consumers of the TaskQueue. Implements a mechanism for
+  // task completion.
+  class Task {

[Pete Williamson, 2016/09/27 23:49:47]

Would it make the code less complicated to move task into its own file?

Maybe more docs about how to use this:  (Derive a calss from Task that
implements your logic, add that to the TaskQueue, set it up for a runner, have
task states that follow on eachother, use the same queue for all tasks using the
same resouorce (such as the request queue).

[fgorski, 2016/09/28 22:19:29]

This is mixing together task implementation guidance, task queue handling, and
stuff that is already delivered by the task/task_queue implementations.

I moved the class and added comments. We can refine it from there, if we learn a
bit
more about that.

+   public:
+    virtual ~Task();
+
+    // Entry point to the task. This is used by the queue to start the task, and
+    // first step of the task should be implemented by overloading this method.
+    // TODO(fgorski): Consider alternative: protected RunImpl(), so that we can
+    // add things like UMA in the Run method.
+    virtual void Run() = 0;
+
+    // Sets the completion callback and whatever is set on it before the task is
+    // put in the queue, will be overwritten. Left public for testing.
+    // If the task is run outside of the queue and completion callback is not
+    // set, it will also work.
+    void SetCompletionCallback(
+        scoped_refptr<base::SingleThreadTaskRunner> runner,
+        const CompletionCallback& completion_callback);
+
+   protected:
+    // Complete call, which should be made at every point, where the task is

[Pete Williamson, 2016/09/27 23:49:47]

nit - clearer without comma.

[fgorski, 2016/09/28 22:19:28]

Done.

+    // finished, so that task queue can pick up another task.
+    // |completion_callback_| will be scheduled on the provided
+    // |completion_runner_|, which means task code is no longer going to be on
+    // stack, when the next call is made.
+    void Complete();
+
+   private:
+    // Completion callback for this task set by |SetCompletionCallback|.
+    CompletionCallback completion_callback_;
+    // Task runner for calling completion callback.
+    scoped_refptr<base::SingleThreadTaskRunner> completion_runner_;
+  };
+
+  TaskQueue();
+  ~TaskQueue();
+
+  // Adds a task to the queue. Queue takes ownership of the task.
+  void AddTask(std::unique_ptr<Task> task);
+  // Whether the task queue has any tasks (running or waiting).
+  bool HasTasks() const;
+  // Whether there is a task currently running. Used for testing.
+  bool CurrentlyRunning() const;
+
+ private:
+  // Checks whether there are any tasks to run, as well as whether no task is
+  // currently running. When both are met, it will start the next task in the
+  // queue.
+  void MaybeStartTask();
+
+  // Callback for informing the queue that a task was completed.
+  void TaskCompleted(Task* task);
+
+  // Currently running tasks.
+  std::unique_ptr<Task> current_task_;
+
+  // A FIFO queue of tasks that will be run using this task queue.
+  std::queue<std::unique_ptr<Task>> tasks_;

[Pete Williamson, 2016/09/27 23:49:47]

Is FIFO the right policy?  For example, if we aborted a request, deleting the
queue entry, maybe that should happen before an update to the same request.

[fgorski, 2016/09/28 22:19:29]

RC has full control over in what order the tasks are called.

+
+  base::WeakPtrFactory<TaskQueue> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskQueue);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_TASK_QUEUE_H_