cc: Simplify Task and its derived classes.

cc/raster/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 CC_RASTER_TASK_H_
+#define CC_RASTER_TASK_H_
+
+#include <stdint.h>
+
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "cc/base/cc_export.h"
+#include "cc/raster/task_types.h"
+
+namespace cc {
+
+// A task which can be run by a TaskGraphRunner. To run a Task, it should be
+// inserted into a TaskGraph, which can then be scheduled on the
+// TaskGraphRunner.
+class CC_EXPORT Task : public base::RefCountedThreadSafe<Task> {
+ public:
+  typedef std::vector<scoped_refptr<Task>> Vector;
+
+  TaskType GetTaskType();

[reveman, 2016/04/16 10:10:28]

can this be private to tilemanager's task implementations?

[prashant.n, 2016/04/17 17:37:48]

Hmm. As TileManager only schedules the type of tasks, we can delimit the task
type to tile manager, but the derived task classes are spread in different
files, like TileManager.cc, SoftwareImageDecodeController.cc and
GpuImageDecodeController.cc.

So IMO, keeping task type generic would help scalability. 

+
+  // Indicates whether this task can be run at the same time as other tasks in
+  // the task graph. If false, this task will be scheduled with
+  // TASK_CATEGORY_NONCONCURRENT_FOREGROUND. The base implementation always
+  // returns true.
+  virtual bool SupportsConcurrentExecution() const;

[reveman, 2016/04/16 10:10:28]

this should preferably be private to tilemanager's task implementation too.

[prashant.n, 2016/04/17 17:37:48]

Making this generic would save having one more hierarchy of derivation

which would simplify task classes and their usage.

+
+  const Task::Vector& dependencies() const { return dependencies_; }

[reveman, 2016/04/16 10:10:28]

and this

[prashant.n, 2016/04/17 17:37:48]

Having dependencies kept in Task, would help TileManager manage tasks without
knowing their implementations or actual classes e.g. (ImageDecodeTaskImpl).

Earlier as it was being used only in RasterTasks, it was TileManager specific
thing, but now image upload task uses dependency. So thought of keeping generic.

In such case, we might need to keep TileTask to make above things private to
TileManager and move out of Task.

TileTask : Task

RasterTaskImpl : TileTask
ImageDecodeTaskImpl : TileTask
ImageUploadTaskImpl : TileTask

But this would again complicate things as Task is cc/ specific only. OR

We can have TaskTraits, where we can keep above things or have derivations in
those.

Task {
Task(TaskTraits t)

TaskTraits traits_;
}


TileTaskTraits : TaskTraits {
// dependencies
// concurrent execution supported
}

Kindly let me know your opinions.
 

[reveman, 2016/04/17 20:08:23]

Task class should not contain anything but what's needed to implement the
TaskGraphRunner interface. All the properties above are tile manager specific
and should be kept so. A derived TileTask is probably what we want.

+
+  // Subclasses should implement this method. RunOnWorkerThread may be called
+  // on any thread, and subclasses are responsible for locking and thread
+  // safety.
+  virtual void RunOnWorkerThread() = 0;
+
+  void WillRun();
+  void DidRun();
+  bool HasFinishedRunning() const;

[prashant.n, 2016/04/17 17:37:48]

Basically Task should expose only HasFinishedRunning()
WillRun(), DidRun() should be private to TaskGraphRunner, which I was thinking
to patch up subsequently after this patch.

+
+  void DidComplete();
+  bool HasCompleted() const;
+
+ protected:
+  friend class base::RefCountedThreadSafe<Task>;
+
+  void SetTaskType(TaskType type);
+
+  Task();
+  explicit Task(Task::Vector* dependencies);
+  virtual ~Task();
+
+  Task::Vector dependencies_;
+  TaskType type_;

[reveman, 2016/04/16 10:10:28]

private to tilemanager's implementation would mean removing these from here too.

+  bool will_run_;
+  bool did_run_;
+  bool did_complete_;
+};
+
+}  // namespace cc
+
+#endif  // CC_RASTER_TASK_H_