Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink

components/display_compositor/gpu_display_compositor_frame_sink.cc

 // Copyright 2017 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 "components/display_compositor/gpu_display_compositor_frame_sink.h"
 
 namespace display_compositor {
 
 GpuDisplayCompositorFrameSink::GpuDisplayCompositorFrameSink(
     GpuCompositorFrameSinkDelegate* delegate,
     cc::SurfaceManager* surface_manager,
     const cc::FrameSinkId& frame_sink_id,
     std::unique_ptr<cc::Display> display,
     std::unique_ptr<cc::BeginFrameSource> begin_frame_source,
     cc::mojom::MojoCompositorFrameSinkAssociatedRequest request,
     cc::mojom::MojoCompositorFrameSinkPrivateRequest
         compositor_frame_sink_private_request,
     cc::mojom::MojoCompositorFrameSinkClientPtr client,
     cc::mojom::DisplayPrivateAssociatedRequest display_private_request)
     : GpuCompositorFrameSink(delegate,
                              surface_manager,
                              frame_sink_id,
-                             display.get(),
                              std::move(compositor_frame_sink_private_request),
-                             std::move(client)),
+                             std::move(client),
+                             true /* has_display */),
       binding_(this, std::move(request)),
       display_private_binding_(this, std::move(display_private_request)),
       display_begin_frame_source_(std::move(begin_frame_source)),
       display_(std::move(display)) {
   binding_.set_connection_error_handler(
       base::Bind(&GpuDisplayCompositorFrameSink::OnClientConnectionLost,
                  base::Unretained(this)));
+  display_->Initialize(this, surface_manager);
   display_->SetVisible(true);
 }
 
 GpuDisplayCompositorFrameSink::~GpuDisplayCompositorFrameSink() = default;
 
+void GpuDisplayCompositorFrameSink::SubmitCompositorFrame(

[danakj, 2017/02/08 19:23:38]

Overloading a method that has an impl in the parent class feels kinda bad and
makes me wonder if we have the right hierarchy? Should this merge with its
parent? Something else?

[Alex Z., 2017/02/08 21:26:47]

As discussed on slack, DisplayPrivate::SetLocalSurfaceId() is added.
FrameGenerator calls SetLocalSurfaceId() before submitting a
CompositorFrame to avoid this override.

+    const cc::LocalSurfaceId& local_surface_id,
+    cc::CompositorFrame frame) {
+  display_->SetLocalSurfaceId(local_surface_id,
+                              frame.metadata.device_scale_factor);
+
+  GpuCompositorFrameSink::SubmitCompositorFrame(local_surface_id,
+                                                std::move(frame));
+}
+
 void GpuDisplayCompositorFrameSink::SetDisplayVisible(bool visible) {
-  DCHECK(support_.display());
+  DCHECK(display_);
   display_->SetVisible(visible);
 }
 
 void GpuDisplayCompositorFrameSink::ResizeDisplay(const gfx::Size& size) {
-  DCHECK(support_.display());
+  DCHECK(display_);
   display_->Resize(size);
 }
 
 void GpuDisplayCompositorFrameSink::SetDisplayColorSpace(
     const gfx::ColorSpace& color_space) {
-  DCHECK(support_.display());
+  DCHECK(display_);
   display_->SetColorSpace(color_space);
 }
 
 void GpuDisplayCompositorFrameSink::SetOutputIsSecure(bool secure) {
-  DCHECK(support_.display());
+  DCHECK(display_);
   display_->SetOutputIsSecure(secure);
 }
 
+void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {}

[danakj, 2017/02/08 19:23:38]

Why does the client not want to hear about the context being lost?

[Alex Z., 2017/02/08 21:26:47]

The client_ (assuming we are changing it from private to protected in 
GpuCompositorFrameSink) is a MojoCompositorFrameSinkClient. It 
doesn't have anything to call for context lost. I can add something 
like that on a separate CL if it's needed. Besides, these 
DisplayClient implementations are moved from 
CompositorFrameSinkSupport, which wasn't doing anything in the 
first place.

[danakj, 2017/02/08 21:42:43]

If the display's context/outputsurface is lost, then the client will have to
resubmit a frame with all its resources (and probably recreate them cuz its own
context is lost). This isn't a layer-tree specific part of the
cc::CompositorFrameSinkClient.
https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink_client.h...

Indeed it wasn't doing anything which was a bug you saw for DisplayCompositor
but other submitters of frames will want to know also. Let's make sure we follow
up. Perhaps the MojoCompositorFrameSinkClient just needs to lose its mojo
connection at that point or something, rather than adding another method - not
sure about that part. But the MojoCompositorFrameSink is dead if its output
surface/context is lost.

[Alex Z., 2017/02/08 21:50:08]

I added a TODO here.

+
+void GpuDisplayCompositorFrameSink::DisplayWillDrawAndSwap(
+    bool will_draw_and_swap,
+    const cc::RenderPassList& render_passes) {}
+
+void GpuDisplayCompositorFrameSink::DisplayDidDrawAndSwap() {}
+
 }  // namespace display_compositor