Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2834553002
Cr-Commit-Position: refs/heads/master@{#466318}
Committed: https://chromium.googlesource.com/chromium/src/+/0838e367b020c960036c1e706b40f5bfd3226e32

[Alex Z., 2017-04-19 22:29:52]

Description was changed from

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
==========

to

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Alex Z., 2017-04-19 22:30:15]

Description was changed from

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Alex Z., 2017-04-19 23:27:27]

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

[commit-bot: I haz the power, 2017-04-19 23:28:22]

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

[Alex Z., 2017-04-19 23:29:30]

staraz@chromium.org changed reviewers:
+ danakj@chromium.org, fsamuel@chromium.org

[Alex Z., 2017-04-19 23:29:31]

danakj@, fsamuel@: SurfaceFactory::SubmitCompositorFrame takes
a WillDrawCallback and SurfaceAggregator no longer goes 
through factory to call WillDraw in this CL. PTAL.

[commit-bot: I haz the power, 2017-04-20 00:02:58]

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

[commit-bot: I haz the power, 2017-04-20 00:02:59]

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

[Fady Samuel, 2017-04-20 00:33:18]

Looks like it's generally moving in the right direction but a bunch of
cc_unittests still seem to be failing to compile. Please fix and I'll re-review.

[Fady Samuel, 2017-04-20 12:35:46]

