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

cc/resources/pixel_buffer_raster_worker_pool.cc

Index: cc/resources/pixel_buffer_raster_worker_pool.cc
diff --git a/cc/resources/pixel_buffer_raster_worker_pool.cc b/cc/resources/pixel_buffer_raster_worker_pool.cc
index 574065e23f59e88ab8599b885e43f1568ddc840b..8eca51b549740985d47a3d25cc7153fb268d7131 100644
--- a/cc/resources/pixel_buffer_raster_worker_pool.cc
+++ b/cc/resources/pixel_buffer_raster_worker_pool.cc
@@ -97,6 +97,10 @@ void AddDependenciesToGraphNode(
   }
 }
 
+bool WasCanceled(const internal::RasterWorkerPoolTask* task) {

[vmpstr, 2013/07/10 16:46:49]

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.

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

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.

+  return task->WasCanceled();
+}
+
 }  // namespace
 
 PixelBufferRasterWorkerPool::PixelBufferRasterWorkerPool(
@@ -200,6 +204,14 @@ void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
       tasks_required_for_activation_.insert(task);
   }
 
+  // |tasks_required_for_activation_| contains all tasks that need to
+  // complete before we can send a "ready to activate" signal. Tasks
+  // that have already completed should not be part of this set.
+  for (TaskDeque::const_iterator it = completed_tasks_.begin();

[vmpstr, 2013/07/10 16:46:49]

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?

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

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.

+       it != completed_tasks_.end(); ++it) {
+    tasks_required_for_activation_.erase(*it);
+  }
+
   pixel_buffer_tasks_.swap(new_pixel_buffer_tasks);
 
   // Check for completed tasks when ScheduleTasks() is called as
@@ -409,10 +421,16 @@ void PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks() {
     ScheduleCheckForCompletedRasterTasks();
 
   // Generate client notifications.
-  if (will_notify_client_that_no_tasks_required_for_activation_are_pending)
+  if (will_notify_client_that_no_tasks_required_for_activation_are_pending) {
+    DCHECK(std::find_if(raster_tasks_required_for_activation().begin(),
+                        raster_tasks_required_for_activation().end(),
+                        WasCanceled) ==
+          raster_tasks_required_for_activation().end());
     client()->DidFinishedRunningTasksRequiredForActivation();
+  }
   if (will_notify_client_that_no_tasks_are_pending) {
     TRACE_EVENT_ASYNC_END0("cc", "ScheduledTasks", this);
+    DCHECK(!HasPendingTasksRequiredForActivation());
     client()->DidFinishedRunningTasks();
   }
 }
@@ -582,6 +600,19 @@ void PixelBufferRasterWorkerPool::OnRasterTaskCompleted(
 
   if (!needs_upload) {
     resource_provider()->ReleasePixelBuffer(task->resource()->id());
+
+    if (was_canceled) {

[vmpstr, 2013/07/10 16:46:49]

Does was_canceled guarantee that needs_upload is false?

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

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

+      // When priorites change, a raster task can be canceled as a result of
+      // no longer being of high enough priority to fit in our throttled
+      // raster task budget. The task has not yet completed in this case.
+      if (std::find(raster_tasks().begin(),

[vmpstr, 2013/07/10 16:46:49]

I prefer you save the iterator first, then do the if check

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

Done.

+                    raster_tasks().end(),
+                    task) != raster_tasks().end()) {
+        pixel_buffer_tasks_[task.get()] = NULL;
+        return;
+      }
+    }
+
     task->DidRun(was_canceled);
     DCHECK(std::find(completed_tasks_.begin(),
                      completed_tasks_.end(),