cc: Add new RasterWorkerPool interface.

cc/resources/image_raster_worker_pool.cc

+// 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.
+
+#include "cc/resources/image_raster_worker_pool.h"
+
+#include "cc/resources/resource.h"
+
+namespace cc {
+
+namespace {
+
+class RootWorkerPoolTaskImpl : public internal::WorkerPoolTask {

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

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.

[reveman, 2013/05/29 19:39:12]

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

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

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.

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

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.

+ public:
+  explicit RootWorkerPoolTaskImpl(
+      internal::WorkerPoolTask::TaskVector* dependencies)
+      : internal::WorkerPoolTask(dependencies) {
+  }
+
+  // Overridden from internal::WorkerPoolTask:
+  virtual void RunOnThread(unsigned thread_index) OVERRIDE {}
+  virtual void DispatchCompletionCallback() OVERRIDE {}
+
+ private:
+  virtual ~RootWorkerPoolTaskImpl() {}
+};
+
+class ImageWorkerPoolTaskImpl : public internal::WorkerPoolTask {
+ public:
+  // First callback parameter is true when task was canceled. Second is
+  // true when bind is needed.
+  typedef base::Callback<void(bool, bool)> Reply;
+
+  ImageWorkerPoolTaskImpl(internal::RasterWorkerPoolTask* task,
+                          TaskVector* dependencies,
+                          uint8_t* buffer,
+                          const Reply& reply)
+      : internal::WorkerPoolTask(dependencies),
+        task_(task),
+        buffer_(buffer),
+        reply_(reply),
+        need_bind_(false) {
+  }
+
+  // Overridden from internal::WorkerPoolTask:
+  virtual void RunOnThread(unsigned thread_index) OVERRIDE {
+    need_bind_ = task_->RunOnThread(buffer_, thread_index);
+  }
+  virtual void DispatchCompletionCallback() OVERRIDE {

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

nit: space between methods

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

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?

+    DCHECK(need_bind_ || !HasFinishedRunning());
+    reply_.Run(!HasFinishedRunning(), need_bind_);
+  }
+
+ private:
+  virtual ~ImageWorkerPoolTaskImpl() {}
+
+  scoped_refptr<internal::RasterWorkerPoolTask> task_;
+  uint8_t* buffer_;
+  const Reply reply_;
+  bool need_bind_;
+};
+
+}  // namespace
+
+ImageRasterWorkerPool::ImageRasterWorkerPool(
+    ResourceProvider* resource_provider, size_t num_threads)
+    : RasterWorkerPool(resource_provider, num_threads) {
+  // TODO(reveman): Remove WorkerPool client interface.
+  WorkerPool::SetClient(this);
+}
+
+ImageRasterWorkerPool::~ImageRasterWorkerPool() {
+}
+
+void ImageRasterWorkerPool::Shutdown() {
+  RasterWorkerPool::Shutdown();
+}
+
+void ImageRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
+  internal::WorkerPoolTask::TaskVector tasks;
+
+  RasterWorkerPool::SetRasterTasks(queue);
+
+  for (RasterTask::Queue::TaskVector::iterator it = raster_tasks_.begin();

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

do we support for-each syntax for Chromium?, i.e. would

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

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

c++0x syntax not allowed afaik.

+       it != raster_tasks_.end(); ++it) {
+    internal::RasterWorkerPoolTask* task = *it;
+
+    // Acquire image for resource.
+    resource_provider_->AcquireImage(task->resource()->id());
+
+    // Map image for raster.
+    uint8* buffer = resource_provider_->MapImage(task->resource()->id());
+
+    // TODO(reveman): Avoid having to make a copy of dependencies.
+    internal::WorkerPoolTask::TaskVector dependencies = task->dependencies();
+    scoped_refptr<internal::WorkerPoolTask> image_task(
+        new ImageWorkerPoolTaskImpl(
+            task,
+            &dependencies,
+            buffer,
+            base::Bind(&ImageRasterWorkerPool::OnRasterTaskCompleted,
+                       base::Unretained(this),
+                       make_scoped_refptr(task))));
+
+    tasks.push_back(image_task);
+  }
+
+  if (tasks.empty()) {
+    WorkerPool::ScheduleTasks(NULL);
+    return;
+  }
+
+  scoped_refptr<RootWorkerPoolTaskImpl> root(
+      new RootWorkerPoolTaskImpl(&tasks));
+  WorkerPool::ScheduleTasks(root);
+}
+
+void ImageRasterWorkerPool::
+    DidFinishDispatchingWorkerPoolCompletionCallbacks() {
+  client_->DidFinishDispatchingRasterWorkerPoolCompletionCallbacks();
+}
+
+void ImageRasterWorkerPool::OnRasterTaskCompleted(
+    scoped_refptr<internal::RasterWorkerPoolTask> task,
+    bool was_canceled,
+    bool need_bind) {
+  // Balanced with MapImage() call in ScheduleTasks().
+  resource_provider_->UnmapImage(task->resource()->id());
+
+  if (need_bind)

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

Should we just unconditionally bind when we acquire the image? Why do we need
the if?

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

Good point. I removed it from the latest patch.

+    resource_provider_->BindImage(task->resource()->id());
+
+  if (!was_canceled)
+    task->DidRun();
+
+  DidFinishRasterTask(task);
+}
+
+void ImageRasterWorkerPool::DidFinishRasterTask(
+    internal::RasterWorkerPoolTask* task) {
+  task->DidComplete();
+  task->DispatchCompletionCallback();
+}
+
+}  // namespace cc