cc: Move internal::Task::did_schedule_ to internal::WorkerPoolTask.

cc: Move internal::Task::did_schedule_ to internal::WorkerPoolTask.

This moves the task scheduling notion out of the TaskGraphRunner and
into the RasterWorkerPool. This is a more appropriate place to handle
this and required as tasks need to complete multiple times when used
with PixelRasterWorkerPool.

BUG=339201
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249236

[vmpstr, 2014-02-05 16:56:23]

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:635: internal::WorkerPoolTask* task =
Can you do a DCHECK here to ensure that this task isn't done yet? Something
along the lines of !HasFinishedRunning or !HasBeenScheduled.

My concern is that will/did schedule indicates that this task will eventually
run, is it possible for the task graph runner to not do that for whatever
reason?

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:666: task->WillSchedule();
Does this also need a ScheduleOnOriginThread? There's a DCHECK in the task that
it's not a gpu rasterization task, but I'm wondering whether that DCHECK should
be an early out instead? I mean it kind of makes sense to me that all tasks do a
WillSchedule/Schedule/DidSchedule cycle instead of special casing GPU tasks to
not Schedule. Wdyt?

I was also thinking of having two ScheduleOnOriginThreadForWorkerThread and
ScheduleOnOriginThreadForOriginThread kind of thing, but I think that's
overkill.

[reveman, 2014-02-05 17:59:59]

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:635: internal::WorkerPoolTask* task =
On 2014/02/05 16:56:23, vmpstr wrote:
> Can you do a DCHECK here to ensure that this task isn't done yet? Something
> along the lines of !HasFinishedRunning or !HasBeenScheduled.

The task might have finished running. We can't know that here. Schedule can be
called multiple times before CompleteOnOriginThread is called. Maybe we should
reset did_schedule_ in DidComplete() and only call Schedule here if
!HasBeenScheduled. That would make life a bit easier for the task
implementation. Wdyt?
> 
> My concern is that will/did schedule indicates that this task will eventually
> run, is it possible for the task graph runner to not do that for whatever
> reason?

It could be canceled if the next call to SetTaskGraph doesn't include this task.

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:666: task->WillSchedule();
On 2014/02/05 16:56:23, vmpstr wrote:
> Does this also need a ScheduleOnOriginThread? There's a DCHECK in the task
that
> it's not a gpu rasterization task, but I'm wondering whether that DCHECK
should
> be an early out instead? I mean it kind of makes sense to me that all tasks do
a
> WillSchedule/Schedule/DidSchedule cycle instead of special casing GPU tasks to
> not Schedule. Wdyt?

I agree. Did the same for CompleteOnOriginThread().

> 
> I was also thinking of having two ScheduleOnOriginThreadForWorkerThread and
> ScheduleOnOriginThreadForOriginThread kind of thing, but I think that's
> overkill.

[vmpstr, 2014-02-05 18:07:42]

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:635: internal::WorkerPoolTask* task =
On 2014/02/05 18:00:00, David Reveman wrote:
> On 2014/02/05 16:56:23, vmpstr wrote:
> > Can you do a DCHECK here to ensure that this task isn't done yet? Something
> > along the lines of !HasFinishedRunning or !HasBeenScheduled.
> 
> The task might have finished running. We can't know that here. Schedule can be
> called multiple times before CompleteOnOriginThread is called. Maybe we should
> reset did_schedule_ in DidComplete() and only call Schedule here if
> !HasBeenScheduled. That would make life a bit easier for the task
> implementation. Wdyt?

I think that makes sense. From the task perspective then it's scheduled once
and it will just keep running until it completes. The fact that it is scheduled
multiple times should be opaque to the task. I mean if it is scheduled once
and run multiple times should be the same as if it goes through multiple
schedule/run cycles from the task perspective.

> > 
> > My concern is that will/did schedule indicates that this task will
eventually
> > run, is it possible for the task graph runner to not do that for whatever
> > reason?
> 
> It could be canceled if the next call to SetTaskGraph doesn't include this
task.

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:666: task->WillSchedule();
On 2014/02/05 18:00:00, David Reveman wrote:
> On 2014/02/05 16:56:23, vmpstr wrote:
> > Does this also need a ScheduleOnOriginThread? There's a DCHECK in the task
> that
> > it's not a gpu rasterization task, but I'm wondering whether that DCHECK
> should
> > be an early out instead? I mean it kind of makes sense to me that all tasks
do
> a
> > WillSchedule/Schedule/DidSchedule cycle instead of special casing GPU tasks
to
> > not Schedule. Wdyt?
> 
> I agree. Did the same for CompleteOnOriginThread().
> 
> > 
> > I was also thinking of having two ScheduleOnOriginThreadForWorkerThread and
> > ScheduleOnOriginThreadForOriginThread kind of thing, but I think that's
> > overkill.
> 

Thanks! I think it looks nice now (ie symmetrical with the way other tasks are
run).

[reveman, 2014-02-05 19:52:41]

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/140333006/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:635: internal::WorkerPoolTask* task =
On 2014/02/05 18:07:42, vmpstr wrote:
> On 2014/02/05 18:00:00, David Reveman wrote:
> > On 2014/02/05 16:56:23, vmpstr wrote:
> > > Can you do a DCHECK here to ensure that this task isn't done yet?
Something
> > > along the lines of !HasFinishedRunning or !HasBeenScheduled.
> > 
> > The task might have finished running. We can't know that here. Schedule can
be
> > called multiple times before CompleteOnOriginThread is called. Maybe we
should
> > reset did_schedule_ in DidComplete() and only call Schedule here if
> > !HasBeenScheduled. That would make life a bit easier for the task
> > implementation. Wdyt?
> 
> I think that makes sense. From the task perspective then it's scheduled once
> and it will just keep running until it completes. The fact that it is
scheduled
> multiple times should be opaque to the task. I mean if it is scheduled once
> and run multiple times should be the same as if it goes through multiple
> schedule/run cycles from the task perspective.

Take a look at latest patch.

> 
> > > 
> > > My concern is that will/did schedule indicates that this task will
> eventually
> > > run, is it possible for the task graph runner to not do that for whatever
> > > reason?
> > 
> > It could be canceled if the next call to SetTaskGraph doesn't include this
> task.
>

[vmpstr, 2014-02-05 19:55:32]

lgtm

[reveman, 2014-02-05 20:33:03]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-05 20:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/140333006/320001

[reveman, 2014-02-05 21:47:33]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2014-02-05 21:47:50]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-05 22:09:33]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-02-05 22:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/140333006/540001

[commit-bot: I haz the power, 2014-02-06 02:34:17]

Change committed as 249236