cc: Unify use of DidSwapBuffers() and did_request_swap

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 647af71e27b91ebf66a6487ab78e7fe1c6470329..91e1c576508d522b71e29b47eca63b359f187bfb 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -24,6 +24,7 @@ SchedulerStateMachine::SchedulerStateMachine(const SchedulerSettings& settings)
       current_frame_number_(0),
       last_frame_number_swap_performed_(-1),
       last_frame_number_begin_main_frame_sent_(-1),
+      last_frame_number_draw_request_sent_(-1),
       last_frame_number_update_visible_tiles_was_called_(-1),
       manage_tiles_funnel_(0),
       consecutive_checkerboard_animations_(0),
@@ -222,6 +223,9 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
       "last_frame_number_begin_main_frame_sent",
       last_frame_number_begin_main_frame_sent_);
   minor_state->SetInteger(
+      "last_frame_number_draw_request_sent",
+      last_frame_number_draw_request_sent_);
+  minor_state->SetInteger(
       "last_frame_number_update_visible_tiles_was_called",
       last_frame_number_update_visible_tiles_was_called_);
 
@@ -286,6 +290,10 @@ bool SchedulerStateMachine::HasSwappedThisFrame() const {
   return current_frame_number_ == last_frame_number_swap_performed_;
 }
 
