cc: Dedup IPCs to return resources to client

cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to
return resources to clients. The only difference is one acknowledges a
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b62b78229e838b0140af7973e83ada9fcce28a19
Cr-Commit-Position: refs/heads/master@{#405660}

[Fady Samuel, 2016-07-13 14:12:18]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

BUG=none
==========

to

==========
cc: Dedup IPCs to return resources to client

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-13 14:14:47]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and ViewMsg_SwapCompositorFrameAck are
identical IPCs to return
resources to clients. This is both confusing and an unnecessary
maintenance burden. This CL deletes 
ViewMsg_ReclaimCompositorResources and associated code using
ViewMsg_SwapCompositorFrameAck everywhere.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-13 14:15:50]

fsamuel@chromium.org changed reviewers:
+ piman@chromium.org

[Fady Samuel, 2016-07-13 14:15:51]

Hooray! Another net negative CL! Looks like there's a lot of cruft that has
crept into this code lately. I'm cleaning up as I go.

[Fady Samuel, 2016-07-13 14:16:25]

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

[Fady Samuel, 2016-07-13 14:16:36]

fsamuel@chromium.org changed reviewers:
+ dcheng@chromium.org

[Fady Samuel, 2016-07-13 14:16:37]

+dcheng@ for IPC.

[commit-bot: I haz the power, 2016-07-13 14:16:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-07-13 14:17:11]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and ViewMsg_SwapCompositorFrameAck are
identical IPCs to return
resources to clients. This is both confusing and an unnecessary
maintenance burden. This CL deletes 
ViewMsg_ReclaimCompositorResources and associated code using
ViewMsg_SwapCompositorFrameAck everywhere.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are identical IPCs to return
resources to clients. This is both confusing and an unnecessary
maintenance burden. This CL deletes 
ViewMsg_ReclaimCompositorResources and associated code using
ViewMsg_SwapCompositorFrameAck everywhere.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2016-07-13 14:26:58]

ipc lgtm

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.cc:164:
client_->DidSwapBuffersComplete();
Does it matter that OnReclaimResources didn't call this?

[commit-bot: I haz the power, 2016-07-13 14:30:41]

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

[commit-bot: I haz the power, 2016-07-13 14:30:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Fady Samuel, 2016-07-13 15:29:48]

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.cc:164:
client_->DidSwapBuffersComplete();
On 2016/07/13 14:26:58, dcheng wrote:
> Does it matter that OnReclaimResources didn't call this?

piman@ can better answer this question. I could add a bool to the IPC to do this
if necessary.

It looks to me that we unref resources in response to a new compositorframe
anyway, and so this is effectively marking the end of a swapbuffers. I don't
really yet understand the implication of calling this more than before yet.

[Fady Samuel, 2016-07-13 15:42:45]

On 2016/07/13 15:29:48, Fady Samuel wrote:
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> File content/renderer/gpu/compositor_output_surface.cc (right):
> 
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> content/renderer/gpu/compositor_output_surface.cc:164:
> client_->DidSwapBuffersComplete();
> On 2016/07/13 14:26:58, dcheng wrote:
> > Does it matter that OnReclaimResources didn't call this?
> 
> piman@ can better answer this question. I could add a bool to the IPC to do
this
> if necessary.
> 
> It looks to me that we unref resources in response to a new compositorframe
> anyway, and so this is effectively marking the end of a swapbuffers. I don't
> really yet understand the implication of calling this more than before yet.

There are three places where we return resources that I've found:

1. We've submitted a new CompositorFrame to a surface that had a previous frame:
https://cs.chromium.org/chromium/src/cc/surfaces/surface.cc?q=Surface::QueueF...

2. We submit invalid resources:
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...

3. Tear down:
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...

[Fady Samuel, 2016-07-13 15:45:32]

On 2016/07/13 15:42:45, Fady Samuel wrote:
> On 2016/07/13 15:29:48, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> > File content/renderer/gpu/compositor_output_surface.cc (right):
> > 
> >
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> > content/renderer/gpu/compositor_output_surface.cc:164:
> > client_->DidSwapBuffersComplete();
> > On 2016/07/13 14:26:58, dcheng wrote:
> > > Does it matter that OnReclaimResources didn't call this?
> > 
> > piman@ can better answer this question. I could add a bool to the IPC to do
> this
> > if necessary.
> > 
> > It looks to me that we unref resources in response to a new compositorframe
> > anyway, and so this is effectively marking the end of a swapbuffers. I don't
> > really yet understand the implication of calling this more than before yet.
> 
> There are three places where we return resources that I've found:
> 
> 1. We've submitted a new CompositorFrame to a surface that had a previous
frame:
>
https://cs.chromium.org/chromium/src/cc/surfaces/surface.cc?q=Surface::QueueF...
> 
> 2. We submit invalid resources:
>
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...
> 
> 3. Tear down:
>
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...

