Split SurfaceFactoryClient Into Four Interfaces

cc/surfaces/surface_aggregator.cc

Index: cc/surfaces/surface_aggregator.cc
diff --git a/cc/surfaces/surface_aggregator.cc b/cc/surfaces/surface_aggregator.cc
index a5e31ddc480f8a6f040c0b4cb7c8a3ac25e0ce35..5d78365b0b4088439d9b32f080801aeec5cab5cd 100644
--- a/cc/surfaces/surface_aggregator.cc
+++ b/cc/surfaces/surface_aggregator.cc
@@ -26,6 +26,7 @@
 #include "cc/quads/texture_draw_quad.h"
 #include "cc/resources/resource_provider.h"
 #include "cc/surfaces/surface.h"
+#include "cc/surfaces/surface_aggregator_client.h"
 #include "cc/surfaces/surface_factory.h"
 #include "cc/surfaces/surface_manager.h"
 #include "cc/trees/blocking_task_runner.h"
@@ -757,7 +758,7 @@ gfx::Rect SurfaceAggregator::PrewalkTree(const SurfaceId& surface_id,
 
   CHECK(debug_weak_this.get());
   if (surface->factory()) {
-    surface->factory()->WillDrawSurface(
+    surface->factory()->surface_aggregator_client()->WillDrawSurface(

[danakj, 2017/04/19 14:52:56]

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]

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?

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

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?

         surface->surface_id().local_surface_id(), damage_rect);
   }