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

components/offline_pages/core/task_queue.cc

Index: components/offline_pages/core/task_queue.cc
diff --git a/components/offline_pages/core/task_queue.cc b/components/offline_pages/core/task_queue.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4f3b3a50151637f5e04facbfab750636ec1b1aa7
--- /dev/null
+++ b/components/offline_pages/core/task_queue.cc
@@ -0,0 +1,70 @@
+// 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.
+
+#include "components/offline_pages/core/task_queue.h"
+
+#include "base/bind.h"
+#include "base/threading/thread_task_runner_handle.h"
+
+namespace offline_pages {
+
+TaskQueue::Task::~Task() {}
+
+void TaskQueue::Task::SetCompletionCallback(

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

Why does this set both the runner and the callback?  Maybe it should be called
SetRunnerAndCallback()?

I also wonder why we need a runner instead of the task queue using its own
thread, more documentation might help.

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

Because we want the completion to be executed on the right thread and outside of
the call stack. I updated the SetCompletionCallback documentation in the header.

+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const TaskQueue::CompletionCallback& callback) {
+  DCHECK(runner);
+  DCHECK(!callback.is_null());
+  completion_runner_ = runner;
+  completion_callback_ = callback;
+}
+
+void TaskQueue::Task::Complete() {
+  if (completion_callback_.is_null() || !completion_runner_)
+    return;
+
+  completion_runner_->PostTask(FROM_HERE,
+                               base::Bind(completion_callback_, this));
+}
+
+TaskQueue::TaskQueue() : weak_ptr_factory_(this) {}
+
+TaskQueue::~TaskQueue() {}
+
+void TaskQueue::AddTask(std::unique_ptr<Task> task) {
+  task->SetCompletionCallback(
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&TaskQueue::TaskCompleted, weak_ptr_factory_.GetWeakPtr()));
+  tasks_.push(std::move(task));

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

Should this be persistent?  I'm guessing not, but I wanted to make sure we
thought about it.

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

You are correct. It shouldn't be persisted. This is completely in memory
serialization of execution.

+  MaybeStartTask();
+}
+
+bool TaskQueue::HasTasks() const {
+  return !tasks_.empty() || CurrentlyRunning();
+}
+
+bool TaskQueue::CurrentlyRunning() const {
+  return current_task_.get() != nullptr;
+}
+
+void TaskQueue::MaybeStartTask() {
+  if (CurrentlyRunning())
+    return;
+  if (!HasTasks())
+    return;
+
+  current_task_ = std::move(tasks_.front());
+  tasks_.pop();
+  current_task_->Run();
+}
+
+void TaskQueue::TaskCompleted(Task* task) {
+  DCHECK_EQ(task, current_task_.get());

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

This doesn't seem to call the completion for the task.  If not here, where does
it get called?

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

line 38

+  if (task == current_task_.get()) {
+    current_task_.reset(nullptr);
+    MaybeStartTask();
+  }
+}
+
+}  // namespace offline_pages