cc: Simplify raster task completion notification logic

cc/resources/pixel_buffer_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/pixel_buffer_raster_worker_pool.h"
 
 #include "base/containers/stack_container.h"
 #include "base/debug/trace_event.h"
 #include "base/values.h"
 #include "cc/debug/traced_value.h"
@@ skipping 232 matching lines @@
 
     task->WillComplete();
     task->CompleteOnOriginThread();
     task->DidComplete();
 
     completed_tasks.pop_front();
   }
 }
 
 void PixelBufferRasterWorkerPool::OnRasterTasksFinished() {
-  // |should_notify_client_if_no_tasks_are_pending_| can be set to false as
-  // a result of a scheduled CheckForCompletedRasterTasks() call. No need to
-  // perform another check in that case as we've already notified the client.
-  if (!should_notify_client_if_no_tasks_are_pending_)
+  if (CheckForCompletedRasterTasks() != ALL_TASKS_COMPLETED)
     return;
 
-  // Call CheckForCompletedRasterTasks() when we've finished running all
-  // raster tasks needed since last time ScheduleTasks() was called.
-  // This reduces latency between the time when all tasks have finished
-  // running and the time when the client is notified.
-  CheckForCompletedRasterTasks();
+  TRACE_EVENT_ASYNC_END0("cc", "ScheduledTasks", this);
+  DCHECK(!HasPendingTasksRequiredForActivation());
+  client()->DidFinishRunningTasks();

[reveman, 2013/12/14 04:31:01]

Hm, what happens if CheckForCompletedRasterTasks() returns TASKS_PENDING because
some uploads are pending? Seems like this client callback is never going to
happen in that case or am I missing something?

What happens in ToT right now is that a periodic check can sometimes result in
us detecting completion and making the client callback before the signal has
been received, right?  I think we can keep the existing code but just make sure
will_notify_client_that_no_tasks_are_pending is not true unless we've received
the ::OnRasterTasksFinished() signal. Wdyt?

[Sami, 2013/12/17 16:27:38]

This shouldn't be any different than before, i.e., DidFinishRunningTasks() will
never get signaled while HasPendingTasks() is true. AFAICT pending uploads don't
factor into that, since they don't get their own worker pool tasks.
Yes, that's exactly the problem I was trying to fix, but perhaps tried to change
things around a little too much. I've now gone for the simpler approach you
suggested.

[reveman, 2013/12/17 20:46:25]

The problem was that you removed the client callback in
CheckForCompletedRasterTasks() which will happend when the uploads complete. Not
a problem in latest patch though.

[Sami, 2013/12/18 12:05:06]

Ah, now I see. I didn't realize pending uploads were also keeping the
CheckForCompletedRasterTasks() tick going.

 }
 
 void PixelBufferRasterWorkerPool::OnRasterTasksRequiredForActivationFinished() {
-  // Analogous to OnRasterTasksFinished(), there's no need to call
-  // CheckForCompletedRasterTasks() if the client has already been notified.
-  if (!should_notify_client_if_no_tasks_required_for_activation_are_pending_)
+  TaskQueueCompletionStatus completion_status = CheckForCompletedRasterTasks();
+  if (completion_status != TASKS_REQUIRED_FOR_ACTIVATION_COMPLETED &&
+      completion_status != ALL_TASKS_COMPLETED)
     return;
 
-  // This reduces latency between the time when all tasks required for
-  // activation have finished running and the time when the client is
-  // notified.
-  CheckForCompletedRasterTasks();
+  DCHECK(std::find_if(raster_tasks_required_for_activation().begin(),
+                      raster_tasks_required_for_activation().end(),
+                      WasCanceled) ==
+        raster_tasks_required_for_activation().end());
+  client()->DidFinishRunningTasksRequiredForActivation();
 }
 
 void PixelBufferRasterWorkerPool::FlushUploads() {
   if (!has_performed_uploads_since_last_flush_)
     return;
 
   resource_provider()->ShallowFlushIfSupported();
   has_performed_uploads_since_last_flush_ = false;
 }
 
