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

components/offline_pages/core/task.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_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_TASK_H_
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/single_thread_task_runner.h"
+
+namespace offline_pages {
+
+// Task interface for consumers of the TaskQueue. Implements a mechanism for
+// task completion.
+//
+// To use TaskQueue:

[Pete Williamson, 2016/09/29 00:54:43]

Maybe this would be better as "To use a Task"
  * Derive your new task from offline_pages::Task
  * 
...

  * Have the calling code put the task on the TaskQueue using AddTask()

[fgorski, 2016/09/29 17:37:37]

Done.

+// * Implement your task with as many async operations on the controlled
+//   resource as is required. (In general the smaller the task the better.)
+// * Whenever the task is terminated, call |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 Task {
+ public:
+  typedef base::Callback<void(Task*)> CompletionCallback;

[dougarnett, 2016/09/29 15:40:04]

TaskCompletionCallback?

[fgorski, 2016/09/29 17:37:37]

Done.

+
+  Task();
+  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.

[Pete Williamson, 2016/09/29 00:54:44]

maybe add:
  // The task will have an additional function for each async step, with
  // each step setting up the next step as the completion callback for
  // the current step.

[fgorski, 2016/09/29 17:37:36]

Done.

+  virtual void Run() = 0;
+
+  // Sets the completion callback and whatever is set on it before the task is

[Pete Williamson, 2016/09/29 00:54:44]

We have lots of different kinds of completions - let's name them in such a way
as to be able to tell them apart easily
1. The callback to the Task owner when everything is done
2. The callback to the Task Queue when everything is done
3. The callback into the task object when a single async step completes.

It would be great to have visually different names for each of these concepts.

[fgorski, 2016/09/29 17:37:36]

I went with Doug's suggestion above and added to documentation. Let me know what
you think.

+  // put in the queue, will be overwritten. Setting the runner and callback here

[dougarnett, 2016/09/29 15:40:05]

wording seems a bit confusing wrt overwritten and before put in the queue. Does
it only take effect before it is put in the queue? Do you expect this to be used
multiple times for a task? Do you expect the Task to keep changing it as it
performs async operations?
[Don't need to answer those questions - just meant to show how the wording might
be confusing]

[fgorski, 2016/09/29 17:37:37]

Actually this raises a valid point. I'll add DCHECKs that it is only set once
and make a comment that it should only be set by TaskQueue.

+  // allows task owner to inject completion callback and specify appropriate

[Pete Williamson, 2016/09/29 00:54:44]

Naive user question: How does the Task client get a task runner for its current
thread?  Might be good to add that to comments here.

[fgorski, 2016/09/29 17:37:37]

That is actually left for the caller to work out. e.g.
base::ThreadTaskRunnerHandle::Get().
This has changed several times in the past, so I don't think it is worth
documenting that way.

+  // thread/runner on which to call it.
+  // If the task is run outside of the queue and completion callback is not
+  // set, it will also work.
+  // Left public for testing.
+  void SetCompletionCallback(scoped_refptr<base::SingleThreadTaskRunner> runner,
+                             const CompletionCallback& completion_callback);
+
+ protected:
+  // Call |Complete| at every point when the task is finished so that task queue

[dougarnett, 2016/09/29 15:40:05]

what does "at every point" mean? Is it finished more than once?

[fgorski, 2016/09/29 17:37:36]

Attempted clarification. Let me know.

+  // 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();

[Pete Williamson, 2016/09/29 00:54:44]

Maybe TaskComplete() to differentiate it a bit more visually from the completion
callback?

[fgorski, 2016/09/29 17:37:37]

Done.

+
+ private:
+  // Completion callback for this task set by |SetCompletionCallback|.
+  CompletionCallback completion_callback_;
+  // Task runner for calling completion callback.
+  scoped_refptr<base::SingleThreadTaskRunner> completion_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(Task);

[Pete Williamson, 2016/09/29 00:54:43]

You've disallowed copy and assign, should you also disallow move ctr and move
assign?

[fgorski, 2016/09/29 17:37:36]

Cool. It got me thinking.

I don't think that is commonly done and I couldn't find a macro. Also notice
that these operation don't create a new copy of a task, which is what this macro
is for. The way task is used (enforced by TaskQueue interface) is through a
unique_ptr, which provides the move semantics explicitly.

Please, correct me if my understanding is incorrect.

+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_TASK_H_