[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2897053002
Cr-Commit-Position: refs/heads/master@{#474983}
Committed: https://chromium.googlesource.com/chromium/src/+/ca6d7cb7945ff4b54d76c1df3806b979cc7584b5

[Eric Seckler, 2017-05-23 14:07:30]

Description was changed from

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
==========

to

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Eric Seckler, 2017-05-23 14:07:31]

Description was changed from

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

[Eric Seckler, 2017-05-23 14:16:00]

eseckler@chromium.org changed reviewers:
+ boliu@chromium.org, brianderson@chromium.org, piman@chromium.org,
reveman@chromium.org

[Eric Seckler, 2017-05-23 14:16:01]

Brian, PTAL. Thanks!

Owners:
priman@ for content/
boliu@ for ui/android/
reveman@ for components/exo/

[Eric Seckler, 2017-05-23 14:21:38]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 14:21:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 14:47:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 14:47:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[reveman, 2017-05-23 14:52:56]

lgtm

[Eric Seckler, 2017-05-23 14:59:44]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 15:00:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-05-23 15:43:17]

On 2017/05/23 14:16:01, Eric Seckler wrote:
> Brian, PTAL. Thanks!
> 
> Owners:
> priman@ for content/
> boliu@ for ui/android/

rs lgtm

> reveman@ for components/exo/

[commit-bot: I haz the power, 2017-05-23 15:56:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 15:56:33]

Dry run: This issue passed the CQ dry run.

[piman, 2017-05-23 17:53:17]

https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:249:
// plumbing of BeginFrameAcks through SurfaceObservers.
What is the plan for bringing back these tests? Should we file a bug?

[Eric Seckler, 2017-05-23 18:01:12]

https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:249:
// plumbing of BeginFrameAcks through SurfaceObservers.
On 2017/05/23 17:53:17, piman wrote:
> What is the plan for bringing back these tests? Should we file a bug?

I'll add them to https://codereview.chromium.org/2854163003/ tomorrow, which is
where we're working on said plumbing. Don't think a bug is necessary given the
short-term nature of this?

[piman, 2017-05-23 18:05:39]

On 2017/05/23 18:01:12, Eric Seckler wrote:
>
https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
> File
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2897053002/diff/20001/content/browser/frame_h...
>
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:249:
> // plumbing of BeginFrameAcks through SurfaceObservers.
> On 2017/05/23 17:53:17, piman wrote:
> > What is the plan for bringing back these tests? Should we file a bug?
> 
> I'll add them to https://codereview.chromium.org/2854163003/ tomorrow, which
is
> where we're working on said plumbing. Don't think a bug is necessary given the
> short-term nature of this?

sg, lgtm

[sunnyps, 2017-05-24 02:38:25]

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

[sunnyps, 2017-05-24 02:38:26]

lgtm % nits

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:252: void DidFinishFrame(BeginFrameObserver*
obs) override;
nit: Define DidFinishFrame here.

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:465: begin_frame_source_->DidFinishFrame(this);
nit: Move this to FinishImplFrame because that's the only place where this is
relevant.

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:3506: client_->Reset();
nit: it's fine to omit the client_->Reset at the end

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.cc:97:
begin_frame_source_->DidFinishFrame(this);
nit: We don't need this. When we use back to back BFS (disable vsync) we always
instantiate it in the client.

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.cc:151:
begin_frame_source_->DidFinishFrame(this);
nit: same here

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:367: // TODO(eseckler): Let client know that
frame was completed.
nit: DCHECK(begin_frame_source_) here

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/surface_syn...
File cc/surfaces/surface_synchronization_unittest.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/surface_syn...
cc/surfaces/surface_synchronization_unittest.cc:977: 
nit: Can you add a TODO here so that we don't forget about this?

https://codereview.chromium.org/2897053002/diff/20001/cc/test/fake_external_b...
File cc/test/fake_external_begin_frame_source.h (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/test/fake_external_b...
cc/test/fake_external_begin_frame_source.h:66:
std::unordered_map<BeginFrameObserver*, BeginFrameAck> last_acks_;
nit: Remove last_acks_

[Eric Seckler, 2017-05-24 09:00:41]

Thanks :)

Sunny, PTAL at comments.

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:252: void DidFinishFrame(BeginFrameObserver*
obs) override;
On 2017/05/24 02:38:25, sunnyps wrote:
> nit: Define DidFinishFrame here.

Done.

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:465: begin_frame_source_->DidFinishFrame(this);
On 2017/05/24 02:38:25, sunnyps wrote:
> nit: Move this to FinishImplFrame because that's the only place where this is
> relevant.

Is this true? I think we also want to ack to the (BackToBack)BeginFrameSource if
we skip a BeginFrame during OnBeginFrameDerivedImpl() without triggering a
BeginImplFrame.

I'll leave this as-is for now.

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:3506: client_->Reset();
On 2017/05/24 02:38:25, sunnyps wrote:
> nit: it's fine to omit the client_->Reset at the end

I moved this from above these checks. Should it not still happen before the
SetNeedsRedraw below, so that we checked that ScheduledActionDrawIfPossible
happened where it should?

Leaving as-is for now.

