Re-land: cc: Add raster finished signals to RasterWorkerPool.

cc/resources/image_raster_worker_pool.cc

Index: cc/resources/image_raster_worker_pool.cc
diff --git a/cc/resources/image_raster_worker_pool.cc b/cc/resources/image_raster_worker_pool.cc
index 1d96b27815e8e1e6d78407a8200b6666a0bead47..24eecb7a85b388ed8703bc8fbb7e50688ebf6ee0 100644
--- a/cc/resources/image_raster_worker_pool.cc
+++ b/cc/resources/image_raster_worker_pool.cc
@@ -5,6 +5,7 @@
 #include "cc/resources/image_raster_worker_pool.h"
 
 #include "base/debug/trace_event.h"
+#include "cc/base/scoped_ptr_deque.h"
 #include "cc/resources/resource.h"
 #include "third_party/skia/include/core/SkDevice.h"
 
@@ -28,6 +29,7 @@ class ImageWorkerPoolTaskImpl : public internal::WorkerPoolTask {
 
   // Overridden from internal::WorkerPoolTask:
   virtual void RunOnWorkerThread(unsigned thread_index) OVERRIDE {
+    TRACE_EVENT0("cc", "ImageWorkerPoolTaskImpl::RunOnWorkerThread");
     if (!buffer_)
       return;
 
@@ -55,31 +57,81 @@ class ImageWorkerPoolTaskImpl : public internal::WorkerPoolTask {
   DISALLOW_COPY_AND_ASSIGN(ImageWorkerPoolTaskImpl);
 };
 
+class RasterFinishedWorkerPoolTaskImpl : public internal::WorkerPoolTask {
+ public:
+  RasterFinishedWorkerPoolTaskImpl(
+      base::MessageLoopProxy* origin_loop,
+      const base::Closure& on_raster_finished_callback)
+      : origin_loop_(origin_loop),
+        on_raster_finished_callback_(on_raster_finished_callback) {
+  }
+
+  // Overridden from internal::WorkerPoolTask:
+  virtual void RunOnWorkerThread(unsigned thread_index) OVERRIDE {
+    TRACE_EVENT0("cc", "RasterFinishedWorkerPoolTaskImpl::RunOnWorkerThread");
+    origin_loop_->PostTask(FROM_HERE, on_raster_finished_callback_);
+  }
+  virtual void CompleteOnOriginThread() OVERRIDE {}
+
+ private:
+  virtual ~RasterFinishedWorkerPoolTaskImpl() {}
+
+  scoped_refptr<base::MessageLoopProxy> origin_loop_;
+  const base::Closure on_raster_finished_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(RasterFinishedWorkerPoolTaskImpl);
+};
+
 }  // namespace
 
 ImageRasterWorkerPool::ImageRasterWorkerPool(
     ResourceProvider* resource_provider, size_t num_threads)
-    : RasterWorkerPool(resource_provider, num_threads) {
+    : RasterWorkerPool(resource_provider, num_threads),
+      weak_ptr_factory_(this),
+      schedule_raster_tasks_count_(0) {
 }
 
 ImageRasterWorkerPool::~ImageRasterWorkerPool() {
   DCHECK_EQ(0u, image_tasks_.size());
 }
 
+void ImageRasterWorkerPool::Shutdown() {
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  RasterWorkerPool::Shutdown();
+}
+
 void ImageRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
   TRACE_EVENT0("cc", "ImageRasterWorkerPool::ScheduleTasks");
 
   RasterWorkerPool::SetRasterTasks(queue);
 
-  RasterTaskGraph graph;
+  ++schedule_raster_tasks_count_;
+
+  enum RasterTaskType {
+    PREPAINT_TYPE = 0,
+    REQUIRED_FOR_ACTIVATION_TYPE = 1,
+    NUM_TYPES = 2
+  };
+  ScopedPtrDeque<GraphNode> tasks[NUM_TYPES];
+  unsigned priority = 2u;  // 0-1 reserved for RasterFinished tasks.
+  TaskGraph graph;
+
   for (RasterTaskVector::const_iterator it = raster_tasks().begin();
        it != raster_tasks().end(); ++it) {
     internal::RasterWorkerPoolTask* task = it->get();
 
+    RasterTaskType type = IsRasterTaskRequiredForActivation(task) ?
+        REQUIRED_FOR_ACTIVATION_TYPE :
+        PREPAINT_TYPE;
+
     TaskMap::iterator image_it = image_tasks_.find(task);
     if (image_it != image_tasks_.end()) {
       internal::WorkerPoolTask* image_task = image_it->second.get();
-      graph.InsertRasterTask(image_task, task->dependencies());
+      tasks[type].push_back(
+          CreateRasterTaskGraphNode(image_task,
+                                    task->dependencies(),
+                                    priority++,
+                                    &graph));
       continue;
     }
 
@@ -99,10 +151,86 @@ void ImageRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
                        base::Unretained(this),
                        make_scoped_refptr(task))));
     image_tasks_[task] = new_image_task;
