cc: Add SchedulerStateMachine::DidDraw and use for forced draws

cc/scheduler/scheduler_state_machine.cc

Index: cc/scheduler/scheduler_state_machine.cc
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index 01ac8cbb1cc2be98421dd455cabc1b330cd9c623..e1cbaa4a4c4d549dc982b214d613d1694d8e3137 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -648,22 +648,69 @@ void SchedulerStateMachine::WillActivate() {
   needs_redraw_ = true;
 }
 
-void SchedulerStateMachine::WillDraw(bool did_request_swap) {
+void SchedulerStateMachine::WillDraw() {
+  // We need to reset needs_redraw_ before we draw since the
+  // draw itself might request another draw.
+  needs_redraw_ = false;
+}
+
+void SchedulerStateMachine::DidDraw(bool did_request_swap,
+                                    DrawResult draw_result) {
+  active_tree_needs_first_draw_ = false;
+
   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)
     forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
 
   if (begin_main_frame_state_ == BEGIN_MAIN_FRAME_STATE_WAITING_FOR_DRAW)
     begin_main_frame_state_ = BEGIN_MAIN_FRAME_STATE_IDLE;
 
-  needs_redraw_ = false;
-  active_tree_needs_first_draw_ = false;
-
   if (did_request_swap) {

[sunnyps, 2015/11/20 23:34:07]

Doesn't draw_result != DRAW_SUCCESS imply that did_request_swap is false and
that it's true otherwise?

[brianderson, 2015/11/21 00:31:33]

It's a bit confusing, and probably deserves some cleaning up:

There's "did request swap", which is true whether or not DrawAndSwap returns
early because of no damage, and "did swap", which is only true when there is
damage and we swap.

Also, we get DRAW_SUCCESS even if the draw returns early because of no damage:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Let me see what I can do to make it less confusing.

     DCHECK(!request_swap_funnel_);
     request_swap_funnel_ = true;
     did_request_swap_in_last_frame_ = true;
     last_frame_number_swap_requested_ = current_frame_number_;
   }
+
+  switch (draw_result) {
+    case INVALID_RESULT:
+      NOTREACHED() << "Uninitialized DrawResult.";
+      break;
+    case DRAW_ABORTED_CANT_DRAW:
+    case DRAW_ABORTED_CONTEXT_LOST:
+      NOTREACHED() << "Invalid return value from DrawAndSwapIfPossible:"
+                   << draw_result;
+      break;
+    case DRAW_SUCCESS:
+      consecutive_checkerboard_animations_ = 0;
+      forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
+      break;
+    case DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
+      needs_redraw_ = true;
+
+      // If we're already in the middle of a redraw, we don't need to
+      // restart it.
+      if (forced_redraw_state_ != FORCED_REDRAW_STATE_IDLE)
+        return;
+
+      needs_begin_main_frame_ = true;
+      consecutive_checkerboard_animations_++;

[sunnyps, 2015/11/20 23:34:07]

nit: needs_begin_main_frame should be inside the if (settings_...) block and
consecutive_checkerboard_animations_ should be incremented above the if
(forced_redraw_state_...) block. See the next comment.

[brianderson, 2015/11/21 00:31:33]

Sg.

+      if (settings_.timeout_and_draw_when_animation_checkerboards &&
+          consecutive_checkerboard_animations_ >=
+              settings_.maximum_number_of_failed_draws_before_draw_is_forced) {
+        consecutive_checkerboard_animations_ = 0;

[sunnyps, 2015/11/20 23:34:07]

nit: We shouldn't have to set consecutive_checkerboard_animations_ here. Setting
forced_redraw_state_ is enough. Having the correct value for
consecutive_checkerboard_animations_ might be useful for UMA.

[brianderson, 2015/11/21 00:31:33]

Also sg.

+        // We need to force a draw, but it doesn't make sense to do this until
+        // we've committed and have new textures.
+        forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
+      }
+      break;
+    case DRAW_ABORTED_MISSING_HIGH_RES_CONTENT:
+      // It's not clear whether this missing content is because of missing
+      // pictures (which requires a commit) or because of memory pressure
+      // removing textures (which might not).  To be safe, request a commit
+      // anyway.
+      needs_begin_main_frame_ = true;
+      break;
+  }
 }
 
 void SchedulerStateMachine::WillPrepareTiles() {
@@ -983,49 +1030,6 @@ void SchedulerStateMachine::SetImplLatencyTakesPriority(
   impl_latency_takes_priority_ = impl_latency_takes_priority;
 }
 
-void SchedulerStateMachine::DidDrawIfPossibleCompleted(DrawResult result) {
-  switch (result) {
-    case INVALID_RESULT:
-      NOTREACHED() << "Uninitialized DrawResult.";
-      break;
-    case DRAW_ABORTED_CANT_DRAW:
-    case DRAW_ABORTED_CONTEXT_LOST:
-      NOTREACHED() << "Invalid return value from DrawAndSwapIfPossible:"
-                   << result;
-      break;
-    case DRAW_SUCCESS:
-      consecutive_checkerboard_animations_ = 0;
-      forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
-      break;
-    case DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
-      needs_redraw_ = true;
-
-      // If we're already in the middle of a redraw, we don't need to
-      // restart it.
-      if (forced_redraw_state_ != FORCED_REDRAW_STATE_IDLE)
-        return;
-
-      needs_begin_main_frame_ = true;
-      consecutive_checkerboard_animations_++;
-      if (settings_.timeout_and_draw_when_animation_checkerboards &&
-          consecutive_checkerboard_animations_ >=
-              settings_.maximum_number_of_failed_draws_before_draw_is_forced) {
-        consecutive_checkerboard_animations_ = 0;
-        // We need to force a draw, but it doesn't make sense to do this until
-        // we've committed and have new textures.
-        forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
-      }
-      break;
-    case DRAW_ABORTED_MISSING_HIGH_RES_CONTENT:
-      // It's not clear whether this missing content is because of missing
-      // pictures (which requires a commit) or because of memory pressure
-      // removing textures (which might not).  To be safe, request a commit
-      // anyway.
-      needs_begin_main_frame_ = true;
-      break;
-  }
-}
-
 void SchedulerStateMachine::SetNeedsBeginMainFrame() {
   needs_begin_main_frame_ = true;
 }