Or were you referring to Reset() calls at the end of test bodies? I'll leave
them there as well, since that seems to be the common idiom in surrounding code.

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
File cc/surfaces/compositor_frame_sink_support.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.cc:97:
begin_frame_source_->DidFinishFrame(this);
On 2017/05/24 02:38:25, sunnyps wrote:
> nit: We don't need this. When we use back to back BFS (disable vsync) we
always
> instantiate it in the client.

I think this isn't true for local clients. DirectCFS currently always exposes an
ExternalBFS to its client, not the original BackToBackBFS. Thus, CFSSupport
needs to ack to the BFS on behalf of such local clients. (And this will also be
true once we stop exposing BFSes to CompositorFrameSink(Client)s.)

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/compositor_...
cc/surfaces/compositor_frame_sink_support.cc:151:
begin_frame_source_->DidFinishFrame(this);
On 2017/05/24 02:38:25, sunnyps wrote:
> nit: same here

see above.

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/display_sch...
File cc/surfaces/display_scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/display_sch...
cc/surfaces/display_scheduler.cc:367: // TODO(eseckler): Let client know that
frame was completed.
On 2017/05/24 02:38:26, sunnyps wrote:
> nit: DCHECK(begin_frame_source_) here

Done.

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/surface_syn...
File cc/surfaces/surface_synchronization_unittest.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/surfaces/surface_syn...
cc/surfaces/surface_synchronization_unittest.cc:977: 
On 2017/05/24 02:38:26, sunnyps wrote:
> nit: Can you add a TODO here so that we don't forget about this?

Done.

https://codereview.chromium.org/2897053002/diff/20001/cc/test/fake_external_b...
File cc/test/fake_external_begin_frame_source.h (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/test/fake_external_b...
cc/test/fake_external_begin_frame_source.h:66:
std::unordered_map<BeginFrameObserver*, BeginFrameAck> last_acks_;
On 2017/05/24 02:38:26, sunnyps wrote:
> nit: Remove last_acks_

Done.

[sunnyps, 2017-05-25 20:20:27]

lgtm

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:465: begin_frame_source_->DidFinishFrame(this);
On 2017/05/24 09:00:40, Eric Seckler wrote:
> On 2017/05/24 02:38:25, sunnyps wrote:
> > nit: Move this to FinishImplFrame because that's the only place where this
is
> > relevant.
> 
> Is this true? I think we also want to ack to the (BackToBack)BeginFrameSource
if
> we skip a BeginFrame during OnBeginFrameDerivedImpl() without triggering a
> BeginImplFrame.
> 
> I'll leave this as-is for now.

We skip begin frames in two cases:
1. not expecting begin frames
2. impl latency recovery which is disabled for !throttled BFS (back to back)

We don't need to call DidFinishFrame in both cases.

[Eric Seckler, 2017-05-26 08:55:31]

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2897053002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:465: begin_frame_source_->DidFinishFrame(this);
On 2017/05/25 20:20:27, sunnyps wrote:
> On 2017/05/24 09:00:40, Eric Seckler wrote:
> > On 2017/05/24 02:38:25, sunnyps wrote:
> > > nit: Move this to FinishImplFrame because that's the only place where this
> is
> > > relevant.
> > 
> > Is this true? I think we also want to ack to the
(BackToBack)BeginFrameSource
> if
> > we skip a BeginFrame during OnBeginFrameDerivedImpl() without triggering a
> > BeginImplFrame.
> > 
> > I'll leave this as-is for now.
> 
> We skip begin frames in two cases:
> 1. not expecting begin frames
> 2. impl latency recovery which is disabled for !throttled BFS (back to back)
> 
> We don't need to call DidFinishFrame in both cases.

Still leaving as-is as discussed offline (the BFS interface doesn't really
clarify that this only needs to be called if BFS may be BackToBack).

[Eric Seckler, 2017-05-26 08:55:39]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 08:55:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 09:14:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 09:14:58]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Eric Seckler, 2017-05-26 09:48:44]

The CQ bit was checked by eseckler@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 09:48:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2017-05-26 11:23:18]

The CQ bit was unchecked by eseckler@chromium.org

[Eric Seckler, 2017-05-26 11:23:18]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-05-26 11:23:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, boliu@chromium.org, piman@chromium.org,
sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2897053002/#ps60001
(title: "sync for PrimaryBFS")

[commit-bot: I haz the power, 2017-05-26 11:23:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 11:33:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495797798889010,
"parent_rev": "bddc4ba03f742af167194a52a0a92516976fa9fb", "commit_rev":
"ca6d7cb7945ff4b54d76c1df3806b979cc7584b5"}

[commit-bot: I haz the power, 2017-05-26 11:34:02]

Description was changed from

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[cc] Remove BeginFrameAck from BFS::DidFinishFrame and update tests.

Follow-up to https://codereview.chromium.org/2888043004/.

BeginFrameAcks are now plumbed through the CompositorFrameSink via
SubmitCompositorFrame and DidNotProduceFrame.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2897053002
Cr-Commit-Position: refs/heads/master@{#474983}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca6d7cb7945ff4b54d76c1df3806...
==========

[commit-bot: I haz the power, 2017-05-26 11:34:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ca6d7cb7945ff4b54d76c1df3806...