+bool SchedulerStateMachine::HasSentDrawRequestThisFrame() const {
+  return current_frame_number_ == last_frame_number_draw_request_sent_;
+}
+
 bool SchedulerStateMachine::PendingDrawsShouldBeAborted() const {
   // These are all the cases where we normally cannot or do not want to draw
   // but, if needs_redraw_ is true and we do not draw to make forward progress,
@@ -364,8 +372,8 @@ bool SchedulerStateMachine::ShouldDraw() const {
   if (PendingDrawsShouldBeAborted())
     return active_tree_needs_first_draw_;
 
-  // After this line, we only want to swap once per frame.
-  if (HasSwappedThisFrame())
+  // After this line, we only want to send draw request once per frame.
+  if (HasSentDrawRequestThisFrame())
     return false;
 
   // Do not queue too many swaps.
@@ -593,16 +601,10 @@ void SchedulerStateMachine::UpdateState(Action action) {
     }
 
     case ACTION_DRAW_AND_SWAP_FORCED:
-    case ACTION_DRAW_AND_SWAP_IF_POSSIBLE: {
-      bool did_request_swap = true;
-      UpdateStateOnDraw(did_request_swap);
-      return;
-    }
-
+    case ACTION_DRAW_AND_SWAP_IF_POSSIBLE:
     case ACTION_DRAW_AND_SWAP_ABORT:
     case ACTION_DRAW_AND_READBACK: {
-      bool did_request_swap = false;
-      UpdateStateOnDraw(did_request_swap);
+      UpdateStateOnDraw();
       return;
     }
 
@@ -739,7 +741,7 @@ void SchedulerStateMachine::UpdateStateOnActivation() {
   needs_redraw_ = true;
 }
 
-void SchedulerStateMachine::UpdateStateOnDraw(bool did_request_swap) {
+void SchedulerStateMachine::UpdateStateOnDraw() {
   DCHECK(readback_state_ != READBACK_STATE_WAITING_FOR_REPLACEMENT_COMMIT &&
          readback_state_ != READBACK_STATE_WAITING_FOR_REPLACEMENT_ACTIVATION)
       << *AsValue();
@@ -763,9 +765,7 @@ void SchedulerStateMachine::UpdateStateOnDraw(bool did_request_swap) {
   needs_redraw_ = false;
   draw_if_possible_failed_ = false;
   active_tree_needs_first_draw_ = false;
-
-  if (did_request_swap)
-    last_frame_number_swap_performed_ = current_frame_number_;
+  last_frame_number_draw_request_sent_ = current_frame_number_;

[brianderson, 2014/05/02 04:08:58]

Hmmm. This would mean that a readback, which didn't swap, could prevent the
replacement commit from drawing until the next BeginFrame.

Maybe we do need to keep separate concepts in the scheduler of swap_requested
vs. swap_performed, at least for this method. Sorry I didn't realize this when I
reviewed the patch last time.

[simonhong, 2014/05/08 01:14:57]

Yep. I created last_frame_number_swap_request_sent_ and checks it in the
ShouldDraw().

 }
 
 void SchedulerStateMachine::UpdateStateOnManageTiles() {
@@ -875,7 +875,7 @@ bool SchedulerStateMachine::ProactiveBeginFrameWanted() const {
   // SetNeedsBeginFrame requests, which may propagate to the BeginImplFrame
   // provider and get sampled at an inopportune time, delaying the next
   // BeginImplFrame.
-  if (last_frame_number_swap_performed_ == current_frame_number_)
+  if (HasSwappedThisFrame())

[brianderson, 2014/05/02 04:08:58]

Probably want to use HasSentDrawRequestThisFrame() instead since we'll want to
stay active even if a swap had an early out or failed.

[simonhong, 2014/05/08 01:14:57]

Done.

     return true;
 
   return false;
@@ -956,7 +956,7 @@ bool SchedulerStateMachine::MainThreadIsInHighLatencyMode() const {
 
   // If we just sent a BeginMainFrame and haven't hit the deadline yet, the main
   // thread is in a low latency mode.
-  if (last_frame_number_begin_main_frame_sent_ == current_frame_number_ &&
+  if (HasSentBeginMainFrameThisFrame() &&
       (begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING ||
        begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME))
     return false;
@@ -964,9 +964,7 @@ bool SchedulerStateMachine::MainThreadIsInHighLatencyMode() const {
   // If there's a commit in progress it must either be from the previous frame
   // or it started after the impl thread's deadline. In either case the main
   // thread is in high latency mode.
-  if (commit_state_ == COMMIT_STATE_BEGIN_MAIN_FRAME_SENT ||
-      commit_state_ == COMMIT_STATE_BEGIN_MAIN_FRAME_STARTED ||
-      commit_state_ == COMMIT_STATE_READY_TO_COMMIT)
+  if (CommitPending())
     return true;
 
   // Similarly, if there's a pending tree the main thread is in high latency
@@ -982,9 +980,8 @@ bool SchedulerStateMachine::MainThreadIsInHighLatencyMode() const {
     // Even if there's a new active tree to draw at the deadline or we've just
     // drawn it, it may have been triggered by a previous BeginImplFrame, in
     // which case the main thread is in a high latency mode.
-    return (active_tree_needs_first_draw_ ||
-            last_frame_number_swap_performed_ == current_frame_number_) &&
-           last_frame_number_begin_main_frame_sent_ != current_frame_number_;
+    return (active_tree_needs_first_draw_ || HasSwappedThisFrame()) &&

[brianderson, 2014/05/02 04:08:58]

Please keep this HasSwappedThisFrame(). I didn't think of this before, but ff a
swap early outs, we would be able to transition from high latency mode to low
latency mode without even skipping a BeginMainFrame because this would allow us
to swap the pending commit immediately.

Yay! =)

[simonhong, 2014/05/08 01:14:57]

Done.

+           !HasSentBeginMainFrameThisFrame();
   }
 
   // If the active tree needs its first draw in any other state, we know the
@@ -1022,6 +1019,8 @@ void SchedulerStateMachine::SetMaxSwapsPending(int max) {
 void SchedulerStateMachine::DidSwapBuffers() {
   pending_swaps_++;
   DCHECK_LE(pending_swaps_, max_pending_swaps_);
+
+  last_frame_number_swap_performed_ = current_frame_number_;
 }
 
 void SchedulerStateMachine::SetSwapUsedIncompleteTile(
@@ -1039,23 +1038,22 @@ void SchedulerStateMachine::SetSmoothnessTakesPriority(
   smoothness_takes_priority_ = smoothness_takes_priority;
 }
 
-void SchedulerStateMachine::DidDrawIfPossibleCompleted(
-    DrawSwapReadbackResult::DrawResult result) {
+void SchedulerStateMachine::DidDrawIfPossibleCompleted(DrawResult result) {
   switch (result) {
-    case DrawSwapReadbackResult::INVALID_RESULT:
-      NOTREACHED() << "Uninitialized DrawSwapReadbackResult.";
+    case INVALID_RESULT:
+      NOTREACHED() << "Uninitialized DrawResult.";
       break;
-    case DrawSwapReadbackResult::DRAW_ABORTED_CANT_DRAW:
-    case DrawSwapReadbackResult::DRAW_ABORTED_CANT_READBACK:
-    case DrawSwapReadbackResult::DRAW_ABORTED_CONTEXT_LOST:
+    case DRAW_ABORTED_CANT_DRAW:
+    case DRAW_ABORTED_CANT_READBACK:
+    case DRAW_ABORTED_CONTEXT_LOST:
       NOTREACHED() << "Invalid return value from DrawAndSwapIfPossible:"
                    << result;
       break;
-    case DrawSwapReadbackResult::DRAW_SUCCESS:
+    case DRAW_SUCCESS:
       consecutive_checkerboard_animations_ = 0;
       forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
       break;
-    case DrawSwapReadbackResult::DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
+    case DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
       needs_redraw_ = true;
 
       // If we're already in the middle of a redraw, we don't need to
@@ -1074,7 +1072,7 @@ void SchedulerStateMachine::DidDrawIfPossibleCompleted(
         forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
       }
       break;
-    case DrawSwapReadbackResult::DRAW_ABORTED_MISSING_HIGH_RES_CONTENT:
+    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