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

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

Refactors the code a bit to unify the regular and forced draw
paths. Adds and switches over to using a
SchedulerStateMachine::DidAction helper method.

A few unit tests are fixed or removed to be more realistic
and to reflect actual needs_redraw_ behavior after a draw
that fails with DRAW_ABORTED_CHECKERBOARD_ANIMATIONS.

BUG=486072
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f677d495ab096cd08a821d965bb29149c6b9d76d
Cr-Commit-Position: refs/heads/master@{#361555}

[brianderson, 2015-08-01 00:55:38]

brianderson@chromium.org changed reviewers:
+ mithro@mithis.com, sunnyps@chromium.org

[brianderson, 2015-08-01 00:55:39]

ptal!

[sunnyps, 2015-08-25 17:23:24]

https://codereview.chromium.org/1265023005/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/1265023005/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.h:126: void DidAction(Action action);
nit: DidPerformAction?

[sunnyps, 2015-08-25 17:23:39]

On 2015/08/25 17:23:24, sunnyps wrote:
>
https://codereview.chromium.org/1265023005/diff/1/cc/scheduler/scheduler_stat...
> File cc/scheduler/scheduler_state_machine.h (right):
> 
>
https://codereview.chromium.org/1265023005/diff/1/cc/scheduler/scheduler_stat...
> cc/scheduler/scheduler_state_machine.h:126: void DidAction(Action action);
> nit: DidPerformAction?

Oops sorry, meant to discard all drafts.

[brianderson, 2015-11-19 19:02:06]

Reviving this for: https://codereview.chromium.org/1460923002/

Sunny, can you take another look?

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

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:667: if (did_request_swap) {
Doesn't draw_result != DRAW_SUCCESS imply that did_request_swap is false and
that it's true otherwise?

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:696:
consecutive_checkerboard_animations_++;
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.

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:700:
consecutive_checkerboard_animations_ = 0;
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.

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:446:
state.DidSwapBuffersComplete();
Do we actually swap when a draw is aborted due to checkerboarding?

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

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:667: if (did_request_swap) {
On 2015/11/20 23:34:07, sunnyps wrote:
> Doesn't draw_result != DRAW_SUCCESS imply that did_request_swap is false and
> that it's true otherwise?

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.

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:696:
consecutive_checkerboard_animations_++;
On 2015/11/20 23:34:07, sunnyps wrote:
> 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.

Sg.

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:700:
consecutive_checkerboard_animations_ = 0;
On 2015/11/20 23:34:07, sunnyps wrote:
> 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.

Also sg.

[brianderson, 2015-11-21 01:19:02]

Ptal again. I think the code is easier to read now.

[brianderson, 2015-11-21 01:25:30]

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine_unittest.cc:446:
state.DidSwapBuffersComplete();
On 2015/11/20 23:34:07, sunnyps wrote:
> Do we actually swap when a draw is aborted due to checkerboarding?

Missed this. You are correct. Let me fix this test and add a DCHECK.

[brianderson, 2015-11-23 23:37:54]

Added DCHECKs to make tests that swap after an aborted draw fail and then fixed
those tests.

Ptal again.

[sunnyps, 2015-11-24 22:42:44]

LGTM with nits

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:252:
state->SetBoolean("did_perform_swap_in_last_draw",
nit: rename did_perform_swap_in_last_draw to did_swap_in_last_draw

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:695: case
DRAW_ABORTED_DRAINING_PIPELINE:
nit: Maybe DCHECK(!did_perform_swap_in_last_draw_) here too.

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:733: last_draw_result_ = DRAW_SUCCESS;
nit: Looks like the only reason to set this is for the DCHECK in DidSwapBuffers.
Seems unnecessary.

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:1039: DCHECK_EQ(last_draw_result_,
DRAW_SUCCESS);
nit: This DCHECK_EQ should be unnecessary.

[brianderson, 2015-11-24 23:19:38]

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/40001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:252:
state->SetBoolean("did_perform_swap_in_last_draw",
On 2015/11/24 22:42:44, sunnyps wrote:
> nit: rename did_perform_swap_in_last_draw to did_swap_in_last_draw

Renamed to did_swap_in_last_frame to be consistent with the
did_draw_in_last_frame naming.

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:695: case
DRAW_ABORTED_DRAINING_PIPELINE:
On 2015/11/24 22:42:44, sunnyps wrote:
> nit: Maybe DCHECK(!did_perform_swap_in_last_draw_) here too.

Tried doing this. DRAW_ABORTED_DRAINING_PIPELINE can come in at anytime compared
to the other aborts which only arrive in the deadline, so the DCHECK doesn't
quite work. I added a comment.

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:733: last_draw_result_ = DRAW_SUCCESS;
On 2015/11/24 22:42:44, sunnyps wrote:
> nit: Looks like the only reason to set this is for the DCHECK in
DidSwapBuffers.
> Seems unnecessary.

True. Removed.

https://codereview.chromium.org/1265023005/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:1039: DCHECK_EQ(last_draw_result_,
DRAW_SUCCESS);
On 2015/11/24 22:42:44, sunnyps wrote:
> nit: This DCHECK_EQ should be unnecessary.

Done.

[brianderson, 2015-11-24 23:19:43]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-24 23:20:25]

The CQ bit was unchecked by brianderson@chromium.org

[brianderson, 2015-11-24 23:20:53]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-24 23:20:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1265023005/#ps100001
(title: "Sunny's comments")

[commit-bot: I haz the power, 2015-11-24 23:23:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265023005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265023005/100001

[commit-bot: I haz the power, 2015-11-25 03:48:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-25 03:49:17]

Patchset 6 (id:??) landed as
https://crrev.com/f677d495ab096cd08a821d965bb29149c6b9d76d
Cr-Commit-Position: refs/heads/master@{#361555}