cc: Simplify Task and its derived classes.

cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/49b3e6465bc0781cffa00374219e97e0348b51bd
Cr-Commit-Position: refs/heads/master@{#388152}

[prashant.n, 2016-04-14 16:52:17]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. All the implementation classes like
RasterTaskImpl, ImageDecodeTaskImpl, etc. are derived directly from
Task.

BUG=599863
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. All the implementation classes like
RasterTaskImpl, ImageDecodeTaskImpl, etc. are derived directly from
Task.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-14 16:53:56]

prashant.n@samsung.com changed reviewers:
+ ericrk@chromium.org, reveman@chromium.org, vmpstr@chromium.org

[prashant.n, 2016-04-14 16:53:56]

WIP patch simplifying Task class.

[prashant.n, 2016-04-15 06:25:14]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. All the implementation classes like
RasterTaskImpl, ImageDecodeTaskImpl, etc. are derived directly from
Task.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-15 06:29:00]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

Task requiring specific handling (e.g. processing completed task in
different manner) needs to be defined in task_types.h. With TaskType
they can be differenciated. Task not specifying the TaskType gets
created with TASK_TYPE_DEFAULT.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-15 06:30:39]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

Task requiring specific handling (e.g. processing completed task in
different manner) needs to be defined in task_types.h. With TaskType
they can be differenciated. Task not specifying the TaskType gets
created with TASK_TYPE_DEFAULT.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

Task requiring specific handling (e.g. processing completed task in
different manner) needs to be declared in task_types.h. With TaskType
they can be differenciated. Task not specifying the TaskType gets
created with TASK_TYPE_DEFAULT.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-15 06:33:58]

PTAL.

[prashant.n, 2016-04-15 07:06:23]

ptal - patch #5.

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

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode25
cc/raster/task.h:25: TaskType GetTaskType();
can this be private to tilemanager's task implementations?

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode31
cc/raster/task.h:31: virtual bool SupportsConcurrentExecution() const;
this should preferably be private to tilemanager's task implementation too.

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode33
cc/raster/task.h:33: const Task::Vector& dependencies() const { return
dependencies_; }
and this

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode57
cc/raster/task.h:57: TaskType type_;
private to tilemanager's implementation would mean removing these from here too.

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task_types.h
File cc/raster/task_types.h (right):

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task_types.h#...
cc/raster/task_types.h:21: };
can we move this into tile_manager.cc or tile_manager.h?

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

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode25
cc/raster/task.h:25: TaskType GetTaskType();
On 2016/04/16 10:10:28, reveman wrote:
> can this be private to tilemanager's task implementations?

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.

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode31
cc/raster/task.h:31: virtual bool SupportsConcurrentExecution() const;
Making this generic would save having one more hierarchy of derivation

which would simplify task classes and their usage.

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode33
cc/raster/task.h:33: const Task::Vector& dependencies() const { return
dependencies_; }
On 2016/04/16 10:10:28, reveman wrote:
> and this

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.

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode42
cc/raster/task.h:42: bool HasFinishedRunning() const;
Basically Task should expose only HasFinishedRunning()
WillRun(), DidRun() should be private to TaskGraphRunner, which I was thinking
to patch up subsequently after this patch.

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

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1890903002/diff/40001/cc/raster/task.h#newcode33
cc/raster/task.h:33: const Task::Vector& dependencies() const { return
dependencies_; }
On 2016/04/17 at 17:37:48, prashant.n wrote:
> On 2016/04/16 10:10:28, reveman wrote:
> > and this
> 
> 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.

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.

[prashant.n, 2016-04-18 13:08:07]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies and
is moved to its own file. Now tasks have simple one level hierarchy
like every task gets derived directly from Task class.

  RasterTaskImpl : Task
  ImageDecodeTaskImpl : Task
  ImageUploadTaskImpl : Task

Task requiring specific handling (e.g. processing completed task in
different manner) needs to be declared in task_types.h. With TaskType
they can be differenciated. Task not specifying the TaskType gets
created with TASK_TYPE_DEFAULT.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-04-18 13:19:01]

PTAL.

[prashant.n, 2016-04-18 13:22:54]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 13:23:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890903002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890903002/80001

[commit-bot: I haz the power, 2016-04-18 14:24:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 14:24:37]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-04-19 00:05:30]

lg, just one minor thing

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
cc/raster/tile_task_runner.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
Note: I think we can rename this file to tile_task.h after removing the
TileTaskRunner classes in favor of just having a few different
RasterBufferProvider implementations

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
const;
can this be a implemented using a "const bool supports_concurrent_execution_;"
member instead that is set by the ctor? and exposed using a simple
supports_concurrent_execution() getter..