-    graph.InsertRasterTask(new_image_task.get(), task->dependencies());
+    tasks[type].push_back(CreateRasterTaskGraphNode(new_image_task.get(),
+                                                    task->dependencies(),
+                                                    priority++,
+                                                    &graph));
+  }
+
+  scoped_refptr<internal::WorkerPoolTask> new_raster_finished_task(

[vmpstr, 2013/06/27 23:31:21]

Can you either add some comments in the code from here to the end of the
function, or possibly move this to a different function since it kind of seems
like wrap-up code (we scheduled everything.. now we just need to tie everything
together into a single graph). 

+      new RasterFinishedWorkerPoolTaskImpl(
+          base::MessageLoopProxy::current(),
+          base::Bind(&ImageRasterWorkerPool::OnRasterFinished,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     schedule_raster_tasks_count_)));
+  scoped_ptr<GraphNode> raster_finished_node(
+      new GraphNode(new_raster_finished_task.get(),
+                    1u));  // Priority 1
+  for (unsigned i = 0; i < NUM_TYPES; ++i) {

[vmpstr, 2013/06/27 23:31:21]

nit: type (to be consistent with the loop above)

[reveman, 2013/06/28 17:26:07]

Done.

+    for (unsigned j = 0; j < tasks[i].size(); ++j) {

[vmpstr, 2013/06/27 23:31:21]

This can be a small anonymous namespace function like AddDependency(tasks[i],
raster_finished_node). You can reuse that in the loop below for
AddDependency(tasks[REQUIRED_FOR_ACTIVATION_TYPE],
raster_required_for_activation_node).

What I'm really trying to say is that there's quite a bit of replication of code
in this block. I would prefer that we try to reduce that a bit in this patch,
but we can also save some of the work for a follow-up.

[reveman, 2013/06/28 17:26:07]

Latest patch reduces code duplication significantly. PTAL.

+      raster_finished_node->add_dependency();
+      tasks[i][j]->add_dependent(raster_finished_node.get());
+    }
+  }
+  graph.set(new_raster_finished_task.get(), raster_finished_node.Pass());
+
+  scoped_refptr<internal::WorkerPoolTask>
+      new_raster_required_for_activation_finished_task(
+          new RasterFinishedWorkerPoolTaskImpl(
+              base::MessageLoopProxy::current(),
+              base::Bind(&ImageRasterWorkerPool::
+                         OnRasterRequiredForActivationFinished,
+                         weak_ptr_factory_.GetWeakPtr(),
+                         schedule_raster_tasks_count_)));
+  scoped_ptr<GraphNode> raster_required_for_activation_finished_node(
+      new GraphNode(new_raster_required_for_activation_finished_task.get(),
+                    0u));  // Priority 0
+  for (unsigned j = 0;
+       j < tasks[REQUIRED_FOR_ACTIVATION_TYPE].size();
+       ++j) {
+    raster_required_for_activation_finished_node->add_dependency();
+    tasks[REQUIRED_FOR_ACTIVATION_TYPE][j]->add_dependent(
+        raster_required_for_activation_finished_node.get());
   }
+  graph.set(new_raster_required_for_activation_finished_task.get(),
+            raster_required_for_activation_finished_node.Pass());
+
+  for (unsigned i = 0; i < NUM_TYPES; ++i) {

[vmpstr, 2013/06/27 23:31:21]

I wonder if it's better to have tasks[i][j] as a raw pointer and insert into the
graph as we go, and just save tasks for easy updates; this loop looks like it's
begging to be removed, but I might be missing some corner case

[reveman, 2013/06/28 17:26:07]

Done.

+    while (!tasks[i].empty()) {
+      scoped_ptr<GraphNode> node = tasks[i].take_front();
+      internal::WorkerPoolTask* task = node->task();
+      graph.set(task, node.Pass());
+    }
+  }
+
+  SetTaskGraph(&graph);
+
+  raster_finished_task_.swap(new_raster_finished_task);
+  raster_required_for_activation_finished_task_.swap(
+      new_raster_required_for_activation_finished_task);
+}
+
+void ImageRasterWorkerPool::OnRasterFinished(
+    int64 schedule_raster_tasks_count) {
+  TRACE_EVENT1("cc", "ImageRasterWorkerPool::OnRasterFinished",
+               "schedule_raster_tasks_count", schedule_raster_tasks_count);
+  DCHECK_GE(schedule_raster_tasks_count_, schedule_raster_tasks_count);
+  // Call DidFinishedRunningTasks() if this callback is

[vmpstr, 2013/06/27 23:31:21]

nit: blank line above this, please

[reveman, 2013/06/28 17:26:07]

code doesn't exist in latest patch.

+  // associated with the last call to SetRasterTaskGraph().
+  if (schedule_raster_tasks_count_ == schedule_raster_tasks_count)
+    client()->DidFinishedRunningTasks();
+}
 
-  SetRasterTaskGraph(&graph);
+void ImageRasterWorkerPool::OnRasterRequiredForActivationFinished(
+    int64 schedule_raster_tasks_count) {
+  TRACE_EVENT1("cc",
+               "ImageRasterWorkerPool::OnRasterRequiredForActivationFinished",
+               "schedule_raster_tasks_count", schedule_raster_tasks_count);
+  DCHECK_GE(schedule_raster_tasks_count_, schedule_raster_tasks_count);
+  // Call DidFinishedRunningTasksRequiredForActivation() if this callback is

[vmpstr, 2013/06/27 23:31:21]

nit: blank line above this, please

[reveman, 2013/06/28 17:26:07]

code doesn't exist in latest patch.

+  // associated with the last call to SetRasterTaskGraph().
+  if (schedule_raster_tasks_count_ == schedule_raster_tasks_count)
+    client()->DidFinishedRunningTasksRequiredForActivation();
 }
 
 void ImageRasterWorkerPool::OnRasterTaskCompleted(