cc: Don't swap throttle BeginMainFrame just after swapping

cc: Don't swap throttle BeginMainFrame just after swapping

This will better reflect behavior prior to the
OutputSurface+Scheduler refactor and will
prevent us from skipping BeginMainFrames that
the scheduler only meant to defer.

BUG=381346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277109

[brianderson, 2014-06-11 21:38:19]

ptal. I modified the test so that it would fail without the fix in this patch.

[danakj, 2014-06-11 23:08:58]

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:467: // SwapAck throttle the
BeginMainFrames unless we just swapped.
This sounds like the opposite of what you have below, more like "unless we
didn't swap inside this deadline yet".

But then, doesn't that make the concept of pending swaps kinda pointless here?
We never wanna commit more than once in a deadline, and this would less us
commit every impl deadline?

[brianderson, 2014-06-11 23:25:50]

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:467: // SwapAck throttle the
BeginMainFrames unless we just swapped.
On 2014/06/11 23:08:57, danakj wrote:
> This sounds like the opposite of what you have below, more like "unless we
> didn't swap inside this deadline yet".

I think the comment and logic do match, it's just a little confusing. I actually
got the logic wrong at first until I updated the unit test.

> 
> But then, doesn't that make the concept of pending swaps kinda pointless here?
> We never wanna commit more than once in a deadline, and this would less us
> commit every impl deadline?

It disables the swap throttling only immediately after a swap within the same
deadline. All subsequent deadlines will not send BeginMainFrames if we remain
swap throttled.

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1787:
SchedulerStateMachine::ACTION_SEND_BEGIN_MAIN_FRAME);
This point in the test verifies the logic still sends the BeginMainFrame just
after swapping.

https://codereview.chromium.org/311353003 also passes this check.

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1821:
EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
This point in the test verifies that we don't send BeginMainFrames in subsequent
deadlines while we are swap throttled.

https://codereview.chromium.org/311353003 did not pass this check.

[Sami, 2014-06-13 10:39:12]

lgtm with a couple of test nits.

Sorry for failing send my comments when I actually reviewed this :P

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:467: // SwapAck throttle the
BeginMainFrames unless we just swapped.
The conceptual double negatives here make your head spin don't they :) The logic
below looks correct to me, but I really hope we can make this a little less
ad-hoc in the future. Maybe it's time to consider not throttling at all here, or
even getting rid of smoothness mode.

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1792: // Finish the previous
commit and draw it.
nit: Maybe consider lifting some of this into a utility function (e.g.,
FinishCommit) since the logic is pretty repetitive.

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1813: // Verify we do not send
another BeginMainFrame in the deadline if we
nit: "if we are swap throttled and did not just swap" to be a little more
precise.

[brianderson, 2014-06-13 18:19:30]

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:467: // SwapAck throttle the
BeginMainFrames unless we just swapped.
On 2014/06/13 10:39:12, Sami wrote:
> The conceptual double negatives here make your head spin don't they :) The
logic
> below looks correct to me, but I really hope we can make this a little less
> ad-hoc in the future. Maybe it's time to consider not throttling at all here,
or
> even getting rid of smoothness mode.

I've been trying to figure out why getting rid of this restriction causes
browser test flakiness in
https://code.google.com/p/chromium/issues/detail?id=368116. I really really want
to get rid of this check.

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

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1792: // Finish the previous
commit and draw it.
On 2014/06/13 10:39:12, Sami wrote:
> nit: Maybe consider lifting some of this into a utility function (e.g.,
> FinishCommit) since the logic is pretty repetitive.

Done.

https://codereview.chromium.org/325303002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:1813: // Verify we do not send
another BeginMainFrame in the deadline if we
On 2014/06/13 10:39:12, Sami wrote:
> nit: "if we are swap throttled and did not just swap" to be a little more
> precise.

Done.

[brianderson, 2014-06-13 18:19:51]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-06-13 18:21:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/brianderson@chromium.org/325303002/20001

[commit-bot: I haz the power, 2014-06-13 22:22:35]

Change committed as 277109