cc: Always use SetNeedsBeginFrame to request the next BeginFrame

cc: Always use SetNeedsBeginFrame to request the next BeginFrame

This avoids relying on SwapBuffers to implicitly trigger the next
BeginFrame. Only a single code path is used to request the next
BeginFrame now: SetNeedsBeginFrame(true).

This avoids issues where OutputSurface subclasses might not call
OutputSurface::DidSwap() when they need to or when we early out
a swap request and don't actually swap anything.

BUG=289755

[brianderson, 2013-09-12 01:35:38]

Should have done this a long time ago.

[brianderson, 2013-09-12 01:47:45]

Daniel, if you are trying this out, note that it might have dependencies on:
https://codereview.chromium.org/23463014 and
https://codereview.chromium.org/24019008

[danakj, 2013-09-12 16:53:58]

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
File cc/output/output_surface_unittest.cc (right):

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
cc/output/output_surface_unittest.cc:213: // DidSwapBuffers should clear the
pending BeginFrame.
comment should be replaced?

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
cc/output/output_surface_unittest.cc:222: // BeginFrame should be throttled by
pending swap buffers.
So if it's not tied to swap, what will throttle begin frame if swap is not
called?

[brianderson, 2013-09-12 18:46:43]

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
File cc/output/output_surface_unittest.cc (right):

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
cc/output/output_surface_unittest.cc:213: // DidSwapBuffers should clear the
pending BeginFrame.
On 2013/09/12 16:53:59, danakj wrote:
> comment should be replaced?

Yes. I will comb the code for similar comments to remove.

https://codereview.chromium.org/23767011/diff/1/cc/output/output_surface_unit...
cc/output/output_surface_unittest.cc:222: // BeginFrame should be throttled by
pending swap buffers.
On 2013/09/12 16:53:59, danakj wrote:
> So if it's not tied to swap, what will throttle begin frame if swap is not
> called?

The OutputSurface will throttle sending the next BeginFrame until
SetNeedsBeginFrame(true) has been called. There are already instances where we
do not swap and the Scheduler requests another BeginFrame by calling
SetNeedsBeginFrame(true). This patch just makes it so that it is the one and
only way to get another BeginFrame.

[danakj, 2013-09-12 20:57:29]

I have some more questions as I don't really know the new BeginFrame flow well
still.

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:177: has_pending_begin_frame_ = true;
Can you explain the purpose of this variable then? It seems like it stays true
until we SetupNextBeginFrameIfNeeded() which would unlock begin frame by setting
it to false? Am I reading right? How does this syncrhronize with the variable by
the same name in OutputSurface? (Why are there two?)

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler_unitte...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler_unitte...
cc/scheduler/scheduler_unittest.cc:239:
EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 2, 3);
Can you update the comment here to explain the new expectation?

[brianderson, 2013-09-12 21:54:21]

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/23767011/diff/1/cc/scheduler/scheduler.cc#new...
cc/scheduler/scheduler.cc:177: has_pending_begin_frame_ = true;
On 2013/09/12 20:57:29, danakj wrote:
> Can you explain the purpose of this variable then?

I was able to remove the has_pending_begin_frame in the deadline patch. I think
I can remove it in this patch as well since it is now (almost) a synonym for
state_machine_.inside_begin_frame().

> It seems like it stays true
> until we SetupNextBeginFrameIfNeeded() which would unlock begin frame by
setting it to false? Am I reading right?'
> How does this syncrhronize with the variable by
> the same name in OutputSurface? (Why are there two?)

It was supposed to be synchronized with OutputSurface::begin_frame_pending_.
Part of the problem this bug was running into was that they became out of sync
and the Scheduler didn't think it needed to request another BeginFrame: The swap
was setting has_pending_begin_frame_ to false in the Scheduler, but not setting
begin_frame_pending_ to false in OutputSurface. 

SetupNextBeginFrameIfNeeded() would only request another BeginFrame if
begin_frame_pending_ was true (the swap could have set it to false.)

[brianderson, 2013-09-12 23:26:09]

Ran into the "missing base files" error, so I am closing this issue and
continuing the review here:
https://codereview.chromium.org/23451055