@@ skipping 68 matching lines @@
 
     tasks_with_completed_uploads.pop_front();
   }
 }
 
 void PixelBufferRasterWorkerPool::ScheduleCheckForCompletedRasterTasks() {
   if (check_for_completed_raster_tasks_pending_)
     return;
 
   check_for_completed_raster_tasks_callback_.Reset(
-      base::Bind(&PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks,
+      base::Bind(base::IgnoreResult(
+                    &PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks),
                  base::Unretained(this)));
   base::MessageLoopProxy::current()->PostDelayedTask(
       FROM_HERE,
       check_for_completed_raster_tasks_callback_.callback(),
       base::TimeDelta::FromMilliseconds(kCheckForCompletedRasterTasksDelayMs));
   check_for_completed_raster_tasks_pending_ = true;
 }
 
-void PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks() {
+PixelBufferRasterWorkerPool::TaskQueueCompletionStatus
+    PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks() {
   TRACE_EVENT0(
       "cc", "PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks");
 
   DCHECK(should_notify_client_if_no_tasks_are_pending_);
 
   check_for_completed_raster_tasks_callback_.Cancel();
   check_for_completed_raster_tasks_pending_ = false;
 
   RasterWorkerPool::CheckForCompletedTasks();
   CheckForCompletedUploads();
   FlushUploads();
 
-  // Determine what client notifications to generate.
-  bool will_notify_client_that_no_tasks_required_for_activation_are_pending =
-      (should_notify_client_if_no_tasks_required_for_activation_are_pending_ &&
-       !HasPendingTasksRequiredForActivation());
-  bool will_notify_client_that_no_tasks_are_pending =
-      (should_notify_client_if_no_tasks_are_pending_ &&
-       !HasPendingTasks());
-
-  // Adjust the need to generate notifications before scheduling more tasks.
-  should_notify_client_if_no_tasks_required_for_activation_are_pending_ &=
-      !will_notify_client_that_no_tasks_required_for_activation_are_pending;
-  should_notify_client_if_no_tasks_are_pending_ &=
-      !will_notify_client_that_no_tasks_are_pending;
+  TaskQueueCompletionStatus completion_status = TASKS_PENDING;
+  if (!HasPendingTasks())
+    completion_status = ALL_TASKS_COMPLETED;
+  else if (!HasPendingTasksRequiredForActivation())
+    completion_status = TASKS_REQUIRED_FOR_ACTIVATION_COMPLETED;
 
   scheduled_raster_task_count_ = 0;
   if (PendingRasterTaskCount())
     ScheduleMoreTasks();
 
   TRACE_EVENT_ASYNC_STEP_INTO1(
       "cc", "ScheduledTasks", this, StateName(),
       "state", TracedValue::FromValue(StateAsValue().release()));
 
   // Schedule another check for completed raster tasks while there are
   // pending raster tasks or pending uploads.
   if (HasPendingTasks())
     ScheduleCheckForCompletedRasterTasks();
-
-  // Generate client notifications.
-  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()->DidFinishRunningTasksRequiredForActivation();
-  }
-  if (will_notify_client_that_no_tasks_are_pending) {
-    TRACE_EVENT_ASYNC_END0("cc", "ScheduledTasks", this);
-    DCHECK(!HasPendingTasksRequiredForActivation());
-    client()->DidFinishRunningTasks();
-  }
+  return completion_status;
 }
 
 void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
   TRACE_EVENT0("cc", "PixelBufferRasterWorkerPool::ScheduleMoreTasks");
 
   enum RasterTaskType {
     PREPAINT_TYPE = 0,
     REQUIRED_FOR_ACTIVATION_TYPE = 1,
     NUM_TYPES = 2
   };
@@ skipping 232 matching lines @@
 
   throttle_state->SetInteger("bytes_available_for_upload",
                              max_bytes_pending_upload_ - bytes_pending_upload_);
   throttle_state->SetInteger("bytes_pending_upload", bytes_pending_upload_);
   throttle_state->SetInteger("scheduled_raster_task_count",
                              scheduled_raster_task_count_);
   return throttle_state.PassAs<base::Value>();
 }
 
 }  // namespace cc