Split SurfaceFactoryClient Into Four Interfaces

cc/surfaces/surface_aggregator.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/surfaces/surface_aggregator.h"
 
 #include <stddef.h>
 
 #include <map>
 
 #include "base/bind.h"
 #include "base/containers/adapters.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/math_util.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/quads/draw_quad.h"
 #include "cc/quads/render_pass_draw_quad.h"
 #include "cc/quads/shared_quad_state.h"
 #include "cc/quads/solid_color_draw_quad.h"
 #include "cc/quads/surface_draw_quad.h"
 #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"
 
 namespace cc {
 namespace {
 
 // Maximum bucket size for the UMA stats.
 constexpr int kUmaStatMaxSurfaces = 30;
 
@@ skipping 711 matching lines @@
   CHECK(debug_weak_this.get());
   for (const auto& surface_id : frame.metadata.referenced_surfaces) {
     if (!contained_surfaces_.count(surface_id)) {
       result->undrawn_surfaces.insert(surface_id);
       PrewalkTree(surface_id, false, 0, result);
     }
   }
 
   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);
   }
 
   CHECK(debug_weak_this.get());
   for (const auto& render_pass : frame.render_pass_list) {
     if (!render_pass->copy_requests.empty()) {
       int remapped_pass_id = RemapPassId(render_pass->id, surface_id);
       copy_request_passes_.insert(remapped_pass_id);
     }
   }
@@ skipping 160 matching lines @@
 }
 
 void SurfaceAggregator::SetOutputColorSpace(
     const gfx::ColorSpace& blending_color_space,
     const gfx::ColorSpace& output_color_space) {
   blending_color_space_ = blending_color_space;
   output_color_space_ = output_color_space;
 }
 
 }  // namespace cc