Make SurfaceFactory lifetime match frame_sink_id_ registration in BlimpCompositor

blimp/client/core/compositor/blimp_compositor.cc

Index: blimp/client/core/compositor/blimp_compositor.cc
diff --git a/blimp/client/core/compositor/blimp_compositor.cc b/blimp/client/core/compositor/blimp_compositor.cc
index 4b94dbcfab23b67609f7b84d2dd4466f1e8ebbe5..649c7546ebbd6f6042abada4ca3abed1e4c8c5fc 100644
--- a/blimp/client/core/compositor/blimp_compositor.cc
+++ b/blimp/client/core/compositor/blimp_compositor.cc
@@ -149,6 +149,8 @@ BlimpCompositor::BlimpCompositor(
 void BlimpCompositor::Initialize() {
   surface_id_allocator_ = base::MakeUnique<cc::SurfaceIdAllocator>();
   GetEmbedderDeps()->GetSurfaceManager()->RegisterFrameSinkId(frame_sink_id_);
+  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
+      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
   host_ = CreateLayerTreeHost();
 
   if (use_threaded_layer_tree_host_) {
@@ -184,7 +186,7 @@ void BlimpCompositor::RequestCopyOfOutput(
     std::unique_ptr<cc::CopyOutputRequest> copy_request,
     bool flush_pending_update) {
   // If we don't have a FrameSink, fail right away.
-  if (!surface_factory_)
+  if (!proxy_client_)

[piman, 2016/11/07 22:31:38]

Here and other places: even though proxy_client_ is a WeakPtr, it looks to have
exactly the same lifetime as what surface_factory_ used to have. It is set to a
!null value in BlimpCompositor::BindToProxyClient (which is called from the
ProxyClient), and reset to null in BlimpCompositor::UnbindProxyClient (which is
also called from the ProxyClient, i.e. it hasn't been destroyed yet). So I think
this transformation is valid, and preferable to add another boolean state
("bound to proxy"), but LMK if I missed something.

[Khushal, 2016/11/08 00:17:22]

Won't be able to de-reference the weak ptr here since its bound to the
compositor thread. For this place, I think if we checked if the local_frame_id_
is null, that should be good enough to early out.

For the other places it is being used only for DCHECKs for calls that should be
made only when a frame sink is bound. I think adding a bool to do this should be
fine and may be more explicit. Wdyt?

     return;
 
   if (!use_threaded_layer_tree_host_) {
@@ -255,7 +257,7 @@ void BlimpCompositor::ApplyViewportDeltas(
 }
 
 void BlimpCompositor::RequestNewCompositorFrameSink() {
-  DCHECK(!surface_factory_);
+  DCHECK(!proxy_client_);
   DCHECK(!compositor_frame_sink_request_pending_);
 
   compositor_frame_sink_request_pending_ = true;
@@ -278,9 +280,10 @@ void BlimpCompositor::DidCommitAndDrawFrame() {
   for (auto it = pending_commit_trackers_.begin();
        it != pending_commit_trackers_.end();) {
     if (--it->first == 0) {
-      if (surface_factory_)
+      if (proxy_client_) {
         surface_factory_->RequestCopyOfSurface(local_frame_id_,
                                                std::move(it->second));
+      }
       it = pending_commit_trackers_.erase(it);
     } else {
       ++it;
@@ -371,8 +374,8 @@ const base::WeakPtr<cc::InputHandler>& BlimpCompositor::GetInputHandler() {
 void BlimpCompositor::OnContextProvidersCreated(
     const scoped_refptr<cc::ContextProvider>& compositor_context_provider,
     const scoped_refptr<cc::ContextProvider>& worker_context_provider) {
-  DCHECK(!surface_factory_) << "Any connection to the old CompositorFrameSink "
-                               "should have been destroyed";
+  DCHECK(!proxy_client_) << "Any connection to the old CompositorFrameSink "
+                            "should have been destroyed";
 
   // Make sure we still have a host and we're still expecting a
   // CompositorFrameSink. This can happen if the host dies while the request is
@@ -400,16 +403,14 @@ void BlimpCompositor::OnContextProvidersCreated(
 void BlimpCompositor::BindToProxyClient(
     base::WeakPtr<BlimpCompositorFrameSinkProxyClient> proxy_client) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!surface_factory_);
+  DCHECK(!proxy_client_);
 
   proxy_client_ = proxy_client;
-  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
-      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
 }
 
 void BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   gfx::Size surface_size = root_pass->output_rect.size();
@@ -451,7 +452,7 @@ void BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
 }
 
 void BlimpCompositor::SubmitCompositorFrameAck() {
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(&BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck,
@@ -465,16 +466,16 @@ void BlimpCompositor::MakeCopyRequestOnNextSwap(
 
 void BlimpCompositor::UnbindProxyClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
 
   DestroyDelegatedContent();
-  surface_factory_.reset();
+  surface_factory_->Reset();
   proxy_client_ = nullptr;
 }
 
 void BlimpCompositor::ReturnResources(
     const cc::ReturnedResourceArray& resources) {
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(
@@ -560,7 +561,6 @@ void BlimpCompositor::DestroyLayerTreeHost() {
   // Tear down the output surface connection with the old LayerTreeHost
   // instance.
   DestroyDelegatedContent();
-  surface_factory_.reset();

[piman, 2016/11/07 22:31:38]

I believe there's no need to do an explicit Reset here, because we reset the
host below, which synchronously unbinds and destroys the CompositorFrameSink
(which is the ProxyClient, and so, will call UnbindProxyClient which will do the
Reset). Either way, this is only called from the destructor, so it's probably
not necessary anyway.

[Khushal, 2016/11/08 00:17:22]

Makes sense. Thanks for the clean up!

 
   // Destroy the old LayerTreeHost state.
   host_.reset();