cc: Fix incorrect completion of tiles and missing "ready to activate" signal.

cc: Fix incorrect completion of tiles and missing "ready to activate" signal.

The presence of completed tiles in |tasks_required_for_activation_|
prevents us from generating a "ready to activate" signal and can
result in an inability to activate the pending tree. Incorrectly
canceling tasks can result in the pending tree being activated
without all tiles required for activation being properly initialized.

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

[vmpstr, 2013-07-10 16:46:48]

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:100: bool WasCanceled(const
internal::RasterWorkerPoolTask* task) {
If this is only for a DCHECK, maybe we can name it something like
WasCanceledForDebug... I'm not sure, I would just like to discourage future use
of this outside of std::find_if and things like that.

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:210: for
(TaskDeque::const_iterator it = completed_tasks_.begin();
I think we can fold this loop and the one above into the remaining two loops in
this function. Having separate loops is a bit clearer, but I'm just wary that
we're doing multiple iterations through the tasks when we really don't have to.
Wdyt?

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:604: if (was_canceled) {
Does was_canceled guarantee that needs_upload is false?

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:608: if
(std::find(raster_tasks().begin(),
I prefer you save the iterator first, then do the if check

https://codereview.chromium.org/18614012/diff/1/cc/resources/raster_worker_po...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/18614012/diff/1/cc/resources/raster_worker_po...
cc/resources/raster_worker_pool.h:230: const RasterTask::Queue::TaskSet&
raster_tasks_required_for_activation()
This one is also just for a DCHECK, right?

[reveman, 2013-07-11 00:37:21]

PTAL

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:100: bool WasCanceled(const
internal::RasterWorkerPoolTask* task) {
On 2013/07/10 16:46:49, vmpstr wrote:
> If this is only for a DCHECK, maybe we can name it something like
> WasCanceledForDebug... I'm not sure, I would just like to discourage future
use
> of this outside of std::find_if and things like that.

ForDebug in the name makes it sound like it's checking something else. I added a
comment that makes it clear this is only for debugging purposes.

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:210: for
(TaskDeque::const_iterator it = completed_tasks_.begin();
On 2013/07/10 16:46:49, vmpstr wrote:
> I think we can fold this loop and the one above into the remaining two loops
in
> this function. Having separate loops is a bit clearer, but I'm just wary that
> we're doing multiple iterations through the tasks when we really don't have
to.
> Wdyt?

This and ScheduleMoreTasks needs a rewrite at some point but I'd rather not mess
with any of that in this patch but instead keep this as simple as possible for
now.

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:604: if (was_canceled) {
On 2013/07/10 16:46:49, vmpstr wrote:
> Does was_canceled guarantee that needs_upload is false?

Yes, see PixelBufferWorkerPoolTaskImpl above. I added a DCHECK below in in
latest patch that verifies this.

https://codereview.chromium.org/18614012/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:608: if
(std::find(raster_tasks().begin(),
On 2013/07/10 16:46:49, vmpstr wrote:
> I prefer you save the iterator first, then do the if check

Done.

[vmpstr, 2013-07-11 17:55:12]

lgtm

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

https://codereview.chromium.org/18614012/diff/6001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:627: DCHECK(!was_canceled);
nit: I would also prefer DCHECK(!was_canceled || !needs_upload) (was_canceled
implies !needs_upload) above the if (!needs_upload) check.

[vmpstr, 2013-07-11 18:12:55]

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

https://codereview.chromium.org/18614012/diff/6001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:627: DCHECK(!was_canceled);
On 2013/07/11 17:55:12, vmpstr wrote:
> nit: I would also prefer DCHECK(!was_canceled || !needs_upload) (was_canceled
> implies !needs_upload) above the if (!needs_upload) check.

Nevermind this, I missed the fact that the if returns at the end.

[commit-bot: I haz the power, 2013-07-11 18:14:19]

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

[commit-bot: I haz the power, 2013-07-11 22:00:45]

Change committed as 211225