1. I think is when we've submitted a frame and the previous frame hasn't made it
to the display.

3. I think is when we've passed the resources to GL and put them on screen...I
think.


2 is the only fishy case, I think. The others seem reasonable to call
DidSwapBuffersComplete, I think?

[Fady Samuel, 2016-07-13 16:05:16]

On 2016/07/13 15:45:32, Fady Samuel wrote:
> On 2016/07/13 15:42:45, Fady Samuel wrote:
> > On 2016/07/13 15:29:48, Fady Samuel wrote:
> > >
> >
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> > > File content/renderer/gpu/compositor_output_surface.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> > > content/renderer/gpu/compositor_output_surface.cc:164:
> > > client_->DidSwapBuffersComplete();
> > > On 2016/07/13 14:26:58, dcheng wrote:
> > > > Does it matter that OnReclaimResources didn't call this?
> > > 
> > > piman@ can better answer this question. I could add a bool to the IPC to
do
> > this
> > > if necessary.
> > > 
> > > It looks to me that we unref resources in response to a new
compositorframe
> > > anyway, and so this is effectively marking the end of a swapbuffers. I
don't
> > > really yet understand the implication of calling this more than before
yet.
> > 
> > There are three places where we return resources that I've found:
> > 
> > 1. We've submitted a new CompositorFrame to a surface that had a previous
> frame:
> >
>
https://cs.chromium.org/chromium/src/cc/surfaces/surface.cc?q=Surface::QueueF...
> > 
> > 2. We submit invalid resources:
> >
>
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...
> > 
> > 3. Tear down:
> >
>
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?dr=CSs...
> 
> 1. I think is when we've submitted a frame and the previous frame hasn't made
it
> to the display.
> 
> 3. I think is when we've passed the resources to GL and put them on screen...I
> think.
> 
> 
> 2 is the only fishy case, I think. The others seem reasonable to call
> DidSwapBuffersComplete, I think?

Actually, looking at the test failures, it looks like I cannot always call this.

[danakj, 2016-07-13 18:40:20]

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

[danakj, 2016-07-13 18:40:20]

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
content/renderer/gpu/compositor_output_surface.cc:164:
client_->DidSwapBuffersComplete();
On 2016/07/13 15:29:48, Fady Samuel wrote:
> On 2016/07/13 14:26:58, dcheng wrote:
> > Does it matter that OnReclaimResources didn't call this?
> 
> piman@ can better answer this question. I could add a bool to the IPC to do
this
> if necessary.
> 
> It looks to me that we unref resources in response to a new compositorframe
> anyway, and so this is effectively marking the end of a swapbuffers. I don't
> really yet understand the implication of calling this more than before yet.

This is what unthrottles the compositor to produce another frame. Calling it a
different number of times would be bad, it needs to match with SwapBuffers.

[piman, 2016-07-13 18:54:33]

On Wed, Jul 13, 2016 at 8:29 AM, <fsamuel@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> File content/renderer/gpu/compositor_output_surface.cc (right):
>
>
>
https://codereview.chromium.org/2147873003/diff/1/content/renderer/gpu/compos...
> content/renderer/gpu/compositor_output_surface.cc:164:
> client_->DidSwapBuffersComplete();
> On 2016/07/13 14:26:58, dcheng wrote:
> > Does it matter that OnReclaimResources didn't call this?
>
> piman@ can better answer this question. I could add a bool to the IPC to
> do this if necessary.
>
> It looks to me that we unref resources in response to a new
> compositorframe anyway, and so this is effectively marking the end of a
> swapbuffers. I don't really yet understand the implication of calling
> this more than before yet.
>

No, we do need a bool, as Dana mentioned.

Some places where we would return resources not as a swap ack would be for
example if we release resources for a tab that goes offscreen.


