Split SurfaceFactoryClient Into Four Interfaces

Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

SurfaceAggregator::PreWalkTree() no longer goes through SurfaceFactory
when calling WillDrawSurface.

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

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

[Alex Z., 2017-04-18 19:58:54]

Description was changed from

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

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

to

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

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

[Alex Z., 2017-04-18 19:58:59]

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

[commit-bot: I haz the power, 2017-04-18 19:59:41]

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-18 20:05:29]

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

[commit-bot: I haz the power, 2017-04-18 20:05:42]

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-18 20:07:10]

Description was changed from

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

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

to

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

SurfaceAggregator::PreWalkTree() no longer goes through SurfaceFactory
when calling WillDrawSurface.

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

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

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

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

danakj@: This CL splits SurfaceFactoryClient into 4 interfaces
as we discussed this morning. PTAL.

[Fady Samuel, 2017-04-18 20:13:51]

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

[Fady Samuel, 2017-04-18 20:13:51]

I really like this! Thanks! LGTM + nit.

https://codereview.chromium.org/2824053003/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_client.h (right):

https://codereview.chromium.org/2824053003/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_client.h:17: virtual void WillDrawSurface(const
LocalSurfaceId& local_surface_id,
Describe when this is called.

[Alex Z., 2017-04-18 20:24:38]

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

[commit-bot: I haz the power, 2017-04-18 20:24:51]

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-18 20:24:56]

https://codereview.chromium.org/2824053003/diff/20001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_client.h (right):

https://codereview.chromium.org/2824053003/diff/20001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_client.h:17: virtual void WillDrawSurface(const
LocalSurfaceId& local_surface_id,
On 2017/04/18 20:13:51, Fady Samuel wrote:
> Describe when this is called.

Done.

[commit-bot: I haz the power, 2017-04-18 22:27:34]

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

