cc: Add new RasterWorkerPool interface.

cc: Add new RasterWorkerPool interface.

Refactor RasterWorkerPool interface such that it can be used
to implement different type of resource updates mechanisms.
The result is that the tile manager is no longer aware of
the need to perform uploads. This cleans up the tile manager
code significantly and allow zero copy resource updates to
be properly supported.

BUG=241644
TEST=cc_unittest --gtest_filter=BasicRasterWorkerPoolTest.*

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

[reveman, 2013-05-29 07:41:38]

This is still WIP so you might want to hold off on the reviews for another day.
However, it should give you a good idea how 0-copy will work in the tile manager
and should be good enough for testing purposes once I've re-landed the
cancellation patch.

[vmpstr, 2013-05-29 16:55:03]

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:13: class RootWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
EmptyWorkerPoolTaskImpl? Also it might be worth making this a
RasterWorkerPool::(Root/Empty)TaskImpl to reduce code repetition. I don't think
it's too terrible if this is exposed outside of anonymous namespace.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/pixel_buffer_...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:132: // Transfer activate pixel
buffer tasks to |new_pixel_buffer_tasks|
activate -> active

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:393: if (did_initialize_visible_tile_) {
This block of code repeats in at least one other spot. Maybe it's worth making
something like "NotifyClientOfVisibleTileInitialization" and call that here and
wherever the code appears

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:724: tile->tile_version().memory_state_ =
CAN_USE_MEMORY;
Why CAN_USE_MEMORY? set_solid_color sets it to NOT_ALLOWED_TO_USE_MEMORY.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:728: tile->tile_version().resource_ =
resource.Pass();
Do you have to set the resource_id_ as well? I can't really find where that's
set. Also, what's the reasoning for separating the two? It seems that you now
have to maintain two items that should(?) be related

[reveman, 2013-05-29 19:39:11]

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:13: class RootWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
On 2013/05/29 16:55:03, vmpstr wrote:
> EmptyWorkerPoolTaskImpl? Also it might be worth making this a
> RasterWorkerPool::(Root/Empty)TaskImpl to reduce code repetition. I don't
think
> it's too terrible if this is exposed outside of anonymous namespace.

Maybe we just make internal::WorkerPoolTask a full implementation by adding
default implementations of RunOnThread and DispatchCompletionCallback like
below?

https://codereview.chromium.org/16190002/diff/3001/cc/resources/pixel_buffer_...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:132: // Transfer activate pixel
buffer tasks to |new_pixel_buffer_tasks|
On 2013/05/29 16:55:03, vmpstr wrote:
> activate -> active

Done.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:393: if (did_initialize_visible_tile_) {
On 2013/05/29 16:55:03, vmpstr wrote:
> This block of code repeats in at least one other spot. Maybe it's worth making
> something like "NotifyClientOfVisibleTileInitialization" and call that here
and
> wherever the code appears

I'm planning to remove this function and the RasterWorkerPoolClient interface
completely before this patch is ready to land. I think that will clean it up
nicely.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:724: tile->tile_version().memory_state_ =
CAN_USE_MEMORY;
On 2013/05/29 16:55:03, vmpstr wrote:
> Why CAN_USE_MEMORY? set_solid_color sets it to NOT_ALLOWED_TO_USE_MEMORY. 

Oh, this is not needed in that case. I still have a hard time reasoning with
this memory state thing. We should be able to remove it completely once this
patch lands as there's no longer such a thing as a TileVersion with unreleasable
memory. It either owns a resource that can be released or it doesn't.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:728: tile->tile_version().resource_ =
resource.Pass();
On 2013/05/29 16:55:03, vmpstr wrote:
> Do you have to set the resource_id_ as well? I can't really find where that's
> set. Also, what's the reasoning for separating the two? It seems that you now
> have to maintain two items that should(?) be related

resource_id_ is needed for ForceUpload. Maybe we should change that API a bit.
How about RasterWorkerPool::ForceUploadToComplete take a raster task as
identifier instead and return a temporary ResourceId that can be used for
drawing until we have the real resource. And we store that temporary id in
something like TileVersion::forced_upload_resource_id_. Wdyt?

[vmpstr, 2013-05-29 20:45:06]

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:13: class RootWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
> Maybe we just make internal::WorkerPoolTask a full implementation by adding
> default implementations of RunOnThread and DispatchCompletionCallback like
> below?

That works. I would suspect that any other uses of the worker pool that we can
think of would have some sort of a empty task with dependencies, so making
internal::WorkerPoolTask that class makes sense.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:393: if (did_initialize_visible_tile_) {
On 2013/05/29 19:39:12, David Reveman wrote:
> On 2013/05/29 16:55:03, vmpstr wrote:
> > This block of code repeats in at least one other spot. Maybe it's worth
making
> > something like "NotifyClientOfVisibleTileInitialization" and call that here
> and
> > wherever the code appears
> 
> I'm planning to remove this function and the RasterWorkerPoolClient interface
> completely before this patch is ready to land. I think that will clean it up
> nicely.

Sounds good.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:724: tile->tile_version().memory_state_ =
CAN_USE_MEMORY;
On 2013/05/29 19:39:12, David Reveman wrote:
> On 2013/05/29 16:55:03, vmpstr wrote:
> > Why CAN_USE_MEMORY? set_solid_color sets it to NOT_ALLOWED_TO_USE_MEMORY. 
> 
> Oh, this is not needed in that case. I still have a hard time reasoning with
> this memory state thing. We should be able to remove it completely once this
> patch lands as there's no longer such a thing as a TileVersion with
unreleasable
> memory. It either owns a resource that can be released or it doesn't.

Agreed. CAN_USE_MEMORY right now means "this version has memory allocated to
it", which means that solid color tiles shouldn't be in that state

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:728: tile->tile_version().resource_ =
resource.Pass();
On 2013/05/29 19:39:12, David Reveman wrote:
> On 2013/05/29 16:55:03, vmpstr wrote:
> > Do you have to set the resource_id_ as well? I can't really find where
that's
> > set. Also, what's the reasoning for separating the two? It seems that you
now
> > have to maintain two items that should(?) be related
> 
> resource_id_ is needed for ForceUpload. Maybe we should change that API a bit.
> How about RasterWorkerPool::ForceUploadToComplete take a raster task as
> identifier instead and return a temporary ResourceId that can be used for
> drawing until we have the real resource. And we store that temporary id in
> something like TileVersion::forced_upload_resource_id_. Wdyt?

I like ForceUpload taking a task, since it makes it a bit more clear (to me)
what it's operating on. I don't know about forced_upload_resource_id_. It kinda
makes managed tile state cluttered with different resources and/or resource ids.
This temporary id would be the same id as the final one, right? Except the
ownership of the resource is still in the upload queue. 

Ideally we can hand off the ownership of the resource (ie eliminate the concept
of forced or not forced from the managed tile state's perspective), and just
deal with the fact that we might get an upload completed notifications for
resources that are no longer owned by the upload queue. However, we have to be
careful not to free the resource until we get that notification.

[kaanb, 2013-05-29 21:04:15]

I skimmed over image_raster_pool_worker mostly.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:49: virtual void
DispatchCompletionCallback() OVERRIDE {
nit: space between methods

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:84: for
(RasterTask::Queue::TaskVector::iterator it = raster_tasks_.begin();
do we support for-each syntax for Chromium?, i.e. would

for (RasterWorkerPoolTask* task : raster_tasks_) work? If so, consider using it.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:130: if (need_bind)
Should we just unconditionally bind when we acquire the image? Why do we need
the if?

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.h (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.h:40: bool need_bind);
nit: s/need/needs/ here and elsewhere

[reveman, 2013-05-30 01:46:10]

+enne

I'm temporarily removing the "force upload at beginSetPixels time" optimization
in this patch. We haven't had this for long and I doubt it makes a huge
difference. The plan is to get this optimization back in a follow up patch that
adds the information of what tiles are required for activation to the
RasterWorkerPool::ScheduleTasks interface. This will also provide an ideal
"tasks required for activation are done" signal. Enne, you OK with this?

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:13: class RootWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
On 2013/05/29 20:45:06, vmpstr wrote:
> > Maybe we just make internal::WorkerPoolTask a full implementation by adding
> > default implementations of RunOnThread and DispatchCompletionCallback like
> > below?
> 
> That works. I would suspect that any other uses of the worker pool that we can
> think of would have some sort of a empty task with dependencies, so making
> internal::WorkerPoolTask that class makes sense.

I ended up doing something a bit different in the latest patch which is in line
with my plans to replace the current OnIdle system in the worker pool with a
simple task created by the RasterWorkerPool. Let me know what you think.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:49: virtual void
DispatchCompletionCallback() OVERRIDE {
On 2013/05/29 21:04:15, kaanb wrote:
> nit: space between methods

Using no space here to make it clear they are both part of the "Overridden from
internal::WorkerPoolTask" group. The alternative is to not inline them but I
prefer it like this if you don't mind?

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:84: for
(RasterTask::Queue::TaskVector::iterator it = raster_tasks_.begin();
On 2013/05/29 21:04:15, kaanb wrote:
> do we support for-each syntax for Chromium?, i.e. would
> 
> for (RasterWorkerPoolTask* task : raster_tasks_) work? If so, consider using
it.

c++0x syntax not allowed afaik.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.cc:130: if (need_bind)
On 2013/05/29 21:04:15, kaanb wrote:
> Should we just unconditionally bind when we acquire the image? Why do we need
> the if?

Good point. I removed it from the latest patch.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
File cc/resources/image_raster_worker_pool.h (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/image_raster_...
cc/resources/image_raster_worker_pool.h:40: bool need_bind);
On 2013/05/29 21:04:15, kaanb wrote:
> nit: s/need/needs/ here and elsewhere

Done.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:393: if (did_initialize_visible_tile_) {
On 2013/05/29 20:45:06, vmpstr wrote:
> On 2013/05/29 19:39:12, David Reveman wrote:
> > On 2013/05/29 16:55:03, vmpstr wrote:
> > > This block of code repeats in at least one other spot. Maybe it's worth
> making
> > > something like "NotifyClientOfVisibleTileInitialization" and call that
here
> > and
> > > wherever the code appears
> > 
> > I'm planning to remove this function and the RasterWorkerPoolClient
interface
> > completely before this patch is ready to land. I think that will clean it up
> > nicely.
> 
> Sounds good.

Please take a look at latest patch.

https://codereview.chromium.org/16190002/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:728: tile->tile_version().resource_ =
resource.Pass();
On 2013/05/29 20:45:06, vmpstr wrote:
> On 2013/05/29 19:39:12, David Reveman wrote:
> > On 2013/05/29 16:55:03, vmpstr wrote:
> > > Do you have to set the resource_id_ as well? I can't really find where
> that's
> > > set. Also, what's the reasoning for separating the two? It seems that you
> now
> > > have to maintain two items that should(?) be related
> > 
> > resource_id_ is needed for ForceUpload. Maybe we should change that API a
bit.
> > How about RasterWorkerPool::ForceUploadToComplete take a raster task as
> > identifier instead and return a temporary ResourceId that can be used for
> > drawing until we have the real resource. And we store that temporary id in
> > something like TileVersion::forced_upload_resource_id_. Wdyt?
> 
> I like ForceUpload taking a task, since it makes it a bit more clear (to me)
> what it's operating on. I don't know about forced_upload_resource_id_. It
kinda
> makes managed tile state cluttered with different resources and/or resource
ids.
> This temporary id would be the same id as the final one, right? Except the
> ownership of the resource is still in the upload queue. 
> 
> Ideally we can hand off the ownership of the resource (ie eliminate the
concept
> of forced or not forced from the managed tile state's perspective), and just
> deal with the fact that we might get an upload completed notifications for
> resources that are no longer owned by the upload queue. However, we have to be
> careful not to free the resource until we get that notification.

ForceUpload takes a task in latest patch. It's out of the scope for this patch
but we should move towards being able to always pass the resource ownership back
at completion. This requires detaching the pixel buffer and the shared memory
from the resource somehow and I'm not sure yet how to best do that.

I'd like to eliminate the idea of a forced upload completely. Doing this
requires adding information about what tiles are required for activation to the
RasterWorkerPool::ScheduleTasks API. ie. ScheduleTasks(const RasterTaskQueue&
tasks_required_for_activation, const RasterTaskQueue& other_tasks).

[kaanb, 2013-05-30 02:02:05]

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:104: use_map_image));
You're not doing anything with |use_map_image| within the constructor, so you
might as well just remove it from its parameters.

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:836: bitmap.setConfig(SkBitmap::kARGB_8888_Config,
rect.width(), rect.height());
While you're touching this file can you also plumb the image stride here?

[reveman, 2013-05-30 03:27:18]

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:104: use_map_image));
On 2013/05/30 02:02:06, kaanb wrote:
> You're not doing anything with |use_map_image| within the constructor, so you
> might as well just remove it from its parameters.

Done.

https://codereview.chromium.org/16190002/diff/11001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:836: bitmap.setConfig(SkBitmap::kARGB_8888_Config,
rect.width(), rect.height());
On 2013/05/30 02:02:06, kaanb wrote:
> While you're touching this file can you also plumb the image stride here?

Done.

[boliu, 2013-05-30 03:30:44]

https://codereview.chromium.org/16190002/diff/16001/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/16001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:104:
raster_tasks.push_back(image_task);
I *think* needs

image_tasks_[task] = image_task;

[reveman, 2013-05-30 03:55:42]

https://codereview.chromium.org/16190002/diff/16001/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/16001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:104:
raster_tasks.push_back(image_task);
On 2013/05/30 03:30:44, boliu wrote:
> I *think* needs
> 
> image_tasks_[task] = image_task;

good catch. now fixed.

[kaanb, 2013-05-30 18:48:36]

lgtm and one question for image_raster_worker_pool

https://codereview.chromium.org/16190002/diff/19002/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:120:
resource_provider_->BindImage(task->resource()->id());
Should we still bind if the task was canceled?

[enne (OOO), 2013-05-30 19:38:31]

I really like what this removes from TileManager.  Nice work!

I'm fine with removing the optimization around uploading at BeginSetPixels time.
 Maybe we should talk some time about how activation changes relate to ongoing
TileManager changes.

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:50: return resource_id_;
Sorry for such a naive question, but I'm not sure I follow why resource_id_
needs to be stored separately from resource_? Can resource_->id() !=
resource_id_?

https://codereview.chromium.org/16190002/diff/19002/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:56: };
DISALLOW_COPY_AND_ASSIGN, here and on a bunch of other classes?

https://codereview.chromium.org/16190002/diff/19002/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/tile_manager...
cc/resources/tile_manager.cc:299: if
(client_->ShouldForceTileUploadsRequiredForActivationToComplete()) {
style nit: early out here instead?

[enne (OOO), 2013-05-30 22:31:52]

When running with this patch I get a bunch of:

FATAL:picture_pile_impl.cc(73)] Check failed: clones_for_drawing_.clones_.size()
> thread_index (0 vs. 0)
 [0x7fd2ef0147ae] base::debug::StackTrace::StackTrace()
 [0x7fd2ef06156f] logging::LogMessage::~LogMessage()
 [0x7fd2e9403613] cc::PicturePileImpl::GetCloneForDrawingOnThread()
 [0x7fd2e9417e06] cc::(anonymous
namespace)::RasterWorkerPoolTaskImpl::RunOnThread()
 [0x7fd2e940862d] cc::(anonymous
namespace)::PixelBufferWorkerPoolTaskImpl::RunOnThread()
 [0x7fd2e92f9dfc] cc::WorkerPool::Inner::Run()

Also sometimes this:

FATAL:picture_pile_impl.cc(265)] Check failed:
!unclipped.Contains(content_rect). 
 [0x7f7ba24287ae] base::debug::StackTrace::StackTrace()
 [0x7f7ba247556f] logging::LogMessage::~LogMessage()
 [0x7f7b9c81863e] cc::PicturePileImpl::RasterCommon()
 [0x7f7b9c818c38] cc::PicturePileImpl::RasterToBitmap()
 [0x7f7b9c85071b] cc::TileManager::RunRasterTask()

I can repro the top crash within a second or two by running with impl-side
painting on Linux against poster circle and then just highlighting the page
wildly with my mouse.

[enne (OOO), 2013-05-30 22:32:52]

I meant to add that is with patch set 5 against r203219.

[reveman, 2013-05-30 23:25:18]

https://codereview.chromium.org/16190002/diff/19002/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:120:
resource_provider_->BindImage(task->resource()->id());
On 2013/05/30 18:48:36, kaanb wrote:
> Should we still bind if the task was canceled?

this is essentially the same as the needs_bind condition I removed. we might
want to avoid the bind in these cases but lets wait and add that when we have
some data showing that this unnecessary bind is worth avoiding.

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:50: return resource_id_;
On 2013/05/30 19:38:31, enne wrote:
> Sorry for such a naive question, but I'm not sure I follow why resource_id_
> needs to be stored separately from resource_? Can resource_->id() !=
> resource_id_?

This is because of the current force upload mess. Force upload allows us to draw
using the resource but the ownership of the resource need to stay with the
RasterWorkerPool as it still has shared memory attached to it that can't be
released. We're planning to fix this but it will require some significant
changes to the ResourceProvider's pixel buffer API.

https://codereview.chromium.org/16190002/diff/19002/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:56: };
On 2013/05/30 19:38:31, enne wrote:
> DISALLOW_COPY_AND_ASSIGN, here and on a bunch of other classes?

Done.

https://codereview.chromium.org/16190002/diff/19002/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/tile_manager...
cc/resources/tile_manager.cc:299: if
(client_->ShouldForceTileUploadsRequiredForActivationToComplete()) {
On 2013/05/30 19:38:31, enne wrote:
> style nit: early out here instead?

Done.

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:72: scoped_refptr<PicturePileImpl>
picture_pile_;
A tile's PicturePileImpl might change during activation. Maybe we can limit the
tiles that we can change piles for to only tiles that doesn't have pending
raster tasks but it seems like a slippery slope. Keeping a ref here is a simple
and reliable way to make sure we always have a valid pile for raster.

[vmpstr, 2013-05-31 15:55:10]

lgtm

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:72: scoped_refptr<PicturePileImpl>
picture_pile_;
On 2013/05/30 23:25:18, David Reveman wrote:
> A tile's PicturePileImpl might change during activation. Maybe we can limit
the
> tiles that we can change piles for to only tiles that doesn't have pending
> raster tasks but it seems like a slippery slope. Keeping a ref here is a
simple
> and reliable way to make sure we always have a valid pile for raster.

I think that makes the most sense.

[enne (OOO), 2013-05-31 17:05:37]

lgtm2

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:50: return resource_id_;
On 2013/05/30 23:25:18, David Reveman wrote:
> On 2013/05/30 19:38:31, enne wrote:
> > Sorry for such a naive question, but I'm not sure I follow why resource_id_
> > needs to be stored separately from resource_? Can resource_->id() !=
> > resource_id_?
> 
> This is because of the current force upload mess. Force upload allows us to
draw
> using the resource but the ownership of the resource need to stay with the
> RasterWorkerPool as it still has shared memory attached to it that can't be
> released. We're planning to fix this but it will require some significant
> changes to the ResourceProvider's pixel buffer API.

Ok, thanks.  Can you add a FIXME here and some comment about when resource_id_
might not equal resource_? I'm sure somebody else will have the same question
when looking at this code.

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/16190002/diff/21002/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:72: scoped_refptr<PicturePileImpl>
picture_pile_;
On 2013/05/31 15:55:10, vmpstr wrote:
> On 2013/05/30 23:25:18, David Reveman wrote:
> > A tile's PicturePileImpl might change during activation. Maybe we can limit
> the
> > tiles that we can change piles for to only tiles that doesn't have pending
> > raster tasks but it seems like a slippery slope. Keeping a ref here is a
> simple
> > and reliable way to make sure we always have a valid pile for raster.
> 
> I think that makes the most sense.

The reason we change the pile during activation is to keep memory under control
by keeping the number of piles in use capped at 2.  If we temporarily go above
that for tiles in flight, that doesn't seem like a big deal.

Correctness isn't affected here, as if using the new pile would have created a
different rasterization we would have created an entirely new tile for it anyway
and it wouldn't be using the old pile.

[reveman, 2013-06-01 03:02:37]

Fyi, I've fixed the unit test failures and also added a framework and a basic
test for the RasterWorkerPool classes.

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/16190002/diff/19002/cc/resources/managed_tile...
cc/resources/managed_tile_state.h:50: return resource_id_;
On 2013/05/31 17:05:38, enne wrote:
> On 2013/05/30 23:25:18, David Reveman wrote:
> > On 2013/05/30 19:38:31, enne wrote:
> > > Sorry for such a naive question, but I'm not sure I follow why
resource_id_
> > > needs to be stored separately from resource_? Can resource_->id() !=
> > > resource_id_?
> > 
> > This is because of the current force upload mess. Force upload allows us to
> draw
> > using the resource but the ownership of the resource need to stay with the
> > RasterWorkerPool as it still has shared memory attached to it that can't be
> > released. We're planning to fix this but it will require some significant
> > changes to the ResourceProvider's pixel buffer API.
> 
> Ok, thanks.  Can you add a FIXME here and some comment about when resource_id_
> might not equal resource_? I'm sure somebody else will have the same question
> when looking at this code.

Done.

[commit-bot: I haz the power, 2013-06-02 18:45:28]

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

[commit-bot: I haz the power, 2013-06-02 18:45:37]

Failed to apply patch for cc/resources/raster_worker_pool.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/raster_worker_pool.cc
  Hunk #2 FAILED at 10.
  1 out of 4 hunks FAILED -- saving rejects to file
cc/resources/raster_worker_pool.cc.rej

Patch:       cc/resources/raster_worker_pool.cc
Index: cc/resources/raster_worker_pool.cc
diff --git a/cc/resources/raster_worker_pool.cc
b/cc/resources/raster_worker_pool.cc
index
a078fb00bb7e9600e7408a10bd37c92b23a5d9fb..97489582d5a30919cd8da6c13395852214e91ae0
100644
--- a/cc/resources/raster_worker_pool.cc
+++ b/cc/resources/raster_worker_pool.cc
@@ -1,4 +1,4 @@
-// Copyright 2012 The Chromium Authors. All rights reserved.
+// Copyright 2013 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.
 
@@ -10,72 +10,70 @@ namespace cc {
 
 namespace {
 
-class RasterWorkerPoolContainerTaskImpl : public internal::WorkerPoolTask {
- public:
-  RasterWorkerPoolContainerTaskImpl(
-      internal::WorkerPoolTask::TaskVector* dependencies)
-      : internal::WorkerPoolTask(dependencies) {
-  }
-
-  // Overridden from internal::WorkerPoolTask:
-  virtual void RunOnThread(unsigned thread_index) OVERRIDE {}
-  virtual void DispatchCompletionCallback() OVERRIDE {}
-
- private:
-  virtual ~RasterWorkerPoolContainerTaskImpl() {}
-};
+void Noop() {}
 
-class RasterWorkerPoolTaskImpl : public internal::WorkerPoolTask {
+class WorkerPoolTaskImpl : public internal::WorkerPoolTask {
  public:
-  RasterWorkerPoolTaskImpl(const base::Closure& callback,
-                           const RasterWorkerPool::Task::Reply& reply)
+  WorkerPoolTaskImpl(const base::Closure& callback,
+                     const base::Closure& reply)
       : callback_(callback),
         reply_(reply) {
   }
+  explicit WorkerPoolTaskImpl(
+      internal::WorkerPoolTask::TaskVector* dependencies)
+      : internal::WorkerPoolTask(dependencies),
+        callback_(base::Bind(&Noop)),
+        reply_(base::Bind(&Noop)) {
+  }
 
   // Overridden from internal::WorkerPoolTask:
   virtual void RunOnThread(unsigned thread_index) OVERRIDE {
     callback_.Run();
   }
   virtual void DispatchCompletionCallback() OVERRIDE {
-    reply_.Run(!HasFinishedRunning());
+    reply_.Run();
   }
 
  private:
-  virtual ~RasterWorkerPoolTaskImpl() {}
+  virtual ~WorkerPoolTaskImpl() {}
 
   const base::Closure callback_;
-  const RasterWorkerPool::Task::Reply reply_;
+  const base::Closure reply_;
+
+  DISALLOW_COPY_AND_ASSIGN(WorkerPoolTaskImpl);
 };
 
-class RasterWorkerPoolPictureTaskImpl : public internal::WorkerPoolTask {
+class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
  public:
-  RasterWorkerPoolPictureTaskImpl(
+  RasterWorkerPoolTaskImpl(
       PicturePileImpl* picture_pile,
-      const RasterWorkerPool::PictureTask::Callback& callback,
-      const RasterWorkerPool::Task::Reply& reply,
+      const Resource* resource,
+      const RasterWorkerPool::RasterTask::Callback& callback,
+      const RasterWorkerPool::RasterTask::Reply& reply,
       internal::WorkerPoolTask::TaskVector* dependencies)
-      : internal::WorkerPoolTask(dependencies),
+      : internal::RasterWorkerPoolTask(resource, dependencies),
         picture_pile_(picture_pile),
         callback_(callback),
         reply_(reply) {
-    DCHECK(picture_pile_);
   }
 
-  // Overridden from internal::WorkerPoolTask:
-  virtual void RunOnThread(unsigned thread_index) OVERRIDE {
-    callback_.Run(picture_pile_->GetCloneForDrawingOnThread(thread_index));
+  // Overridden from internal::RasterWorkerPoolTask:
+  virtual bool RunOnThread(SkDevice* device, unsigned thread_index) OVERRIDE {
+    return callback_.Run(
+        device, picture_pile_->GetCloneForDrawingOnThread(thread_index));
   }
   virtual void DispatchCompletionCallback() OVERRIDE {
     reply_.Run(!HasFinishedRunning());
   }
 
  private:
-  virtual ~RasterWorkerPoolPictureTaskImpl() {}
+  virtual ~RasterWorkerPoolTaskImpl() {}
 
   scoped_refptr<PicturePileImpl> picture_pile_;
-  const RasterWorkerPool::PictureTask::Callback callback_;
-  const RasterWorkerPool::Task::Reply reply_;
+  const RasterWorkerPool::RasterTask::Callback callback_;
+  const RasterWorkerPool::RasterTask::Reply reply_;
+
+  DISALLOW_COPY_AND_ASSIGN(RasterWorkerPoolTaskImpl);
 };
 
 const char* kWorkerThreadNamePrefix = "CompositorRaster";
@@ -84,13 +82,47 @@ const int kCheckForCompletedTasksDelayMs = 6;
 
 }  // namespace
 
-RasterWorkerPool::Task::Queue::Queue() {
+namespace internal {
+
+RasterWorkerPoolTask::RasterWorkerPoolTask(
+    const Resource* resource,
+    WorkerPoolTask::TaskVector* dependencies)
+    : did_run_(false),
+      did_complete_(false),
+      resource_(resource) {
+  dependencies_.swap(*dependencies);
+}
+
+RasterWorkerPoolTask::~RasterWorkerPoolTask() {
+}
+
+void RasterWorkerPoolTask::DidRun() {
+  DCHECK(!did_run_);
+  did_run_ = true;
+}
+
+bool RasterWorkerPoolTask::HasFinishedRunning() const {
+  return did_run_;
+}
+
+void RasterWorkerPoolTask::DidComplete() {
+  DCHECK(!did_complete_);
+  did_complete_ = true;
 }
 
-RasterWorkerPool::Task::Queue::~Queue() {
+bool RasterWorkerPoolTask::HasCompleted() const {
+  return did_complete_;
 }
 
-void RasterWorkerPool::Task::Queue::Append(const Task& task) {
+}  // namespace internal
+
+RasterWorkerPool::Task::Set::Set() {
+}
+
+RasterWorkerPool::Task::Set::~Set() {
+}
+
+void RasterWorkerPool::Task::Set::Insert(const Task& task) {
   DCHECK(!task.is_null());
   tasks_.push_back(task.internal_);
 }
@@ -99,54 +131,84 @@ RasterWorkerPool::Task::Task() {
 }
 
 RasterWorkerPool::Task::Task(const base::Closure& callback,
-                             const Reply& reply)
-    : internal_(new RasterWorkerPoolTaskImpl(callback, reply)) {
+                             const base::Closure& reply)
+    : internal_(new WorkerPoolTaskImpl(callback, reply)) {
+}
+
+RasterWorkerPool::Task::~Task() {
 }
 
-RasterWorkerPool::Task::Task(Queue* dependencies)
-    : internal_(new RasterWorkerPoolContainerTaskImpl(&dependencies->tasks_)) {
+void RasterWorkerPool::Task::Reset() {
+  internal_ = NULL;
 }
 
-RasterWorkerPool::Task::Task(scoped_refptr<internal::WorkerPoolTask> internal)
-    : internal_(internal) {
+RasterWorkerPool::RasterTask::Queue::Queue() {
 }
 
-RasterWorkerPool::Task::~Task() {
+RasterWorkerPool::RasterTask::Queue::~Queue() {
 }
 
-void RasterWorkerPool::Task::Reset() {
+void RasterWorkerPool::RasterTask::Queue::Append(const RasterTask& task) {
+  DCHECK(!task.is_null());
+  tasks_.push_back(task.internal_);
+}
+
+RasterWorkerPool::RasterTask::RasterTask() {
+}
+
+RasterWorkerPool::RasterTask::RasterTask(PicturePileImpl* picture_pile,
+                                         const Resource* resource,
+                                         const Callback& callback,
+                                         const Reply& reply,
+                                         Task::Set* dependencies)
+    : internal_(new RasterWorkerPoolTaskImpl(picture_pile,
+                                             resource,
+                                             callback,
+                                             reply,
+                                             &dependencies->tasks_)) {
+}
+
+void RasterWorkerPool::RasterTask::Reset() {
   internal_ = NULL;
 }
 
-RasterWorkerPool::PictureTask::PictureTask(PicturePileImpl* picture_pile,
-                                           const Callback& callback,
-                                           const Reply& reply,
-                                           Task::Queue* dependencies)
-    : RasterWorkerPool::Task(
-        new RasterWorkerPoolPictureTaskImpl(picture_pile,
-                                            callback,
-                                            reply,
-                                            &dependencies->tasks_)) {
+RasterWorkerPool::RasterTask::~RasterTask() {
 }
 
-RasterWorkerPool::RasterWorkerPool(size_t num_threads) : WorkerPool(
-    num_threads,
-    base::TimeDelta::FromMilliseconds(kCheckForCompletedTasksDelayMs),
-    kWorkerThreadNamePrefix) {
+RasterWorkerPool::RasterWorkerPool(ResourceProvider* resource_provider,
+                                   size_t num_threads)
+    : WorkerPool(
+        num_threads,
+        base::TimeDelta::FromMilliseconds(kCheckForCompletedTasksDelayMs),
+        kWorkerThreadNamePrefix),
+      resource_provider_(resource_provider) {
 }
 
 RasterWorkerPool::~RasterWorkerPool() {
 }
 
 void RasterWorkerPool::Shutdown() {
-  // Cancel all previously scheduled tasks.
-  WorkerPool::ScheduleTasks(NULL);
-
+  raster_tasks_.clear();
   WorkerPool::Shutdown();
 }
 
-void RasterWorkerPool::ScheduleTasks(Task* task) {
-  WorkerPool::ScheduleTasks(task ? task->internal_ : NULL);
+bool RasterWorkerPool::ForceUploadToComplete(const RasterTask& raster_task) {
+  return false;
+}
+
+void RasterWorkerPool::SetRasterTasks(RasterTask::Queue* queue) {
+  raster_tasks_.swap(queue->tasks_);
+}
+
+void RasterWorkerPool::ScheduleRasterTasks(
+    internal::WorkerPoolTask::TaskVector* raster_tasks) {
+  if (raster_tasks->empty()) {
+    WorkerPool::ScheduleTasks(NULL);
+    return;
+  }
+
+  scoped_refptr<WorkerPoolTaskImpl> root(new WorkerPoolTaskImpl(raster_tasks));
+  WorkerPool::ScheduleTasks(root);
 }
 
 }  // namespace cc

[commit-bot: I haz the power, 2013-06-03 03:08:59]

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

[commit-bot: I haz the power, 2013-06-03 05:18:45]

Change committed as 203670