[cc] Add and plumb CFS::DidNotProduceFrame.

[cc] Add and plumb CFS::DidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
DidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
TBR=junov@chromium.org

Review-Url: https://codereview.chromium.org/2888043004
Cr-Commit-Position: refs/heads/master@{#473577}
Committed: https://chromium.googlesource.com/chromium/src/+/9404a23e0073ff0f04f156963ffe99fba1c63a4a

[Eric Seckler, 2017-05-17 16:32:46]

Description was changed from

==========
[cc] Add CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

to

==========
[cc] Add CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

[Eric Seckler, 2017-05-17 16:32:47]

Description was changed from

==========
[cc] Add CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

to

==========
[cc] Add CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

[Eric Seckler, 2017-05-17 16:33:00]

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

[commit-bot: I haz the power, 2017-05-17 16:35:10]

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-17 18:47:33]

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

[commit-bot: I haz the power, 2017-05-17 18:47:34]

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

[Eric Seckler, 2017-05-18 08:37:46]

Description was changed from

==========
[cc] Add CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

to

==========
[cc] Add and plumb CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

[Eric Seckler, 2017-05-18 09:37:12]

Description was changed from

==========
[cc] Add and plumb CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

We will update the DidFinishFrame interface and remaining unit tests
separately.

The patch also renames existing BeginFrameDidNotSwap methods to unify
the naming across the code base.

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

to

==========
[cc] Add and plumb CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

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

[Eric Seckler, 2017-05-18 09:39:27]

eseckler@chromium.org changed reviewers:
+ fsamuel@chromium.org, junov@chromium.org, mkwst@chromium.org,
piman@chromium.org, reveman@chromium.org, sadrul@chromium.org,
sunnyps@chromium.org

[Eric Seckler, 2017-05-18 09:39:28]

Sunny, PTAL.

Also adding owners:
+mkwst for IPC name change.
+fsamuel for services/ui/ and components/viz/
+piman for content/
+sadrul for ui/
+reveman for components/exo/
+junov for OffscreenCanvasFrameDispatcherImpl.cpp

Thank you!

[piman, 2017-05-18 19:16:38]

lgtm

https://codereview.chromium.org/2888043004/diff/1/cc/surfaces/direct_composit...
File cc/surfaces/direct_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2888043004/diff/1/cc/surfaces/direct_composit...
cc/surfaces/direct_compositor_frame_sink.cc:162: void
DirectCompositorFrameSink::OnDidFinishFrame(const BeginFrameAck& ack) {}
nit: Are there any non-empty OnDidFinishFrame left? Could it be removed
altogether?

[reveman, 2017-05-18 19:27:04]

components/exo lgtm

[Fady Samuel, 2017-05-18 20:14:39]

lgtm

[brianderson, 2017-05-18 23:41:57]

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

[brianderson, 2017-05-18 23:41:58]

lgtm.

I missed the bikeshed naming discussion earlier, but my vote would be for
"SkipCompositorFrame".

[sadrul, 2017-05-19 03:55:04]

rs lgtm

[Eric Seckler, 2017-05-19 08:13:37]

On 2017/05/18 23:41:58, brianderson wrote:
> I missed the bikeshed naming discussion earlier, but my vote would be for
> "SkipCompositorFrame".

It'd be shorter for one :) - if people like this more, I'll rename.
BeginFrameDidNotProduceFrame is less ambiguous, though - SkipCompositorFrame
doesn't indidicate that it's referring to the current BeginFrame rather than
e.g. the last 
submitted CompositorFrame. (That's something we could clarify in comments, of
course.)

Alternatively, we could drop "BeginFrame" --> CFS::DidNotProduceFrame(const
BeginFrameAck&).

https://codereview.chromium.org/2888043004/diff/1/cc/surfaces/direct_composit...
File cc/surfaces/direct_compositor_frame_sink.cc (right):

https://codereview.chromium.org/2888043004/diff/1/cc/surfaces/direct_composit...
cc/surfaces/direct_compositor_frame_sink.cc:162: void
DirectCompositorFrameSink::OnDidFinishFrame(const BeginFrameAck& ack) {}
On 2017/05/18 19:16:37, piman wrote:
> nit: Are there any non-empty OnDidFinishFrame left? Could it be removed
> altogether?

You're right, done. Also got rid of the related observer tracking in
ExternalBFS.

[Eric Seckler, 2017-05-19 08:14:03]

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

[commit-bot: I haz the power, 2017-05-19 08:14:41]

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-19 10:07:30]

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

[commit-bot: I haz the power, 2017-05-19 10:07:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mike West, 2017-05-21 05:11:50]

IPC rename LGTM.

[sunnyps, 2017-05-22 07:17:13]

lgtm % nits

https://codereview.chromium.org/2888043004/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2888043004/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:40:
BeginFrameDidNotProduceFrame(cc.mojom.BeginFrameAck ack);
nit: My suggestion was DidNotProduceFrame (skipping the redundant BeginFrame
IMO) but I don't care too much about the name.