[commit-bot: I haz the power, 2017-04-18 22:27:35]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-19 14:52:57]

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_manager_client.h (right):

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_manager_client.h:14: virtual ~FrameSinkManagerClient() {}
=default

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.h (right):

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.h:46: // Associates a FrameSinkManagerClient with
the  frame_sink_id it  uses.
bonus whitespace after "with the"

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

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:761:
surface->factory()->surface_aggregator_client()->WillDrawSurface(
Ah ok this one isn't what I expected. A SurfaceAggregatorClient would normally
be given to and held in SurfaceAggregator.

This is still going through the surface (and factory), so maybe this expresses
the wrong relationship.

The goal is to get rid of SurfaceFactory right? This gets closer in that the
call goes directly to CFSSupport. If SurfaceFactory stopped existing though,
since we're still depending on the factory() right now, how would this look?

[Alex Z., 2017-04-19 15:03:44]

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

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:761:
surface->factory()->surface_aggregator_client()->WillDrawSurface(
On 2017/04/19 14:52:56, danakj wrote:
> Ah ok this one isn't what I expected. A SurfaceAggregatorClient would normally
> be given to and held in SurfaceAggregator.
> 
> This is still going through the surface (and factory), so maybe this expresses
> the wrong relationship.
> 
> The goal is to get rid of SurfaceFactory right? This gets closer in that the
> call goes directly to CFSSupport. If SurfaceFactory stopped existing though,
> since we're still depending on the factory() right now, how would this look?

Surface::factory() will be replaced with a 
compositor_frame_sink_support() (WIP CL: 
https://codereview.chromium.org/2802023002/).

One alternative is that we have something like 
RegisterSurfaceAggregatorClient and call it in 
CFSSupport::Init(). SurfaceAggregator then does a lookup based
on the FrameSinkId to acquire the client and call 
WillDrawSurface(). WDYT?

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

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

[commit-bot: I haz the power, 2017-04-19 15:15:37]

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 15:15:40]

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/frame_sink_...
File cc/surfaces/frame_sink_manager_client.h (right):

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/frame_sink_...
cc/surfaces/frame_sink_manager_client.h:14: virtual ~FrameSinkManagerClient() {}
On 2017/04/19 14:52:56, danakj wrote:
> =default

Done. I also changed the dtor to default in the other 3 interfaces.

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.h (right):

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.h:46: // Associates a FrameSinkManagerClient with
the  frame_sink_id it  uses.
On 2017/04/19 14:52:56, danakj wrote:
> bonus whitespace after "with the"

Done.

[commit-bot: I haz the power, 2017-04-19 16:24:25]

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

[commit-bot: I haz the power, 2017-04-19 16:24:27]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-04-19 18:07:55]

Sorry for the slow reply, I had to think about this for a while.

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

https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator.cc:761:
surface->factory()->surface_aggregator_client()->WillDrawSurface(
On 2017/04/19 15:03:44, Alex Z. wrote:
> On 2017/04/19 14:52:56, danakj wrote:
> > Ah ok this one isn't what I expected. A SurfaceAggregatorClient would
normally
> > be given to and held in SurfaceAggregator.
> > 
> > This is still going through the surface (and factory), so maybe this
expresses
> > the wrong relationship.
> > 
> > The goal is to get rid of SurfaceFactory right? This gets closer in that the
> > call goes directly to CFSSupport. If SurfaceFactory stopped existing though,
> > since we're still depending on the factory() right now, how would this look?
> 
> Surface::factory() will be replaced with a 
> compositor_frame_sink_support() (WIP CL: 
> https://codereview.chromium.org/2802023002/).
> 
> One alternative is that we have something like 
> RegisterSurfaceAggregatorClient and call it in 
> CFSSupport::Init(). SurfaceAggregator then does a lookup based
> on the FrameSinkId to acquire the client and call 
> WillDrawSurface(). WDYT?

A surface can be inserted into multiple displays right, so I think that's how
this came to be. Each display (with its own SurfaceAggregator) needs to find the
client of the Surface(Factory) to tell it the frame is submitted. So every
SurfaceAggregator would need to know about every CFSSupport, not the best.

ok so conceptually what we have is the user of CompositorFrameSinkSupport (ie
DelegatedFrameHost) submits frames and wants to hear when a child surface's
frame has damage.

Internally frames get turned into cc::Surface which is what the aggregator sees.

I see two things here:
1. The callback is noisier than it needs to be. The client is interested in
damage to child surfaces, but it reports on every draw regardless of damage, and
it also reports damage in the submitted frame that the client already knows
about, instead of only reporting when a child has damage. That's unrelated
really but pointing it out cuz I was thinking thru the name WillDrawSurface
which is accurate but pointed this out to me.
2. These interfaces are some pretty heavy plumbing, including through weak
pointers. The other callback of this sort is the draw callback, called via
Surface::RunDrawCallbacks. It looks to me that this WillDrawSurface() is trying
to go to the same places as the DrawCallback, so I'd suggest making them
similar. DrawCallback is done  with base::Bind, which could work here. What if
when CompositorFrameSinkSupport submits a frame, it includes a
base::RepeatingCallback for this. And Surface adds a method like
RunDrawCallbacks for the willdraw case.

Then we go SurfaceAggregator -> Surface (a concrete type it has to know as it
receives it) -> CompositorFrameSink -> CompositorFrameSinkClient. We avoid
SurfaceFactory needing to know about the details, and we avoid having to go
through the factory on the way back.

Once the factory is gone, probably neither of these need to be a Callback and
could be a SurfaceClient interface implemented by CompositorFrameSinkSupport..
but would this work for now?

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

On 2017/04/19 18:07:55, danakj wrote:
> Sorry for the slow reply, I had to think about this for a while.
> 
>
https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/surface_agg...
> File cc/surfaces/surface_aggregator.cc (right):
> 
>
https://codereview.chromium.org/2824053003/diff/40001/cc/surfaces/surface_agg...
> cc/surfaces/surface_aggregator.cc:761:
> surface->factory()->surface_aggregator_client()->WillDrawSurface(
> On 2017/04/19 15:03:44, Alex Z. wrote:
> > On 2017/04/19 14:52:56, danakj wrote:
> > > Ah ok this one isn't what I expected. A SurfaceAggregatorClient would
> normally
> > > be given to and held in SurfaceAggregator.
> > > 
> > > This is still going through the surface (and factory), so maybe this
> expresses
> > > the wrong relationship.
> > > 
> > > The goal is to get rid of SurfaceFactory right? This gets closer in that
the
> > > call goes directly to CFSSupport. If SurfaceFactory stopped existing
though,
> > > since we're still depending on the factory() right now, how would this
look?
> > 
> > Surface::factory() will be replaced with a 
> > compositor_frame_sink_support() (WIP CL: 
> > https://codereview.chromium.org/2802023002/).
> > 
> > One alternative is that we have something like 
> > RegisterSurfaceAggregatorClient and call it in 
> > CFSSupport::Init(). SurfaceAggregator then does a lookup based
> > on the FrameSinkId to acquire the client and call 
> > WillDrawSurface(). WDYT?
> 
> A surface can be inserted into multiple displays right, so I think that's how
> this came to be. Each display (with its own SurfaceAggregator) needs to find
the
> client of the Surface(Factory) to tell it the frame is submitted. So every
> SurfaceAggregator would need to know about every CFSSupport, not the best.
> 
> ok so conceptually what we have is the user of CompositorFrameSinkSupport (ie
> DelegatedFrameHost) submits frames and wants to hear when a child surface's
> frame has damage.
> 
> Internally frames get turned into cc::Surface which is what the aggregator
sees.
> 
> I see two things here:
> 1. The callback is noisier than it needs to be. The client is interested in
> damage to child surfaces, but it reports on every draw regardless of damage,
and
> it also reports damage in the submitted frame that the client already knows
> about, instead of only reporting when a child has damage. That's unrelated
> really but pointing it out cuz I was thinking thru the name WillDrawSurface
> which is accurate but pointed this out to me.
> 2. These interfaces are some pretty heavy plumbing, including through weak
> pointers. The other callback of this sort is the draw callback, called via
> Surface::RunDrawCallbacks. It looks to me that this WillDrawSurface() is
trying
> to go to the same places as the DrawCallback, so I'd suggest making them
> similar. DrawCallback is done  with base::Bind, which could work here. What if
> when CompositorFrameSinkSupport submits a frame, it includes a
> base::RepeatingCallback for this. And Surface adds a method like
> RunDrawCallbacks for the willdraw case.
> 
> Then we go SurfaceAggregator -> Surface (a concrete type it has to know as it
> receives it) -> CompositorFrameSink -> CompositorFrameSinkClient. We avoid
> SurfaceFactory needing to know about the details, and we avoid having to go
> through the factory on the way back.
> 
> Once the factory is gone, probably neither of these need to be a Callback and
> could be a SurfaceClient interface implemented by CompositorFrameSinkSupport..
> but would this work for now?

Yes, replacing WillDrawSurface with a RepeatingCallback should work for now.

[Alex Z., 2017-04-21 14:52:56]

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

[commit-bot: I haz the power, 2017-04-21 14:53:16]

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-21 14:54:22]

fsamuel@, danakj@: I rebased the CL after the WillDrawCallback
landed. Please take another look :).

[danakj, 2017-04-21 15:06:48]

LGTM w/ a few comments

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.h (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.h:46: // Associates a FrameSinkManagerClient with
the frame_sink_id it  uses.
heh, extra space before "uses" too. weird.

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_perftest.cc (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_perftest.cc:29: class EmptyClient : public
SurfaceFactoryClient,
I suggest breaking this into three StubFooClients. They could go in cc/test/ if
you want as well for reuse, up to you.

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory_unittest.cc (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory_unittest.cc:41: class TestClient : public
SurfaceFactoryClient,
also suggest splitting this up. It looks like this would be the same 2 stubs
from the perftest, and a FakeSurfaceResourceHolderClient that lets you see the
returned resources. (FWIW the ReturnResources checks could be done with mock as
well with ElementsAre() - one example
https://cs.chromium.org/chromium/src/cc/output/gl_renderer_unittest.cc?rcl=41...)

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_res...
File cc/surfaces/surface_resource_holder_client.h (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_res...
cc/surfaces/surface_resource_holder_client.h:16: virtual void
ReturnResources(const ReturnedResourceArray& resources) = 0;
Document when this is called/what it should do.

[commit-bot: I haz the power, 2017-04-21 16:08:07]

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

[commit-bot: I haz the power, 2017-04-21 16:08:08]

Dry run: This issue passed the CQ dry run.

[Alex Z., 2017-04-21 18:14:28]

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

[Alex Z., 2017-04-21 18:14:37]

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/framesink_m...
File cc/surfaces/framesink_manager.h (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/framesink_m...
cc/surfaces/framesink_manager.h:46: // Associates a FrameSinkManagerClient with
the frame_sink_id it  uses.
On 2017/04/21 15:06:48, danakj wrote:
> heh, extra space before "uses" too. weird.

Done.

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_agg...
File cc/surfaces/surface_aggregator_perftest.cc (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_agg...
cc/surfaces/surface_aggregator_perftest.cc:29: class EmptyClient : public
SurfaceFactoryClient,
On 2017/04/21 15:06:48, danakj wrote:
> I suggest breaking this into three StubFooClients. They could go in cc/test/
if
> you want as well for reuse, up to you.

It now uses StubSurfaceFactoryClient. I left a 
StubSurfaceResourceHolderClient in here since I don't know if there's
any performance impact on using the fake class (storing returned 
resources) and this is the only place that's using the stub class.

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory_unittest.cc (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory_unittest.cc:41: class TestClient : public
SurfaceFactoryClient,
On 2017/04/21 15:06:48, danakj wrote:
> also suggest splitting this up. It looks like this would be the same 2 stubs
> from the perftest, and a FakeSurfaceResourceHolderClient that lets you see the
> returned resources. (FWIW the ReturnResources checks could be done with mock
as
> well with ElementsAre() - one example
>
https://cs.chromium.org/chromium/src/cc/output/gl_renderer_unittest.cc?rcl=41...)

I created StubSurfaceFactoryClient and FakeSurfaceResourceHolderClient
classes.

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_res...
File cc/surfaces/surface_resource_holder_client.h (right):

https://codereview.chromium.org/2824053003/diff/80001/cc/surfaces/surface_res...
cc/surfaces/surface_resource_holder_client.h:16: virtual void
ReturnResources(const ReturnedResourceArray& resources) = 0;
On 2017/04/21 15:06:48, danakj wrote:
> Document when this is called/what it should do.

Done.

[commit-bot: I haz the power, 2017-04-21 18:14:56]

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-21 18:16:50]

Thanks! LGTM

[Fady Samuel, 2017-04-21 19:04:18]

A couple of nits but LGTM otherwise.

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
File cc/surfaces/framesink_manager.h (left):

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
cc/surfaces/framesink_manager.h:49: void RegisterSurfaceFactoryClient(const
FrameSinkId& frame_sink_id,
nit: RegisterFrameSinkManagerClient

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
cc/surfaces/framesink_manager.h:51: void UnregisterSurfaceFactoryClient(const
FrameSinkId& frame_sink_id);
nit: UnregisterFrameSinkManagerClient

[commit-bot: I haz the power, 2017-04-21 19:21:00]

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

[commit-bot: I haz the power, 2017-04-21 19:21:01]

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by staraz@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2824053003/#ps120001
(title: "Address Nits")

[commit-bot: I haz the power, 2017-04-21 21:11:11]

CQ is trying da patch.

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

[Alex Z., 2017-04-21 21:11:32]

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
File cc/surfaces/framesink_manager.h (left):

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
cc/surfaces/framesink_manager.h:49: void RegisterSurfaceFactoryClient(const
FrameSinkId& frame_sink_id,
On 2017/04/21 19:04:18, Fady Samuel wrote:
> nit: RegisterFrameSinkManagerClient

Done.

https://codereview.chromium.org/2824053003/diff/100001/cc/surfaces/framesink_...
cc/surfaces/framesink_manager.h:51: void UnregisterSurfaceFactoryClient(const
FrameSinkId& frame_sink_id);
On 2017/04/21 19:04:18, Fady Samuel wrote:
> nit: UnregisterFrameSinkManagerClient

Done.

[commit-bot: I haz the power, 2017-04-21 22:39:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492809049086340,
"parent_rev": "423d7c1b4001927a1a6f6180aa0f3eebe72348e4", "commit_rev":
"dcf699d9ec671447372b367a375a622271396db5"}

[commit-bot: I haz the power, 2017-04-21 22:39:11]

Description was changed from

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

SurfaceAggregator::PreWalkTree() no longer goes through SurfaceFactory
when calling WillDrawSurface.

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

to

==========
Split SurfaceFactoryClient Into Four Interfaces

SurfaceFactoryClient has four methods and each one of them is being used by
a different class: ReferencedSurfacesChanged is used by SurfaceFactory;
ReturnResources is used by SurfaceResourceHolder; WillDrawSurface is called by
SurfaceFactory but SurfaceFactory is simply forwarding the call from
SurfaceAggregator; SetBeginFrameSource is used by FrameSinkManager.

Giving the four classes their own client types allows the implementation class
to be more flexible. The implementation class would no longer have to implement
all four methods when only some of them are needed.

SurfaceAggregator::PreWalkTree() no longer goes through SurfaceFactory
when calling WillDrawSurface.

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

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

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

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