>
> https://codereview.chromium.org/2147873003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-07-13 20:47:38]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are identical IPCs to return
resources to clients. This is both confusing and an unnecessary
maintenance burden. This CL deletes 
ViewMsg_ReclaimCompositorResources and associated code using
ViewMsg_SwapCompositorFrameAck everywhere.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is acknowledges a swap 
and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Fady Samuel, 2016-07-13 20:48:11]

PTAL everyone! This updated CL adds the is_swap_ack bool and gets rid of
CompositorFrameAck.

[Fady Samuel, 2016-07-13 20:48:48]

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

[commit-bot: I haz the power, 2016-07-13 20:49:29]

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, 2016-07-13 20:49:31]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names without "master." prefix is deprecated:
tryserver.chromium.linux. For more details, see http://crbug.com/617627.

[Fady Samuel, 2016-07-13 20:49:34]

fsamuel@chromium.org changed reviewers:
+ boliu@chromium.org

[Fady Samuel, 2016-07-13 20:49:35]

+boliu@ for android_webview

[Fady Samuel, 2016-07-13 20:50:16]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is acknowledges a swap 
and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[piman, 2016-07-13 20:59:15]

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

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:513:
SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
this one is a swap ack.

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:535:
pending_delegated_ack_count_--;
this (the DCHECK and the decrement) should only happen if is_swap_ack is true.

[commit-bot: I haz the power, 2016-07-13 21:02:07]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-13 21:10:32]

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

[commit-bot: I haz the power, 2016-07-13 21:10:33]

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_...)

[Fady Samuel, 2016-07-13 22:11:42]

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

[Fady Samuel, 2016-07-13 22:12:16]

PTAL Antoine!

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

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:513:
SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
On 2016/07/13 20:59:14, piman wrote:
> this one is a swap ack.

Done.

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:535:
pending_delegated_ack_count_--;
On 2016/07/13 20:59:14, piman wrote:
> this (the DCHECK and the decrement) should only happen if is_swap_ack is true.

Done.

[commit-bot: I haz the power, 2016-07-13 22:12:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-07-13 22:15:03]

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

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:513:
SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
On 2016/07/13 22:12:16, Fady Samuel wrote:
> On 2016/07/13 20:59:14, piman wrote:
> > this one is a swap ack.
> 
> Done.

Are there (can there be) tests that cover these mistakes?

[piman, 2016-07-13 22:15:32]

lgtm

[boliu, 2016-07-13 22:16:57]

my part lgtm

[commit-bot: I haz the power, 2016-07-13 22:42:29]

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

[commit-bot: I haz the power, 2016-07-13 22:42:30]

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

[Fady Samuel, 2016-07-13 22:43:36]

On 2016/07/13 22:15:03, danakj wrote:
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/delegated_frame_host.cc:513:
> SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
> On 2016/07/13 22:12:16, Fady Samuel wrote:
> > On 2016/07/13 20:59:14, piman wrote:
> > > this one is a swap ack.
> > 
> > Done.
> 
> Are there (can there be) tests that cover these mistakes?

So certainly tests will fail if we report more as swap acks than the right
number. If we report fewer, I'm not sure. I'll look into it.

[Fady Samuel, 2016-07-13 22:56:16]

fsamuel@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Fady Samuel, 2016-07-13 22:56:17]

+mbarbella@ for ipc_fuzzer

[Fady Samuel, 2016-07-13 22:56:29]

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

[commit-bot: I haz the power, 2016-07-13 22:57:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-07-13 23:00:20]

On 2016/07/13 22:15:03, danakj wrote:
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/delegated_frame_host.cc:513:
> SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
> On 2016/07/13 22:12:16, Fady Samuel wrote:
> > On 2016/07/13 20:59:14, piman wrote:
> > > this one is a swap ack.
> > 
> > Done.
> 
> Are there (can there be) tests that cover these mistakes?

https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine.cc?...

It looks like the appropriate DCHECKs are in pass for too many or too few swap
acks and tests do appear to fail if we get this wrong.

[Martin Barbella, 2016-07-13 23:05:08]

ipc_fuzzer lgtm

[danakj, 2016-07-13 23:09:55]

On Wed, Jul 13, 2016 at 4:00 PM, <fsamuel@chromium.org> wrote:

