cc: Simplify SetNeedsBeginFrames code in scheduler.

cc: Simplify SetNeedsBeginFrames code in scheduler.

Simplify the code that calls SetNeedsBeginFrames on the frame source so
that it's easier to understand. This paves the way for the state machine
to know as little as possible about the deadline which is required for
the upcoming webview-scheduler refactor.

BUG=439275
Committed: https://crrev.com/32f01258fdc1f2a620e0f1b347287a8aa804f57d
Cr-Commit-Position: refs/heads/master@{#321285}

[brianderson, 2015-03-05 22:51:23]

brianderson@chromium.org changed reviewers:
+ brianderson@chromium.org

[brianderson, 2015-03-05 22:51:23]

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

https://codereview.chromium.org/987563002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:726: bool
SchedulerStateMachine::ShouldSetNeedsBeginFrames(
Looks like you can delete this and frame_source_needs_begin_frames now?

[sunnyps, 2015-03-06 00:49:04]

Addressed comments. PTAL.

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

https://codereview.chromium.org/987563002/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:726: bool
SchedulerStateMachine::ShouldSetNeedsBeginFrames(
On 2015/03/05 22:51:23, brianderson wrote:
> Looks like you can delete this and frame_source_needs_begin_frames now?

Done. frame_source_needs_begin_frames is a local var not a class member.

[brianderson, 2015-03-06 01:34:36]

lgtm

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1568: // SetNeedsBeginFrames(false) is not
called until the end of the frame.
It's a little concerning that we won't stop requesting SetNeedsBeginFrames
immediately anymore. But I guess we trigger the deadline asap when we lose the
output surface and will stop requesting the BeginFrames at that point.

We may want to decouple SetNeedsBeginFrames from OS lost in the future (and try
to recreate the OS super fast) anyway, so this is a tiny step in that direction.
=)

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1579:
EXPECT_ACTION("SetNeedsBeginFrames(false)", client_, 1, 3);
I was going to say to try and change the code so SetNeedsBeginFrames(false) is
called before ScheduledActionBeginOutputSurfaceCreation, but both actions are
potentially async and our code should work either way.

Just trying to think of potential race conditions. Haven't thought of any real
ones, which is good.

[sunnyps, 2015-03-06 01:38:50]

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1568: // SetNeedsBeginFrames(false) is not
called until the end of the frame.
On 2015/03/06 01:34:36, brianderson wrote:
> It's a little concerning that we won't stop requesting SetNeedsBeginFrames
> immediately anymore. But I guess we trigger the deadline asap when we lose the
> output surface and will stop requesting the BeginFrames at that point.
> 
> We may want to decouple SetNeedsBeginFrames from OS lost in the future (and
try
> to recreate the OS super fast) anyway, so this is a tiny step in that
direction.
> =)

We do have the ability to ignore BeginFrames in the scheduler so maybe this is
doable now.

https://codereview.chromium.org/987563002/diff/40001/cc/scheduler/scheduler_u...
cc/scheduler/scheduler_unittest.cc:1579:
EXPECT_ACTION("SetNeedsBeginFrames(false)", client_, 1, 3);
On 2015/03/06 01:34:36, brianderson wrote:
> I was going to say to try and change the code so SetNeedsBeginFrames(false) is
> called before ScheduledActionBeginOutputSurfaceCreation, but both actions are
> potentially async and our code should work either way.
> 
> Just trying to think of potential race conditions. Haven't thought of any real
> ones, which is good.

I would've liked that too - it's difficult to achieve because the state machine
is responsible for ScheduledActionBeginOutputSurfaceCreation while the scheduler
is responsible for SetNeedsBeginFrames. The scheduler is trying to enforce that
SetNeedsBeginFrames is the last thing which happens in a frame.

[brianderson, 2015-03-19 01:25:54]

still lgtm

https://codereview.chromium.org/987563002/diff/60001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/987563002/diff/60001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:740: if (!HasInitializedOutputSurface())
This condition isn't taken into account in the code ported to the Scheduler, but
we'll trigger the deadline immediately in this case and so it shouldn't matter.
Is that true?


... nm. I see that your test changes capture this change in behavior and the
tests look good to me.

[sunnyps, 2015-03-19 01:31:53]

https://codereview.chromium.org/987563002/diff/60001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (left):

https://codereview.chromium.org/987563002/diff/60001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:740: if (!HasInitializedOutputSurface())
On 2015/03/19 01:25:54, brianderson wrote:
> This condition isn't taken into account in the code ported to the Scheduler,
but
> we'll trigger the deadline immediately in this case and so it shouldn't
matter.
> Is that true?
> 
> 
> ... nm. I see that your test changes capture this change in behavior and the
> tests look good to me.

Yes, the new way of calling SetNeedsBeginFrames(false) is more general than what
we had before.

Before we would call SetNBF(false) when the output surface was lost. I'm pretty
sure this was done because the output surface can be lost in between BeginFrames
(idle state).

Now we call SetBNF(false) if we're idle.

[sunnyps, 2015-03-19 01:32:13]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-19 01:32:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/987563002/60001

[commit-bot: I haz the power, 2015-03-19 03:50:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-19 03:51:06]

Patchset 4 (id:??) landed as
https://crrev.com/32f01258fdc1f2a620e0f1b347287a8aa804f57d
Cr-Commit-Position: refs/heads/master@{#321285}

[mithro-old, 2015-03-19 07:28:37]

On 2015/03/19 03:51:06, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/32f01258fdc1f2a620e0f1b347287a8aa804f57d
> Cr-Commit-Position: refs/heads/master@{#321285}

LGTM.