DirectCompositorFrameSink Uses CompositorFrameSinkSupport

cc/surfaces/compositor_frame_sink_support.cc

Index: cc/surfaces/compositor_frame_sink_support.cc
diff --git a/cc/surfaces/compositor_frame_sink_support.cc b/cc/surfaces/compositor_frame_sink_support.cc
index c36e35121216113a756f8550dfb60068a26f3b84..6a9531f037c849a2f6fe8989d59e03aefd74ab92 100644
--- a/cc/surfaces/compositor_frame_sink_support.cc
+++ b/cc/surfaces/compositor_frame_sink_support.cc
@@ -17,7 +17,7 @@ CompositorFrameSinkSupport::CompositorFrameSinkSupport(
     CompositorFrameSinkSupportClient* client,
     SurfaceManager* surface_manager,
     const FrameSinkId& frame_sink_id,
-    std::unique_ptr<Display> display,
+    Display* display,
     std::unique_ptr<BeginFrameSource> display_begin_frame_source)
     : client_(client),
       surface_manager_(surface_manager),
@@ -25,15 +25,7 @@ CompositorFrameSinkSupport::CompositorFrameSinkSupport(
       display_begin_frame_source_(std::move(display_begin_frame_source)),
       display_(std::move(display)),
       surface_factory_(frame_sink_id_, surface_manager_, this),
-      weak_factory_(this) {
-  surface_manager_->RegisterFrameSinkId(frame_sink_id_);
-  surface_manager_->RegisterSurfaceFactoryClient(frame_sink_id_, this);
-
-  if (display_) {
-    display_->Initialize(this, surface_manager_);
-    display_->SetVisible(true);
-  }
-}
+      weak_factory_(this) {}
 
 CompositorFrameSinkSupport::~CompositorFrameSinkSupport() {
   for (auto& child_frame_sink_id : child_frame_sinks_) {
@@ -41,12 +33,6 @@ CompositorFrameSinkSupport::~CompositorFrameSinkSupport() {
     surface_manager_->UnregisterFrameSinkHierarchy(frame_sink_id_,
                                                    child_frame_sink_id);
   }
-  // SurfaceFactory's destructor will attempt to return resources which will
-  // call back into here and access |client_| so we should destroy
-  // |surface_factory_|'s resources early on.
-  surface_factory_.EvictSurface();
-  surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_);
-  surface_manager_->InvalidateFrameSinkId(frame_sink_id_);
 }
 
 void CompositorFrameSinkSupport::EvictFrame() {
@@ -132,6 +118,45 @@ void CompositorFrameSinkSupport::RemoveChildFrameSink(
   child_frame_sinks_.erase(it);
 }
 
+void CompositorFrameSinkSupport::SetFactoryNeedsSyncPoints(

[Fady Samuel, 2017/01/04 20:59:25]

nit: Call it SetNeedsSyncPoints?

[Alex Z., 2017/01/04 21:36:09]

Done.

+    bool needs_sync_points) {
+  surface_factory_.set_needs_sync_points(needs_sync_points);
+}
+
+void CompositorFrameSinkSupport::ForceReclaimResources() {
+  if (local_frame_id_.is_valid())
+    surface_factory_.ClearSurface();
+}
+
+void CompositorFrameSinkSupport::BindToClient() {

[Fady Samuel, 2017/01/04 20:59:25]

Instead of calling BindToClient, create the CompositorFrameSinkSupport in
DirectCompositorFrameSink::BindToClient()

[Alex Z., 2017/01/04 21:36:09]

Done.

+  surface_manager_->RegisterSurfaceFactoryClient(frame_sink_id_, this);
+  if (display_)
+    display_->Initialize(this, surface_manager_);
+}
+
+void CompositorFrameSinkSupport::DetachFromClient() {

[Fady Samuel, 2017/01/04 20:59:25]

Instead of calling DetachFromClient here, destroy the CompositorFrameSinkSupport
in DirectCompositorFrameSink::DetachFromClient.

[Alex Z., 2017/01/04 21:36:09]

Done.

+  // Unregister the SurfaceFactoryClient here instead of the dtor so that only
+  // one client is alive for this namespace at any given time.
+  surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_);
+  surface_factory_.EvictSurface();
+}
+
+void CompositorFrameSinkSupport::SetDisplayVisible(bool visible) {

[Fady Samuel, 2017/01/04 20:59:25]

No need to introduce this. support_->display()->SetVisible(...)

[Alex Z., 2017/01/04 21:36:10]

Done.

+  if (display_)
+    display_->SetVisible(visible);
+}
+
+void CompositorFrameSinkSupport::RegisterFrameSinkId() {
+  surface_manager_->RegisterFrameSinkId(frame_sink_id_);

[Fady Samuel, 2017/01/04 20:59:25]

Remove this. This is unnecessary.

[Alex Z., 2017/01/04 21:36:09]

DirectCompositorFrameSink doesn't register and invalidate FrameSinkId.
Doing it triggers the DCHECK at 
https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?sq=packag....

+}
+
+void CompositorFrameSinkSupport::InvalidateFrameSinkId() {

[Fady Samuel, 2017/01/04 20:59:25]

Remove this. This is unnecessary.

[Alex Z., 2017/01/04 21:36:10]

DirectCompositorFrameSink doesn't register and invalidate FrameSinkId.
Doing it triggers the DCHECK at 
https://cs.chromium.org/chromium/src/cc/surfaces/surface_manager.cc?sq=packag....

+  // SurfaceFactory's destructor will attempt to return resources which will
+  // call back into here and access |client_| so we should destroy
+  // |surface_factory_|'s resources early on.
+  surface_manager_->InvalidateFrameSinkId(frame_sink_id_);
+}
+
 void CompositorFrameSinkSupport::DisplayOutputSurfaceLost() {}
 
 void CompositorFrameSinkSupport::DisplayWillDrawAndSwap(