> On 2016/07/13 22:15:03, danakj wrote:
> >
>
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> > File content/browser/renderer_host/delegated_frame_host.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> > content/browser/renderer_host/delegated_frame_host.cc:513:
> > SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack
> */);
> > On 2016/07/13 22:12:16, Fady Samuel wrote:
> > > On 2016/07/13 20:59:14, piman wrote:
> > > > this one is a swap ack.
> > >
> > > Done.
> >
> > Are there (can there be) tests that cover these mistakes?
>
>
>
https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine.cc?...
>
> It looks like the appropriate DCHECKs are in pass for too many or too few
> swap
> acks and tests do appear to fail if we get this wrong.
>

Which tests would fail without these fixes?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-07-13 23:27:13]

On 2016/07/13 23:09:55, danakj wrote:
> On Wed, Jul 13, 2016 at 4:00 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > On 2016/07/13 22:15:03, danakj wrote:
> > >
> >
> >
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> > > File content/browser/renderer_host/delegated_frame_host.cc (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
> > > content/browser/renderer_host/delegated_frame_host.cc:513:
> > > SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack
> > */);
> > > On 2016/07/13 22:12:16, Fady Samuel wrote:
> > > > On 2016/07/13 20:59:14, piman wrote:
> > > > > this one is a swap ack.
> > > >
> > > > Done.
> > >
> > > Are there (can there be) tests that cover these mistakes?
> >
> >
> >
>
https://cs.chromium.org/chromium/src/cc/scheduler/scheduler_state_machine.cc?...
> >
> > It looks like the appropriate DCHECKs are in pass for too many or too few
> > swap
> > acks and tests do appear to fail if we get this wrong.
> >
> 
> Which tests would fail without these fixes?
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sigh, I got it wrong. The tests failing are all related to too many swap ACKs
and not too few. I found the "two few ACKS" DCHECK but it doesn't actually seem
to be hit by bots now that they've had more time to complete. :( Compiling and
running blocks of code in my head and hitting mental DCHECK doesn't count as a
test, it seems :P

[commit-bot: I haz the power, 2016-07-14 01:38:36]

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

[commit-bot: I haz the power, 2016-07-14 01:38:38]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-07-14 02:50:10]

Dana,

I've added three new unit tests. Two directly cover my mistakes in a previous
patchset. The third RenderWidgetHostViewAuraTest.TwoOutputSurfaces just
exercises a bunch of additional code paths within
DelegatedFrameHost::SwapDelegatedFrame for good measure. I'm CQ'ing. Please let
me know if you have any concerns and I can fix them in a subsequent patch.
Thanks!

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

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:513:
SendReclaimCompositorResources(output_surface_id, false /* is_swap_ack */);
On 2016/07/13 22:15:03, danakj wrote:
> On 2016/07/13 22:12:16, Fady Samuel wrote:
> > On 2016/07/13 20:59:14, piman wrote:
> > > this one is a swap ack.
> > 
> > Done.
> 
> Are there (can there be) tests that cover these mistakes?

This is now covered by
RenderWidgetHostViewAuraTest.ResettingCompositorReturnsResources

https://codereview.chromium.org/2147873003/diff/20001/content/browser/rendere...
content/browser/renderer_host/delegated_frame_host.cc:535:
pending_delegated_ack_count_--;
On 2016/07/13 22:12:16, Fady Samuel wrote:
> On 2016/07/13 20:59:14, piman wrote:
> > this (the DCHECK and the decrement) should only happen if is_swap_ack is
true.
> 
> Done.

This is now covered by RenderWidgetHostViewAuraTest.ReturnedResources

[Fady Samuel, 2016-07-14 02:50:20]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-14 02:50:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
boliu@chromium.org, piman@chromium.org, mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/2147873003/#ps70001
(title: "Added three unit tests")

[commit-bot: I haz the power, 2016-07-14 02:50:35]

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, 2016-07-14 02:52:31]

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

[commit-bot: I haz the power, 2016-07-14 02:52:33]

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...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[dcheng, 2016-07-14 03:17:43]

(This is probably a substantial enough change it should have gone through ipc
review again. In any case, the changes still lgtm)

[Fady Samuel, 2016-07-14 03:30:28]

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

[commit-bot: I haz the power, 2016-07-14 03:30:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-07-14 03:33:47]

On 2016/07/14 03:17:43, dcheng wrote:
> (This is probably a substantial enough change it should have gone through ipc
> review again. In any case, the changes still lgtm)

In spectrum between rapid forward progress, and careful review iteration prior
to commit, I think I may have skewed too much towards the former side in this
CL. Apologies. I'll do a dry run and wait for tomorrow. This isn't strictly
blocking other work.