https://codereview.chromium.org/2888043004/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2888043004/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator_unittest.cc:114: const cc::BeginFrameAck&
last_begin_frame_ack() {
nit: const method

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1943:
compositor_frame_sink_->BeginFrameDidNotProduceFrame(ack);
nit: Check if compositor_frame_sink_ is non-null because the CFS can be lost.

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:810: void
SingleThreadProxy::BeginFrameDidNotProduceFrame(const BeginFrameAck& ack) {
nit: Set the impl task runner so that DCHECKs for impl thread don't get tripped.
Like this:
DebugScopedSetImplThread impl(task_runner_provider_);

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1236: if
(!delegated_frame_host_) {
nit: Can you add a comment about this?

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:949:
delegated_frame_host_->BeginFrameDidNotProduceFrame(ack);
nit: Check if delegated_frame_host_ is non-null.

[Eric Seckler, 2017-05-22 11:22:02]

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

[commit-bot: I haz the power, 2017-05-22 11:22:18]

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-22 11:24:53]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-22 11:25:32]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eric Seckler, 2017-05-22 11:28:02]

Description was changed from

==========
[cc] Add and plumb CFS::BeginFrameDidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
BeginFrameDidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

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

to

==========
[cc] Add and plumb CFS::DidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
DidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
TBR=junov@chromium.org
==========

[commit-bot: I haz the power, 2017-05-22 11:28:31]

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

[commit-bot: I haz the power, 2017-05-22 11:28:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2017-05-22 11:29:43]

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

[commit-bot: I haz the power, 2017-05-22 11:29:52]

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-22 11:29:58]

Thanks everyone!

https://codereview.chromium.org/2888043004/diff/1/cc/ipc/mojo_compositor_fram...
File cc/ipc/mojo_compositor_frame_sink.mojom (right):

https://codereview.chromium.org/2888043004/diff/1/cc/ipc/mojo_compositor_fram...
cc/ipc/mojo_compositor_frame_sink.mojom:40:
BeginFrameDidNotProduceFrame(cc.mojom.BeginFrameAck ack);
On 2017/05/22 07:17:12, sunnyps wrote:
> nit: My suggestion was DidNotProduceFrame (skipping the redundant BeginFrame
> IMO) but I don't care too much about the name.

Ah, misunderstood. Renamed, I like it shorter, too.

https://codereview.chromium.org/2888043004/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator_unittest.cc (right):

https://codereview.chromium.org/2888043004/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator_unittest.cc:114: const cc::BeginFrameAck&
last_begin_frame_ack() {
On 2017/05/22 07:17:12, sunnyps wrote:
> nit: const method

Done.

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1943:
compositor_frame_sink_->BeginFrameDidNotProduceFrame(ack);
On 2017/05/22 07:17:12, sunnyps wrote:
> nit: Check if compositor_frame_sink_ is non-null because the CFS can be lost.

Done.

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:810: void
SingleThreadProxy::BeginFrameDidNotProduceFrame(const BeginFrameAck& ack) {
On 2017/05/22 07:17:13, sunnyps wrote:
> nit: Set the impl task runner so that DCHECKs for impl thread don't get
tripped.
> Like this:
> DebugScopedSetImplThread impl(task_runner_provider_);

Done.

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1236: if
(!delegated_frame_host_) {
On 2017/05/22 07:17:13, sunnyps wrote:
> nit: Can you add a comment about this?

Done.

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2888043004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:949:
delegated_frame_host_->BeginFrameDidNotProduceFrame(ack);
On 2017/05/22 07:17:13, sunnyps wrote:
> nit: Check if delegated_frame_host_ is non-null.

Done.

[commit-bot: I haz the power, 2017-05-22 11:49:48]

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

[commit-bot: I haz the power, 2017-05-22 11:49:49]

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

[Eric Seckler, 2017-05-22 12:15:18]

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

[commit-bot: I haz the power, 2017-05-22 12:15:35]

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-22 12:17:09]

Patchset #4 (id:80001) has been deleted

[Eric Seckler, 2017-05-22 12:17:38]

Patchset #3 (id:60001) has been deleted

[Eric Seckler, 2017-05-22 12:17:48]

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

[commit-bot: I haz the power, 2017-05-22 12:18:07]

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-22 14:28:13]

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

[commit-bot: I haz the power, 2017-05-22 14:28:15]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-05-22 14:42:06]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-05-22 14:42:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, brianderson@chromium.org, piman@chromium.org,
fsamuel@chromium.org, sadrul@chromium.org, mkwst@chromium.org,
sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2888043004/#ps100001
(title: "address nits, rename to DidNotProduceFrame.")

[commit-bot: I haz the power, 2017-05-22 14:42: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-22 14:49:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495464126548940,
"parent_rev": "0ee82f7e38627cd7382166e219f4ec72631c816a", "commit_rev":
"9404a23e0073ff0f04f156963ffe99fba1c63a4a"}

[commit-bot: I haz the power, 2017-05-22 14:49:59]

Description was changed from

==========
[cc] Add and plumb CFS::DidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
DidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
TBR=junov@chromium.org
==========

to

==========
[cc] Add and plumb CFS::DidNotProduceFrame.

We're planning to remove BeginFrameAcks from BFS::DidFinishFrame.
Instead, we plumb them through the CompositorFrameSink via
SubmitCompositorFrame (this plumbing exists already) and
DidNotProduceFrame (new in this patch).

The DidFinishFrame interface (and remaining unit tests) will be
updated separately.

The patch also renames existing BeginFrameDidNotSwap methods/IPCs to
unify the naming across the code base.

BUG=697086
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_site_isolation
TBR=junov@chromium.org

Review-Url: https://codereview.chromium.org/2888043004
Cr-Commit-Position: refs/heads/master@{#473577}
Committed:
https://chromium.googlesource.com/chromium/src/+/9404a23e0073ff0f04f156963ffe...
==========

[commit-bot: I haz the power, 2017-05-22 14:50:02]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9404a23e0073ff0f04f156963ffe...