Re-land: cc: Cleanup internal::WorkerPoolTaskClient interface.

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 b5b978874048255be38b40e26c11e1f24fad75b0..e144b3e3bf4a1168c2e6c1605ccdbef0cdad0525 100644
--- a/cc/resources/pixel_buffer_raster_worker_pool.cc
+++ b/cc/resources/pixel_buffer_raster_worker_pool.cc
@@ -21,7 +21,7 @@ typedef base::StackVector<internal::WorkerPoolTask*, kMaxScheduledRasterTasks>
     WorkerPoolTaskVector;
 
 // Only used as std::find_if predicate for DCHECKs.
-bool WasCanceled(const internal::RasterWorkerPoolTask* task) {
+bool WasCanceled(const internal::WorkerPoolTask* task) {
   return !task->HasFinishedRunning();
 }
 
@@ -77,12 +77,13 @@ void PixelBufferRasterWorkerPool::Shutdown() {
   for (RasterTaskStateMap::iterator it = raster_task_states_.begin();
        it != raster_task_states_.end();
        ++it) {
-    internal::RasterWorkerPoolTask* task = it->first;
+    internal::WorkerPoolTask* task = it->first;
+    RasterTaskState& state = it->second;
 
     // All unscheduled tasks need to be canceled.
-    if (it->second == UNSCHEDULED) {
+    if (state.type == RasterTaskState::UNSCHEDULED) {
       completed_raster_tasks_.push_back(task);
-      it->second = COMPLETED;
+      state.type = RasterTaskState::COMPLETED;
     }
   }
   DCHECK_EQ(completed_raster_tasks_.size(), raster_task_states_.size());
@@ -111,24 +112,25 @@ void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTaskQueue* queue) {
        it != queue->items.end();
        ++it) {
     const RasterTaskQueue::Item& item = *it;
-    internal::RasterWorkerPoolTask* task = item.task;
+    internal::WorkerPoolTask* task = item.task;
     DCHECK(new_raster_task_states.find(task) == new_raster_task_states.end());
 
     RasterTaskStateMap::iterator state_it = raster_task_states_.find(task);
     if (state_it != raster_task_states_.end()) {
-      RasterTaskState state = state_it->second;
+      RasterTaskState& state = state_it->second;
 
       new_raster_task_states[task] = state;
       // |raster_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.
-      if (state != COMPLETED && item.required_for_activation)
+      if (state.type != RasterTaskState::COMPLETED &&
+          item.required_for_activation)
         raster_tasks_required_for_activation_.insert(task);
 
       raster_task_states_.erase(state_it);
     } else {
       DCHECK(!task->HasBeenScheduled());
-      new_raster_task_states[task] = UNSCHEDULED;
+      new_raster_task_states[task].type = RasterTaskState::UNSCHEDULED;

[vmpstr, 2014/02/19 00:49:31]

You either need to set resource to NULL here or have a constructor for
RasterTaskState (I would prefer the latter).

[reveman, 2014/02/19 01:41:13]

added an explicit default ctor.

       if (item.required_for_activation)
         raster_tasks_required_for_activation_.insert(task);
     }
@@ -139,22 +141,23 @@ void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTaskQueue* queue) {
   for (RasterTaskStateMap::iterator it = raster_task_states_.begin();
        it != raster_task_states_.end();
        ++it) {
-    internal::RasterWorkerPoolTask* task = it->first;
+    internal::WorkerPoolTask* task = it->first;
+    RasterTaskState& state = it->second;
     DCHECK(new_raster_task_states.find(task) == new_raster_task_states.end());
 
     // Unscheduled task can be canceled.
-    if (it->second == UNSCHEDULED) {
+    if (state.type == RasterTaskState::UNSCHEDULED) {
       DCHECK(!task->HasBeenScheduled());
       DCHECK(std::find(completed_raster_tasks_.begin(),
                        completed_raster_tasks_.end(),
                        task) == completed_raster_tasks_.end());
       completed_raster_tasks_.push_back(task);
-      new_raster_task_states[task] = COMPLETED;
+      new_raster_task_states[task].type = RasterTaskState::COMPLETED;
       continue;
     }
 
     // Move state to |new_raster_task_states|.
-    new_raster_task_states[task] = it->second;
+    new_raster_task_states[task] = state;
   }
 
   raster_tasks_.Swap(queue);
@@ -209,10 +212,9 @@ void PixelBufferRasterWorkerPool::CheckForCompletedTasks() {
   }
 
   while (!completed_raster_tasks_.empty()) {
-    internal::RasterWorkerPoolTask* task =
-        completed_raster_tasks_.front().get();
+    internal::WorkerPoolTask* task = completed_raster_tasks_.front().get();
     DCHECK(raster_task_states_.find(task) != raster_task_states_.end());
-    DCHECK_EQ(COMPLETED, raster_task_states_[task]);
+    DCHECK_EQ(RasterTaskState::COMPLETED, raster_task_states_[task].type);
 
     raster_task_states_.erase(task);
 
@@ -223,67 +225,21 @@ void PixelBufferRasterWorkerPool::CheckForCompletedTasks() {
 }
 
 SkCanvas* PixelBufferRasterWorkerPool::AcquireCanvasForRaster(
-    internal::RasterWorkerPoolTask* task) {
-  resource_provider()->AcquirePixelRasterBuffer(task->resource()->id());
-  return resource_provider()->MapPixelRasterBuffer(task->resource()->id());
-}
-
-void PixelBufferRasterWorkerPool::OnRasterCompleted(
-    internal::RasterWorkerPoolTask* task,
-    const PicturePileImpl::Analysis& analysis) {
-  TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("cc"),
-               "PixelBufferRasterWorkerPool::OnRasterCompleted",
-               "was_canceled",
-               !task->HasFinishedRunning(),
-               "needs_upload",
-               task->HasFinishedRunning() && !analysis.is_solid_color);
-
+    internal::WorkerPoolTask* task,
+    const Resource* resource) {
   DCHECK(raster_task_states_.find(task) != raster_task_states_.end());
-  DCHECK_EQ(SCHEDULED, raster_task_states_[task]);
-
-  // Balanced with MapPixelRasterBuffer() call in AcquireCanvasForRaster().
-  resource_provider()->UnmapPixelRasterBuffer(task->resource()->id());
-
-  if (!task->HasFinishedRunning() || analysis.is_solid_color) {
-    resource_provider()->ReleasePixelRasterBuffer(task->resource()->id());
-
-    if (!task->HasFinishedRunning()) {
-      // 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.
-      for (RasterTaskQueue::Item::Vector::const_iterator it =
-               raster_tasks_.items.begin();
-           it != raster_tasks_.items.end();
-           ++it) {
-        if (it->task == task) {
-          raster_task_states_[task] = UNSCHEDULED;
-          return;
-        }
-      }
-    }
-
-    DCHECK(std::find(completed_raster_tasks_.begin(),
-                     completed_raster_tasks_.end(),
-                     task) == completed_raster_tasks_.end());
-    completed_raster_tasks_.push_back(task);
-    raster_task_states_[task] = COMPLETED;
-    raster_tasks_required_for_activation_.erase(task);
-    return;
-  }
-
-  DCHECK(task->HasFinishedRunning());
-
-  resource_provider()->BeginSetPixels(task->resource()->id());
-  has_performed_uploads_since_last_flush_ = true;
-
-  bytes_pending_upload_ += task->resource()->bytes();
-  raster_tasks_with_pending_upload_.push_back(task);
-  raster_task_states_[task] = UPLOADING;
+  DCHECK(!raster_task_states_[task].resource);
+  raster_task_states_[task].resource = resource;
+  resource_provider()->AcquirePixelRasterBuffer(resource->id());
+  return resource_provider()->MapPixelRasterBuffer(resource->id());
 }
 
-void PixelBufferRasterWorkerPool::OnImageDecodeCompleted(
-    internal::WorkerPoolTask* task) {
-  completed_image_decode_tasks_.push_back(task);
+void PixelBufferRasterWorkerPool::ReleaseCanvasForRaster(
+    internal::WorkerPoolTask* task,
+    const Resource* resource) {
+  DCHECK(raster_task_states_.find(task) != raster_task_states_.end());
+  DCHECK(raster_task_states_[task].resource == resource);
+  resource_provider()->ReleasePixelRasterBuffer(resource->id());
 }
 
 void PixelBufferRasterWorkerPool::OnRasterTasksFinished() {
@@ -323,17 +279,18 @@ void PixelBufferRasterWorkerPool::FlushUploads() {
 }
 
 void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
-  RasterTaskDeque tasks_with_completed_uploads;
+  TaskDeque tasks_with_completed_uploads;
 
   // First check if any have completed.
   while (!raster_tasks_with_pending_upload_.empty()) {
-    internal::RasterWorkerPoolTask* task =
+    internal::WorkerPoolTask* task =
         raster_tasks_with_pending_upload_.front().get();
     DCHECK(raster_task_states_.find(task) != raster_task_states_.end());
-    DCHECK_EQ(UPLOADING, raster_task_states_[task]);
+    RasterTaskState& state = raster_task_states_[task];
+    DCHECK_EQ(RasterTaskState::UPLOADING, state.type);
 
     // Uploads complete in the order they are issued.
-    if (!resource_provider()->DidSetPixelsComplete(task->resource()->id()))
+    if (!resource_provider()->DidSetPixelsComplete(state.resource->id()))
       break;
 
     tasks_with_completed_uploads.push_back(task);
@@ -345,10 +302,10 @@ void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
       shutdown_ || client()->ShouldForceTasksRequiredForActivationToComplete();
 
   if (should_force_some_uploads_to_complete) {
-    RasterTaskDeque tasks_with_uploads_to_force;
-    RasterTaskDeque::iterator it = raster_tasks_with_pending_upload_.begin();
+    TaskDeque tasks_with_uploads_to_force;
+    TaskDeque::iterator it = raster_tasks_with_pending_upload_.begin();
     while (it != raster_tasks_with_pending_upload_.end()) {
-      internal::RasterWorkerPoolTask* task = it->get();
+      internal::WorkerPoolTask* task = it->get();
       DCHECK(raster_task_states_.find(task) != raster_task_states_.end());
 
       // Force all uploads required for activation to complete.
@@ -365,11 +322,14 @@ void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
 
     // Force uploads in reverse order. Since forcing can cause a wait on
     // all previous uploads, we would rather wait only once downstream.
-    for (RasterTaskDeque::reverse_iterator it =
-             tasks_with_uploads_to_force.rbegin();
+    for (TaskDeque::reverse_iterator it = tasks_with_uploads_to_force.rbegin();
          it != tasks_with_uploads_to_force.rend();
          ++it) {
-      resource_provider()->ForceSetPixelsToComplete((*it)->resource()->id());
+      internal::WorkerPoolTask* task = it->get();
+      RasterTaskState& state = raster_task_states_[task];
+      DCHECK(state.resource);
+
+      resource_provider()->ForceSetPixelsToComplete(state.resource->id());
       has_performed_uploads_since_last_flush_ = true;
     }
   }
@@ -377,19 +337,20 @@ void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
   // Release shared memory and move tasks with completed uploads
   // to |completed_raster_tasks_|.
   while (!tasks_with_completed_uploads.empty()) {
-    internal::RasterWorkerPoolTask* task =
-        tasks_with_completed_uploads.front().get();
+    internal::WorkerPoolTask* task = tasks_with_completed_uploads.front().get();
+    RasterTaskState& state = raster_task_states_[task];
 
-    // It's now safe to release the pixel buffer and the shared memory.
-    resource_provider()->ReleasePixelRasterBuffer(task->resource()->id());
+    bytes_pending_upload_ -= state.resource->bytes();
 
-    bytes_pending_upload_ -= task->resource()->bytes();
+    task->WillComplete();
+    task->CompleteOnOriginThread(this);
+    task->DidComplete();
 
     DCHECK(std::find(completed_raster_tasks_.begin(),
                      completed_raster_tasks_.end(),
                      task) == completed_raster_tasks_.end());
     completed_raster_tasks_.push_back(task);
-    raster_task_states_[task] = COMPLETED;
+    state.type = RasterTaskState::COMPLETED;
 
     raster_tasks_required_for_activation_.erase(task);
 
@@ -523,8 +484,10 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
     if (state_it == raster_task_states_.end())
       continue;
 
+    RasterTaskState& state = state_it->second;
+
     // Skip task if completed.
-    if (state_it->second == COMPLETED) {
+    if (state.type == RasterTaskState::COMPLETED) {
       DCHECK(std::find(completed_raster_tasks_.begin(),
                        completed_raster_tasks_.end(),
                        task) != completed_raster_tasks_.end());
@@ -540,7 +503,7 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
     }
 
     // If raster has finished, just update |bytes_pending_upload|.
-    if (state_it->second == UPLOADING) {
+    if (state.type == RasterTaskState::UPLOADING) {
       DCHECK(task->HasCompleted());
       bytes_pending_upload = new_bytes_pending_upload;
       continue;
@@ -556,8 +519,9 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
     // throttling limits.
     bytes_pending_upload = new_bytes_pending_upload;
 
-    DCHECK(state_it->second == UNSCHEDULED || state_it->second == SCHEDULED);
-    state_it->second = SCHEDULED;
+    DCHECK(state.type == RasterTaskState::UNSCHEDULED ||
+           state.type == RasterTaskState::SCHEDULED);
+    state.type = RasterTaskState::SCHEDULED;
 
     InsertNodeForRasterTask(&graph_, task, task->dependencies(), priority++);
 
@@ -662,15 +626,69 @@ void PixelBufferRasterWorkerPool::CheckForCompletedWorkerPoolTasks() {
     internal::WorkerPoolTask* task =
         static_cast<internal::WorkerPoolTask*>(it->get());
 
+    RasterTaskStateMap::iterator state_it = raster_task_states_.find(task);
+    if (state_it != raster_task_states_.end()) {

[vmpstr, 2014/02/19 00:49:31]

Can you put the smaller block in if? 

if (state_it == raster_task_state_.end()) {
  // handle image decode
  continue;
}

....

[reveman, 2014/02/19 01:41:13]

Done.

+      RasterTaskState& state = state_it->second;
+
+      DCHECK_EQ(RasterTaskState::SCHEDULED, state.type);
+      DCHECK(state.resource);
+
+      // Balanced with MapPixelRasterBuffer() call in AcquireCanvasForRaster().
+      bool content_has_changed =
+          resource_provider()->UnmapPixelRasterBuffer(state.resource->id());
+
+      if (!content_has_changed) {

[vmpstr, 2014/02/19 00:49:31]

Can you put a comment here explaining what has happened if this is true (ie,
it's solid color).

[reveman, 2014/02/19 01:41:13]

Done.

+        task->WillComplete();
+        task->CompleteOnOriginThread(this);
+        task->DidComplete();
+
+        if (!task->HasFinishedRunning()) {
+          // 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.
+          for (RasterTaskQueue::Item::Vector::const_iterator it =
+                   raster_tasks_.items.begin();
+               it != raster_tasks_.items.end();
+               ++it) {
+            if (it->task == task) {
+              state.type = RasterTaskState::UNSCHEDULED;
+              state.resource = NULL;
+              continue;
+            }
+          }
+        }
+
+        DCHECK(std::find(completed_raster_tasks_.begin(),
+                         completed_raster_tasks_.end(),
+                         task) == completed_raster_tasks_.end());
+        completed_raster_tasks_.push_back(task);
+        state.type = RasterTaskState::COMPLETED;
+        raster_tasks_required_for_activation_.erase(task);
+        continue;
+      }
+
+      DCHECK(task->HasFinishedRunning());
+
+      resource_provider()->BeginSetPixels(state.resource->id());
+      has_performed_uploads_since_last_flush_ = true;
+
+      bytes_pending_upload_ += state.resource->bytes();
+      raster_tasks_with_pending_upload_.push_back(task);
+      state.type = RasterTaskState::UPLOADING;
+      continue;
+    }
+
     task->WillComplete();
     task->CompleteOnOriginThread(this);
     task->DidComplete();
+
+    completed_image_decode_tasks_.push_back(task);
   }
   completed_tasks_.clear();
 }
 
 bool PixelBufferRasterWorkerPool::IsRasterTaskRequiredForActivation(
-    internal::RasterWorkerPoolTask* task) const {
+    internal::WorkerPoolTask* task) const {
   return raster_tasks_required_for_activation_.find(task) !=
          raster_tasks_required_for_activation_.end();
 }