Note to Dana: we currently add a callback on QueueFrame (I hate that name
because it's not really a queue), because ForceReclaimResources passes along
do-nothing callbacks because it just wants to clear the pending and active
frames. Once ForceReclaimResources goes away (kylechar@ is working on that),
then these callbacks exist for the lifetime of the surface and so we no longer
need to bundle them with the CompositorFrame in FrameData. For now, I think what
staraz@ is doing is correct, but it won't be necessary for long.

[Alex Z., 2017-04-20 12:41:25]

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

[commit-bot: I haz the power, 2017-04-20 12:41:38]

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

[Alex Z., 2017-04-20 12:41:39]

On 2017/04/20 00:33:18, Fady Samuel wrote:
> Looks like it's generally moving in the right direction but a bunch of
> cc_unittests still seem to be failing to compile. Please fix and I'll
re-review.

Done.

[Fady Samuel, 2017-04-20 12:45:48]

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:291: void Surface::RunWillDrawCallbacks(const gfx::Rect&
damage_rect) {
I'm really not sure why this is plural. RunWillDrawCallback is more appropriate.

Also, this code below is a bit harder to read (I know we do this elsewhere and I
didn't update the code:

if (!active_frame_data_ || active_frame_data_->will_draw_callback.is_null())
  return;

active_frame_data_->will_draw_callback.Run(surface_id_.local_surface_id(),
                                                                          
damage_rect);

This is maybe a bit easier to read.

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:29: #include "cc/surfaces/surface_factory.h"
Is this still necessary?

[danakj, 2017-04-20 13:56:45]

Is there no unit test for WillDrawCallback?

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:291: void Surface::RunWillDrawCallbacks(const gfx::Rect&
damage_rect) {
On 2017/04/20 12:45:48, Fady Samuel wrote:
> I'm really not sure why this is plural. RunWillDrawCallback is more
appropriate.
> 
> Also, this code below is a bit harder to read (I know we do this elsewhere and
I
> didn't update the code:
> 
> if (!active_frame_data_ || active_frame_data_->will_draw_callback.is_null())
>   return;
> 
> active_frame_data_->will_draw_callback.Run(surface_id_.local_surface_id(),
>                                                                           
> damage_rect);
> 
> This is maybe a bit easier to read.

+1 to RunWillDrawCallback. Early out is fine, but also doesn't seem important
for a one-line function. It gains more value when there's more nesting or longer
logic.

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:762: if (!damage_rect.IsEmpty())
Can you leave a TODO that this shouldn't need to call the callback when the
damage is from |surface| and not from |child_surfaces|?

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:763:
surface->RunWillDrawCallbacks(damage_rect);
Also, DelegatedFrameHost doesn't need to check for an empty rect now.

[Alex Z., 2017-04-20 13:58:05]

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:291: void Surface::RunWillDrawCallbacks(const gfx::Rect&
damage_rect) {
On 2017/04/20 12:45:48, Fady Samuel wrote:
> I'm really not sure why this is plural. RunWillDrawCallback is more
appropriate.
> 
> Also, this code below is a bit harder to read (I know we do this elsewhere and
I
> didn't update the code:
> 
> if (!active_frame_data_ || active_frame_data_->will_draw_callback.is_null())
>   return;
> 
> active_frame_data_->will_draw_callback.Run(surface_id_.local_surface_id(),
>                                                                           
> damage_rect);
> 
> This is maybe a bit easier to read.

Done. I got the plural form RunDrawCallbacks. I fixed both names.

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:29: #include "cc/surfaces/surface_factory.h"
On 2017/04/20 12:45:48, Fady Samuel wrote:
> Is this still necessary?

Done.

[Alex Z., 2017-04-20 14:04:41]

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

[commit-bot: I haz the power, 2017-04-20 14:04:55]

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

[Alex Z., 2017-04-20 14:05:04]

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:762: if (!damage_rect.IsEmpty())
On 2017/04/20 13:56:45, danakj wrote:
> Can you leave a TODO that this shouldn't need to call the callback when the
> damage is from |surface| and not from |child_surfaces|?

Done.

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:763:
surface->RunWillDrawCallbacks(damage_rect);
On 2017/04/20 13:56:45, danakj wrote:
> Also, DelegatedFrameHost doesn't need to check for an empty rect now.

Done. Does DFH need to check the local surface id? When would the DFH
have a different local surface id from the surface?

[commit-bot: I haz the power, 2017-04-20 14:17:11]

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

[commit-bot: I haz the power, 2017-04-20 14:17:12]

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

[danakj, 2017-04-20 14:18:43]

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:763:
surface->RunWillDrawCallbacks(damage_rect);
On 2017/04/20 14:05:03, Alex Z. wrote:
> On 2017/04/20 13:56:45, danakj wrote:
> > Also, DelegatedFrameHost doesn't need to check for an empty rect now.
> 
> Done. Does DFH need to check the local surface id? When would the DFH
> have a different local surface id from the surface?

I'm not sure, you could look at when each is set to reason it out?

Also - are there no unit tests that verify that WillDraw happens, I didn't see
one here? Would you be able to add one if not?

[Alex Z., 2017-04-20 14:20:16]

On 2017/04/20 14:18:43, danakj wrote:
>
https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
> File cc/surfaces/surface_aggregator.cc (right):
> 
>
https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:763:
> surface->RunWillDrawCallbacks(damage_rect);
> On 2017/04/20 14:05:03, Alex Z. wrote:
> > On 2017/04/20 13:56:45, danakj wrote:
> > > Also, DelegatedFrameHost doesn't need to check for an empty rect now.
> > 
> > Done. Does DFH need to check the local surface id? When would the DFH
> > have a different local surface id from the surface?
> 
> I'm not sure, you could look at when each is set to reason it out?
> 
> Also - are there no unit tests that verify that WillDraw happens, I didn't see
> one here? Would you be able to add one if not?

SurfaceAggregatorTest.SimpleFrame verifies that WillDraw is called: 
https://cs.chromium.org/chromium/src/cc/surfaces/surface_aggregator_unittest....

[Alex Z., 2017-04-20 17:15:58]

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

[commit-bot: I haz the power, 2017-04-20 17:16:48]

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

[Alex Z., 2017-04-20 17:42:26]

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/2834553002/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:763:
surface->RunWillDrawCallbacks(damage_rect);
On 2017/04/20 14:18:43, danakj wrote:
> On 2017/04/20 14:05:03, Alex Z. wrote:
> > On 2017/04/20 13:56:45, danakj wrote:
> > > Also, DelegatedFrameHost doesn't need to check for an empty rect now.
> > 
> > Done. Does DFH need to check the local surface id? When would the DFH
> > have a different local surface id from the surface?
> 
> I'm not sure, you could look at when each is set to reason it out?
> 
DelegatedFrameHost updates local_surface_id_ in 
SubmitdCompositorFrame after submitting the frame to SurfaceFactory.
I replaced the if in DFH with a DCHECK and the check failed during
resizing. It seems that there a race.

> Also - are there no unit tests that verify that WillDraw happens, I didn't see
> one here? Would you be able to add one if not?

SurfaceAggregatorTest.SimpleFrame verifies that WillDraw happens.

[danakj, 2017-04-20 17:56:45]

Thanks LG just needs some documentation

https://codereview.chromium.org/2834553002/diff/100001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2834553002/diff/100001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:56: void QueueFrame(CompositorFrame frame,
Can you add a comment explaining what these 2 callbacks are, when they are
called, because they sound awfully alike but are very different.

https://codereview.chromium.org/2834553002/diff/100001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2834553002/diff/100001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:509: // do not attempt
capture if |damage_rect| is empty.  This prevents the draws
This comment no longer works

[Alex Z., 2017-04-20 18:10:02]

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

[Alex Z., 2017-04-20 18:10:21]

https://codereview.chromium.org/2834553002/diff/100001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2834553002/diff/100001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:56: void QueueFrame(CompositorFrame frame,
On 2017/04/20 17:56:45, danakj wrote:
> Can you add a comment explaining what these 2 callbacks are, when they are
> called, because they sound awfully alike but are very different.

Done.

https://codereview.chromium.org/2834553002/diff/100001/content/browser/render...
File content/browser/renderer_host/delegated_frame_host.cc (right):

https://codereview.chromium.org/2834553002/diff/100001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:509: // do not attempt
capture if |damage_rect| is empty.  This prevents the draws
On 2017/04/20 17:56:45, danakj wrote:
> This comment no longer works

Done.

[commit-bot: I haz the power, 2017-04-20 18:10:40]

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

[danakj, 2017-04-20 19:16:48]

Thanks, LGTM

[Alex Z., 2017-04-20 19:24:59]

staraz@chromium.org changed reviewers:
+ sadrul@chromium.org

[Alex Z., 2017-04-20 19:25:00]

sadrul@chromium.org: Please review the name change in
render_widget_host_view_aura_unittest.cc

[commit-bot: I haz the power, 2017-04-20 20:08:49]

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

[commit-bot: I haz the power, 2017-04-20 20:08:50]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-04-20 22:02:34]

lgtm

[Fady Samuel, 2017-04-20 23:11:40]

Given this is a mechanical change in the unit test, I think it's fine to TBR
sadrul@.

[sadrul, 2017-04-21 03:19:03]

lgtm

[Alex Z., 2017-04-21 12:34:07]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2017-04-21 12:34:18]

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-04-21 12:39:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492778047983970,
"parent_rev": "2c772b93310fcf295f3de133e67095c7fe49a910", "commit_rev":
"0838e367b020c960036c1e706b40f5bfd3226e32"}

[commit-bot: I haz the power, 2017-04-21 12:39:25]

Description was changed from

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Replace CompositorFrameSinkSupport::WillDrawSurface With RepeatingCallback

As discussed in another CL (https://codereview.chromium.org/2824053003/),
WillDrawSurface is noisier than it needs to be. It is called on every draw
regardless of damage and reports damage in the submitted frame that the client
already knows about while the client is only interested in damage to child
surfaces.

By replacing WillDrawSurface with a base::RepeatingCallback, SurfaceFactory
doesn't need to know about the details and we avoid having to go through the
factory on the way back.

BUG=707105
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2834553002
Cr-Commit-Position: refs/heads/master@{#466318}
Committed:
https://chromium.googlesource.com/chromium/src/+/0838e367b020c960036c1e706b40...
==========

[commit-bot: I haz the power, 2017-04-21 12:39:26]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/0838e367b020c960036c1e706b40...