cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

cc/tiles/tile_manager.cc

Index: cc/tiles/tile_manager.cc
diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc
index 58fc640d8ddabec0022996ae04e2099c2d801543..74d12ef0c9ca64b8c10f1c1faf3eb1cf005171e9 100644
--- a/cc/tiles/tile_manager.cc
+++ b/cc/tiles/tile_manager.cc
@@ -43,7 +43,8 @@ DEFINE_SCOPED_UMA_HISTOGRAM_AREA_TIMER(
 
 class RasterTaskImpl : public TileTask {

[vmpstr, 2016/05/21 00:02:14]

We might want to restructure this task a bit. Specifically, since we're passing
a tile, we can have a private struct like ThreadSafeState and just pull out all
of the relevant information from the tile into ThreadSafeState at construction
time. Then, it's clear what can and cannot be accessed in RunOnWorkerThread and
we pass far fewer parameters into RasterTaskImpl...

Can you leave a TODO here?

[prashant.n, 2016/05/21 00:33:53]

Yes. I'll create two structures one for running task and one for processing
completion in next patch. For this I'll leave the comment in current patch.

  public:
-  RasterTaskImpl(const Resource* resource,
+  RasterTaskImpl(TileManager* tile_manager,
+                 Resource* resource,
                  scoped_refptr<RasterSource> raster_source,
                  const gfx::Rect& content_rect,
                  const gfx::Rect& invalid_content_rect,
@@ -52,15 +53,16 @@ class RasterTaskImpl : public TileTask {
                  TileResolution tile_resolution,
                  int layer_id,
                  uint64_t source_prepare_tiles_id,
-                 const void* tile,
+                 Tile* tile,
                  uint64_t new_content_id,
                  uint64_t previous_content_id,
                  uint64_t resource_content_id,
                  int source_frame_number,
-                 const base::Callback<void(bool)>& reply,
+                 std::unique_ptr<RasterBuffer> raster_buffer,
                  TileTask::Vector* dependencies,
                  bool supports_concurrent_execution)
       : TileTask(supports_concurrent_execution, dependencies),
+        tile_manager_(tile_manager),
         resource_(resource),
         raster_source_(std::move(raster_source)),
         content_rect_(content_rect),
@@ -75,7 +77,7 @@ class RasterTaskImpl : public TileTask {
         previous_content_id_(previous_content_id),
         resource_content_id_(resource_content_id),
         source_frame_number_(source_frame_number),
-        reply_(reply) {}
+        raster_buffer_(std::move(raster_buffer)) {}
 
   // Overridden from Task:
   void RunOnWorkerThread() override {
@@ -98,21 +100,17 @@ class RasterTaskImpl : public TileTask {
   }
 
   // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {
-    DCHECK(!raster_buffer_);
-    raster_buffer_ = provider->AcquireBufferForRaster(
-        resource_, resource_content_id_, previous_content_id_);
-  }
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {
-    provider->ReleaseBufferForRaster(std::move(raster_buffer_));
-    reply_.Run(!state().IsFinished());
+  void OnTaskCompleted() override {
+    tile_manager_->OnRasterTaskCompleted(std::move(raster_buffer_), tile_,
+                                         resource_, state().IsCanceled());
   }
 
  protected:
   ~RasterTaskImpl() override { DCHECK(!raster_buffer_); }
 
  private:
-  const Resource* resource_;
+  TileManager* tile_manager_;
+  Resource* resource_;
   scoped_refptr<RasterSource> raster_source_;
   gfx::Rect content_rect_;
   gfx::Rect invalid_content_rect_;
@@ -121,12 +119,11 @@ class RasterTaskImpl : public TileTask {
   TileResolution tile_resolution_;
   int layer_id_;
   uint64_t source_prepare_tiles_id_;
-  const void* tile_;
+  Tile* tile_;

[vmpstr, 2016/05/21 00:02:14]

The reason we had this as a void* is that it would only be used as an id for
tracing. That is, we have to be careful not to be accessing tiles from different
threads. I think the patch you have now is fine, but can you leave a comment
here to say that tile object should not be accessed in any function that runs on
a different thread (it's not a thread safe object)

[prashant.n, 2016/05/21 00:33:53]

Yes.

I was thinking to add clousres here one for running and one for completing. But
passing members is better than having closures.

   uint64_t new_content_id_;
   uint64_t previous_content_id_;
   uint64_t resource_content_id_;
   int source_frame_number_;
-  const base::Callback<void(bool)> reply_;
   std::unique_ptr<RasterBuffer> raster_buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterTaskImpl);
@@ -188,7 +185,7 @@ void InsertNodeForDecodeTask(TaskGraph* graph,
   if (task->dependencies().size()) {
     DCHECK_EQ(task->dependencies().size(), 1u);
     auto* dependency = task->dependencies()[0].get();
-    if (!dependency->HasCompleted()) {
+    if (!dependency->state().IsFinished()) {
       InsertNodeForDecodeTask(graph, dependency, use_foreground_category,
                               priority);
       graph->edges.push_back(TaskGraph::Edge(dependency, task));
@@ -213,7 +210,7 @@ void InsertNodesForRasterTask(TaskGraph* graph,
     TileTask* decode_task = it->get();
 
     // Skip if already decoded.
-    if (decode_task->HasCompleted())
+    if (decode_task->state().IsFinished())
       continue;
 
     dependencies++;
@@ -251,6 +248,14 @@ void InsertNodesForRasterTask(TaskGraph* graph,
       dependencies);
 }
 
+bool CheckDependenciesForFinishedRasterTask(TileTask* raster_task) {

[vmpstr, 2016/05/21 00:02:14]

Our pattern is usually:

void CheckFoo(Bar* bar) {
#if DCHECK_IS_ON()
  for (...)
    ...
#endif
}

and then in the calling code:

CheckFoo(bar);

Also can you move this function after the class below?

[prashant.n, 2016/05/21 00:33:53]

Yes.

+  // If there is any dependency which is not finished, add a DCHECK for it.
+  for (auto& decode_task : raster_task->dependencies())
+    DCHECK(decode_task->state().IsFinished());
+
+  return true;
+}
+
 class TaskSetFinishedTaskImpl : public TileTask {
  public:
   explicit TaskSetFinishedTaskImpl(
@@ -266,9 +271,7 @@ class TaskSetFinishedTaskImpl : public TileTask {
     TaskSetFinished();
   }
 
-  // Overridden from TileTask:
-  void ScheduleOnOriginThread(RasterBufferProvider* provider) override {}
-  void CompleteOnOriginThread(RasterBufferProvider* provider) override {}
+  void OnTaskCompleted() override{};
 
  protected:
   ~TaskSetFinishedTaskImpl() override {}
@@ -508,7 +511,6 @@ void TileManager::Flush() {
   }
 
   tile_task_manager_->CheckForCompletedTasks();
-
   did_check_for_completed_tasks_since_last_schedule_tasks_ = true;
 
   TRACE_EVENT_INSTANT1("cc", "DidFlush", TRACE_EVENT_SCOPE_THREAD, "stats",
@@ -795,7 +797,13 @@ void TileManager::ScheduleTasks(
       tile->raster_task_ = CreateRasterTask(prioritized_tile);
 
     TileTask* task = tile->raster_task_.get();
-    DCHECK(!task->HasCompleted());
+
+    // Skip already finished raster task. Finished raster task should have all
+    // its dependencies finished.
+    if (task->state().IsFinished()) {

[vmpstr, 2016/05/21 00:02:14]

In what situation can we be getting a finished task here? Also, is cancelled a
type of finished or does finish imply it ran to completion?

[prashant.n, 2016/05/21 00:33:53]

See crbug.com/613529 for more details. I'll modify the comment to depict the
situation.
Only finished state implies to it ran to completion. The canceled state implies
it completed as canceled. The task task is modified to canceled state in
TaskGraphWorkQueue::ScheduleTasks(). So this won't arise but I'll check further
- canceled task must be collected before next scheduling happens, so that if
tile which had task canceled should get new raster task.

+      DCHECK(CheckDependenciesForFinishedRasterTask(task));
+      continue;
+    }
 
     if (tile->required_for_activation()) {
       required_for_activate_count++;
@@ -915,25 +923,28 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
   }
 
   bool supports_concurrent_execution = !use_gpu_rasterization_;
+  std::unique_ptr<RasterBuffer> raster_buffer =
+      tile_task_manager_->GetRasterBufferProvider()->AcquireBufferForRaster(
+          resource, resource_content_id, tile->invalidated_id());
   return make_scoped_refptr(new RasterTaskImpl(
-      resource, prioritized_tile.raster_source(), tile->content_rect(),
+      this, resource, prioritized_tile.raster_source(), tile->content_rect(),
       tile->invalidated_content_rect(), tile->contents_scale(),
       playback_settings, prioritized_tile.priority().resolution,
-      tile->layer_id(), prepare_tiles_count_, static_cast<const void*>(tile),
-      tile->id(), tile->invalidated_id(), resource_content_id,
-      tile->source_frame_number(),
-      base::Bind(&TileManager::OnRasterTaskCompleted, base::Unretained(this),
-                 tile->id(), resource),
-      &decode_tasks, supports_concurrent_execution));
+      tile->layer_id(), prepare_tiles_count_, tile, tile->id(),
+      tile->invalidated_id(), resource_content_id, tile->source_frame_number(),
+      std::move(raster_buffer), &decode_tasks, supports_concurrent_execution));
 }
 
 void TileManager::OnRasterTaskCompleted(
-    Tile::Id tile_id,
+    std::unique_ptr<RasterBuffer> raster_buffer,
+    Tile* tile,
     Resource* resource,
     bool was_canceled) {
-  DCHECK(tiles_.find(tile_id) != tiles_.end());
+  DCHECK(tile);
+  DCHECK(tiles_.find(tile->id()) != tiles_.end());
+  tile_task_manager_->GetRasterBufferProvider()->ReleaseBufferForRaster(
+      std::move(raster_buffer));
 
-  Tile* tile = tiles_[tile_id];
   TileDrawInfo& draw_info = tile->draw_info();
   DCHECK(tile->raster_task_.get());
   orphan_tasks_.push_back(tile->raster_task_);