[commit-bot: I haz the power, 2016-07-14 04:08:33]

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

[commit-bot: I haz the power, 2016-07-14 04:08:34]

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_...)

[Fady Samuel, 2016-07-14 04:35:49]

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

[commit-bot: I haz the power, 2016-07-14 04:36:03]

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, 2016-07-14 06:35:13]

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

[commit-bot: I haz the power, 2016-07-14 06:35:14]

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_...)

[Fady Samuel, 2016-07-14 11:13:58]

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

[commit-bot: I haz the power, 2016-07-14 11:14:20]

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, 2016-07-14 11:31:32]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 12:02:08]

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

[commit-bot: I haz the power, 2016-07-14 12:02:09]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-07-14 19:55:44]

Thanks for the tests! LGTM

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1633:
view_->InitAsChild(NULL);
nullptr

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1643: //
Swappinga  frame should trigger a swap ACK IPC because we have reset the
Swapping a frame

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1654:
EXPECT_EQ(true, std::get<1>(params));  // is_swap_ack
EXPECT_TRUE (i've seen EQ(true/false) fail to compile on some platforms?

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1685:
EXPECT_EQ(false, std::get<1>(params));  // is_swap_ack
EXPECT_FALSE

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1690: //
DelegateFrameHost
weird line wrapping

[Fady Samuel, 2016-07-14 23:17:04]

Thanks for your review, Dana! I'm CQ'ing now!

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1633:
view_->InitAsChild(NULL);
On 2016/07/14 19:55:44, danakj wrote:
> nullptr

Done. I just did a s/NULL/nullptr

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1643: //
Swappinga  frame should trigger a swap ACK IPC because we have reset the
On 2016/07/14 19:55:44, danakj wrote:
> Swapping a frame

Done.

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1654:
EXPECT_EQ(true, std::get<1>(params));  // is_swap_ack
On 2016/07/14 19:55:44, danakj wrote:
> EXPECT_TRUE (i've seen EQ(true/false) fail to compile on some platforms?

Done.

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1685:
EXPECT_EQ(false, std::get<1>(params));  // is_swap_ack
On 2016/07/14 19:55:44, danakj wrote:
> EXPECT_FALSE

Done.

https://codereview.chromium.org/2147873003/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1690: //
DelegateFrameHost
On 2016/07/14 19:55:44, danakj wrote:
> weird line wrapping

Done.

[Fady Samuel, 2016-07-14 23:27:47]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-14 23:27:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, boliu@chromium.org, dcheng@chromium.org,
piman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2147873003/#ps130001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-07-14 23:28:31]

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, 2016-07-14 23:28:35]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-14 23:31:14]

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

[commit-bot: I haz the power, 2016-07-14 23:31:17]

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...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Fady Samuel, 2016-07-14 23:56:02]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-07-14 23:56:03]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
mbarbella@chromium.org, boliu@chromium.org, dcheng@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2147873003/#ps150001
(title: "Rebased")

[commit-bot: I haz the power, 2016-07-14 23:56:24]

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, 2016-07-14 23:56:29]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[Fady Samuel, 2016-07-15 00:03:06]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-15 01:14:45]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-15 01:14:47]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-07-15 01:14:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 01:17:54]

Description was changed from

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and 
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to 
return resources to clients. The only difference is one acknowledges a 
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL 
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer 
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Dedup IPCs to return resources to client

ViewMsg_ReclaimCompositorResources and
ViewMsg_SwapCompositorFrameAck are virtually identical IPCs to
return resources to clients. The only difference is one acknowledges a
swap and the other returns resources independent of a swap.
This is both confusing and an unnecessary maintenance burden. This CL
deletes ViewMsg_SwapCompositorFrameAck and replaces it with
ViewMsg_ReclaimCompositorResources and a bool for is_swap_ack.

This CL also removes CompositorFrameAck which is no longer
necessary as it only contains one field.

BUG=none
TBR=ben@chromium.org for services/ui/public/cpp/lib/output_surface.cc
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b62b78229e838b0140af7973e83ada9fcce28a19
Cr-Commit-Position: refs/heads/master@{#405660}
==========

[commit-bot: I haz the power, 2016-07-15 01:17:56]

Patchset 9 (id:??) landed as
https://crrev.com/b62b78229e838b0140af7973e83ada9fcce28a19
Cr-Commit-Position: refs/heads/master@{#405660}