[prashant.n, 2016-04-19 01:19:02]

Would you pls. lgtm in advance, so that I'll finish this patch by doing needed
modifications?

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
cc/raster/tile_task_runner.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2016/04/19 00:05:30, reveman wrote:
> Note: I think we can rename this file to tile_task.h after removing the
> TileTaskRunner classes in favor of just having a few different
> RasterBufferProvider implementations

Yes. I've a patch which merges TileTaskRunner interface to TileTaskWorkerPool,
to ttwp as a wrapper over TaskGraphRunner.
(https://codereview.chromium.org/1888713002/)

In that patch, I'll rename this filename to tile_task.h.

https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
const;
On 2016/04/19 00:05:30, reveman wrote:
> can this be a implemented using a "const bool supports_concurrent_execution_;"
> member instead that is set by the ctor? and exposed using a simple
> supports_concurrent_execution() getter..

Yes.

[prashant.n, 2016-04-19 04:04:06]

>
https://codereview.chromium.org/1890903002/diff/80001/cc/raster/tile_task_run...
> cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
> const;
> On 2016/04/19 00:05:30, reveman wrote:
> > can this be a implemented using a "const bool
supports_concurrent_execution_;"
> > member instead that is set by the ctor? and exposed using a simple
> > supports_concurrent_execution() getter..
> 
> Yes.

PTAL.

[prashant.n, 2016-04-19 04:06:02]

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:43: explicit TileTask(bool
supports_concurrent_execution);
Here we cannot prevent implicit cast of pointer to bool :(, if somebody
mistakenly passes that.

[prashant.n, 2016-04-19 04:38:23]

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
const;
OR we can have pure virtual function to make derived classes explicitly set this
property.

[reveman, 2016-04-19 05:02:16]

lgtm after removing the virtual function

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/task.h#newcode1
cc/raster/task.h:1: // Copyright 2016 The Chromium Authors. All rights reserved.
nit: maybe it makes more sense for this file to be named task_graph.h but either
is fine with me

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
const;
On 2016/04/19 at 04:38:23, prashant.n wrote:
> OR we can have pure virtual function to make derived classes explicitly set
this property.

The point of adding the member was to remove the need for a virtual function.
ie.

bool supports_concurrent_execution() const { return
supports_concurrent_execution_; }

[prashant.n, 2016-04-19 05:28:07]

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/task.h#newcode1
cc/raster/task.h:1: // Copyright 2016 The Chromium Authors. All rights reserved.
On 2016/04/19 05:02:16, reveman wrote:
> nit: maybe it makes more sense for this file to be named task_graph.h but
either
> is fine with me
I too was in dilemma which name to be chosen, but then thought having simple
name - task.h, so that we can have tile_task.h for derived category.

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/100001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:29: virtual bool SupportsConcurrentExecution()
const;
Aaah. I missed to remove virtual keyword.

I took it as - Providing ctor to define supports_concurrent_execution_ would
also mandate subclasses to specify this explicitly. So there are two ways -
either currently implemented OR pure virtual function.

I'll remove virtual keyword.

[prashant.n, 2016-04-19 05:48:12]

The CQ bit was checked by prashant.n@samsung.com

[prashant.n, 2016-04-19 05:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1890903002/#ps120001
(title: "feedback")

[commit-bot: I haz the power, 2016-04-19 05:48:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890903002/120001

[commit-bot: I haz the power, 2016-04-19 07:04:57]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-19 07:04:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-19 07:06:10]

Description was changed from

==========
cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Simplify Task and its derived classes.

This patch merges ImageDecodeTask and RasterTask into TileTask, single
class defining properties specific to tasks used for tiles. Now all
the tile related tasks get derived directly from TileTask.

  RasterTaskImpl : TileTask
  ImageDecodeTaskImpl : TileTask
  ImageUploadTaskImpl : TileTask

This patch also splits (Task + TaskGraph) from TaskGraphRunner and
moves it to its own file which simplifies file include dependencies.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/49b3e6465bc0781cffa00374219e97e0348b51bd
Cr-Commit-Position: refs/heads/master@{#388152}
==========

[commit-bot: I haz the power, 2016-04-19 07:06:12]

Patchset 7 (id:??) landed as
https://crrev.com/49b3e6465bc0781cffa00374219e97e0348b51bd
Cr-Commit-Position: refs/heads/master@{#388152}

[reveman, 2016-04-19 07:08:17]

https://codereview.chromium.org/1890903002/diff/120001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1890903002/diff/120001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:29: bool SupportsConcurrentExecution() const {
This should not be CamelCase as a simple getter. See the style guide. It should
be "supports_concurrent_execution() const". Please fix asap.