Remove forced commit and readback from the scheduler.

Remove forced commit and readback from the scheduler.

This removes CompositeAndReadback support from the scheduler, since the
code path is now removed.

R=brianderson
BUG=251936
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271722

[danakj, 2014-05-16 17:19:17]

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:324: if (active_tree_needs_first_draw_
|| has_pending_tree_)
Think this can go away entirely?

[brianderson, 2014-05-16 21:38:18]

Thanks Dana, can't wait to see this code gone!

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:582: void
SchedulerStateMachine::CheckInvariants() {
Sad to see this method go, but we can bring it back later if we want.

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:324: if (active_tree_needs_first_draw_
|| has_pending_tree_)
On 2014/05/16 17:19:17, danakj wrote:
> Think this can go away entirely?

Not having the replacement commit makes the pipeline much simpler. I think we
should be able to get rid of it, but let's do it in a followup patch instead.

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1433:
TEST(SchedulerStateMachineTest, TestFinishAllRenderingWhileContextLost) {
I was worried we might be losing DRAW_AND_SWAP_ABORT coverage here, but there
seem to be enough other tests that handle DRAW_AND_SWAP_ABORT.

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1976: // Should be back in the
idle state, but needing a commit.
Do we need to test aborted commits without a readback?

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1459:
TEST(SchedulerStateMachineTest, TestSendBeginMainFrameWhenInvisible) {
TestNoBeginMainFrameWhenInvisible?

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1466:
EXPECT_EQ(SchedulerStateMachine::ACTION_NONE, state.NextAction());
Before this line, put:
state.SetNeedsCommit();

so we test that a commit request is ignored.

[danakj, 2014-05-16 22:16:03]

PTAL

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:582: void
SchedulerStateMachine::CheckInvariants() {
On 2014/05/16 21:38:17, brianderson wrote:
> Sad to see this method go, but we can bring it back later if we want.

ya, it's just not checking anything anymore, soo..

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1976: // Should be back in the
idle state, but needing a commit.
On 2014/05/16 21:38:17, brianderson wrote:
> Do we need to test aborted commits without a readback?

Line 1101 does verify that an aborted commit leaves us in the commit idle state:
AbortBeginMainFrameAndCancelCommit. Do you think that is enough?>

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (right):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1459:
TEST(SchedulerStateMachineTest, TestSendBeginMainFrameWhenInvisible) {
On 2014/05/16 21:38:17, brianderson wrote:
> TestNoBeginMainFrameWhenInvisible?

Done.

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1466:
EXPECT_EQ(SchedulerStateMachine::ACTION_NONE, state.NextAction());
On 2014/05/16 21:38:17, brianderson wrote:
> Before this line, put:
> state.SetNeedsCommit();
> 
> so we test that a commit request is ignored.

Done.

[brianderson, 2014-05-16 22:25:45]

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1976: // Should be back in the
idle state, but needing a commit.
On 2014/05/16 22:16:03, danakj wrote:
> On 2014/05/16 21:38:17, brianderson wrote:
> > Do we need to test aborted commits without a readback?
> 
> Line 1101 does verify that an aborted commit leaves us in the commit idle
state:
> AbortBeginMainFrameAndCancelCommit. Do you think that is enough?>

This test is testing with !visible. I don't think we test aborted commits when
!visible anywhere else. Can you copy+modify one of the abort tests?

[danakj, 2014-05-16 22:42:31]

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine_unittest.cc (left):

https://codereview.chromium.org/292533002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1976: // Should be back in the
idle state, but needing a commit.
On 2014/05/16 22:25:45, brianderson wrote:
> On 2014/05/16 22:16:03, danakj wrote:
> > On 2014/05/16 21:38:17, brianderson wrote:
> > > Do we need to test aborted commits without a readback?
> > 
> > Line 1101 does verify that an aborted commit leaves us in the commit idle
> state:
> > AbortBeginMainFrameAndCancelCommit. Do you think that is enough?>
> 
> This test is testing with !visible. I don't think we test aborted commits when
> !visible anywhere else. Can you copy+modify one of the abort tests?

Ok, I added a few tests, PTAL!

[brianderson, 2014-05-16 23:02:25]

Thanks for the new tests! Just one more change, then lgtm.

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

https://codereview.chromium.org/292533002/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1220: // Become visible but
nothing happens until the next frame.
Can you call state.OnBeginImplFrame(CreateBeginFrameArgsForTesting()) before
this point too and verify that nothing happens?

[simonhong, 2014-05-17 00:03:21]

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1214:
base::Bind(&ThreadProxy::HasInitializedOutputSurfaceOnImplThread,
I think we can also remove this check.

[danakj, 2014-05-17 06:07:11]

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1214:
base::Bind(&ThreadProxy::HasInitializedOutputSurfaceOnImplThread,
On 2014/05/17 00:03:21, simonhong wrote:
> I think we can also remove this check.

Yep that's in another cl already :)

[simonhong, 2014-05-17 08:45:31]

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/292533002/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1214:
base::Bind(&ThreadProxy::HasInitializedOutputSurfaceOnImplThread,
On 2014/05/17 06:07:11, danakj wrote:
> On 2014/05/17 00:03:21, simonhong wrote:
> > I think we can also remove this check.
> 
> Yep that's in another cl already :)

Aha! great!

[danakj, 2014-05-20 16:05:24]

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

https://codereview.chromium.org/292533002/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine_unittest.cc:1220: // Become visible but
nothing happens until the next frame.
On 2014/05/16 23:02:25, brianderson wrote:
> Can you call state.OnBeginImplFrame(CreateBeginFrameArgsForTesting()) before
> this point too and verify that nothing happens?

Well, this causes us to be in an impl frame, which means when it becomes
visible, it immediately sends the begin frame.

I'll split this into another test case.

[danakj, 2014-05-20 16:05:34]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-20 16:06:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/292533002/60001

[commit-bot: I haz the power, 2014-05-20 19:16:07]

Change committed as 271722