cc: Merge TileTaskRunner to TileTaskWorkerPool.

cc: Merge TileTaskRunner to TileTaskWorkerPool.

This patches merges all interface functions of TileTaskRunner to
TileTaskWorkerPool.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6cb1bb8c566eedf7c10ee1a367d83c2a6ac6a898
Cr-Commit-Position: refs/heads/master@{#388430}

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

Description was changed from

==========
cc: Merge TileTaskRunner to TileTaskWorkerPool.

This patches merges all interface functions of TileTaskRunner to
TileTaskWorkerPool.

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

to

==========
cc: Merge TileTaskRunner to TileTaskWorkerPool.

This patches merges all interface functions of TileTaskRunner to
TileTaskWorkerPool.

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

[prashant.n, 2016-04-14 06:34:53]

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

[prashant.n, 2016-04-14 06:34:53]

ptal.

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

ptal - patch #4.

[prashant.n, 2016-04-17 18:30:07]

ping

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

ptal.

[prashant.n, 2016-04-19 10:28:40]

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

[commit-bot: I haz the power, 2016-04-19 10:28:54]

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

[commit-bot: I haz the power, 2016-04-19 11:33:28]

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

[commit-bot: I haz the power, 2016-04-19 11:33:29]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-04-20 00:22:29]

LGTM

Nice refactor - a couple comments, but overall looks good.

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.cc
File cc/raster/tile_task.cc (left):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.cc#...
cc/raster/tile_task.cc:18: dependencies_(std::move(*dependencies)),
should we std::swap(dependencies_, dependencies) to avoid returning an object in
an unspecified state?

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h
File cc/raster/tile_task.h (right):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h#n...
cc/raster/tile_task.h:8: #include <stdint.h>
is anything using stdint.h? Same for other includes - callback/resource_format
may be unused?

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h#n...
cc/raster/tile_task.h:27: // TASK_CATEGORY_NONCONCURRENT_FOREGROUND. The base
implementation always
nit: remove the comment "The base implementation always returns true." This is
no longer the case after your previous change.

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h#n...
cc/raster/tile_task.h:29: bool SupportsConcurrentExecution() const {
Per David's comment on crrev.com/1890903002, please change this to
supports_concurrent_execution(), now that it is a simple accessor.

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool.h (right):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool.h:27: class CC_EXPORT TileTaskWorkerPool {
I wonder if this should be called TileTaskRunner, and we should just get rid of
the name TileTaskWorkerPool - runner seems more descriptive of what the
interface does - WDYT? Either way, feel free to deal with this in a separate CL.

[prashant.n, 2016-04-20 02:04:48]

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.cc
File cc/raster/tile_task.cc (left):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.cc#...
cc/raster/tile_task.cc:18: dependencies_(std::move(*dependencies)),
On 2016/04/20 00:22:29, ericrk wrote:
> should we std::swap(dependencies_, dependencies) to avoid returning an object
in
> an unspecified state?

I guess as we are passing vector object created on stack in function, it shd be
okay. std::move would be faster.

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h
File cc/raster/tile_task.h (right):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h#n...
cc/raster/tile_task.h:8: #include <stdint.h>
On 2016/04/20 00:22:29, ericrk wrote:
> is anything using stdint.h? Same for other includes - callback/resource_format
> may be unused?

Yes I'll remove callback/resource_format after checking, while changing patch
dependency I missed it.

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task.h#n...
cc/raster/tile_task.h:29: bool SupportsConcurrentExecution() const {
On 2016/04/20 00:22:29, ericrk wrote:
> Per David's comment on crrev.com/1890903002, please change this to
> supports_concurrent_execution(), now that it is a simple accessor.

Now I understand why few functions do not use camel case. :)

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool.h (right):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool.h:27: class CC_EXPORT TileTaskWorkerPool {
On 2016/04/20 00:22:29, ericrk wrote:
> I wonder if this should be called TileTaskRunner, and we should just get rid
of
> the name TileTaskWorkerPool - runner seems more descriptive of what the
> interface does - WDYT? Either way, feel free to deal with this in a separate
CL.

I was thinking to call this as TileTaskController or just TileTaskWorker as it
is raster buffer provider + wrapper over tile graph runner.

[reveman, 2016-04-20 02:24:44]

lgtm but we should really remove the TileTaskWorkerPool classes completely in
favor of just having a few different RasterBufferProvider implementations.
that's fine as a follow up though

[prashant.n, 2016-04-20 04:00:42]

On 2016/04/20 02:24:44, reveman wrote:
> lgtm but we should really remove the TileTaskWorkerPool classes completely in
> favor of just having a few different RasterBufferProvider implementations.
> that's fine as a follow up though

Yes, I'll remove those once we remove schedule/complete functions. (pending
https://codereview.chromium.org/1866043006/)

My idea is

Split TileManager into TileManager (handling only tile related stuff) and
tile_task_scheduler (handling task related stuff - move all task related stuff
(e.g. InsertNodeForTask, CreateRasterTask, RasterTaskImpl, etc.) from
TileManager to this new file)

TileManager {
tile_task_scheduler
}

TileTaskScheduler {

task_graph_runner -> { single threaded, raster worker pool, sync }
raster_buffer_provider -> { 0, 1, gpu, bitmap}

}

[prashant.n, 2016-04-20 04:16:12]

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool.h (right):

https://codereview.chromium.org/1888713002/diff/20001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool.h:27: class CC_EXPORT TileTaskWorkerPool {
As David suggested, we should get rid of TileTaskWorkerPool and have different
implementations of RasterBufferProviders.

[prashant.n, 2016-04-20 04:16:32]

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

[prashant.n, 2016-04-20 04:16:32]

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

[commit-bot: I haz the power, 2016-04-20 04:16:40]

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

[commit-bot: I haz the power, 2016-04-20 05:16:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:21:01]

Description was changed from

==========
cc: Merge TileTaskRunner to TileTaskWorkerPool.

This patches merges all interface functions of TileTaskRunner to
TileTaskWorkerPool.

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

to

==========
cc: Merge TileTaskRunner to TileTaskWorkerPool.

This patches merges all interface functions of TileTaskRunner to
TileTaskWorkerPool.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6cb1bb8c566eedf7c10ee1a367d83c2a6ac6a898
Cr-Commit-Position: refs/heads/master@{#388430}
==========

[commit-bot: I haz the power, 2016-04-22 19:21:02]

Patchset 3 (id:??) landed as
https://crrev.com/6cb1bb8c566eedf7c10ee1a367d83c2a6ac6a898
Cr-Commit-Position: refs/heads/master@{#388430}