cc: Move ownership of surfaces to SurfaceManager

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 89c41bbbf8b352f4fbe6b3156f12b825dce2053b..0e7b31653e3ed66833f95b9d9311975cd2eb6645 100644
--- a/cc/surfaces/compositor_frame_sink_support.cc
+++ b/cc/surfaces/compositor_frame_sink_support.cc
@@ -77,9 +77,11 @@ void CompositorFrameSinkSupport::SetBeginFrameSource(
 }
 
 void CompositorFrameSinkSupport::EvictCurrentSurface() {
-  if (!current_surface_)
+  if (!current_surface_id_.is_valid())
     return;
-  DestroyCurrentSurface();
+  SurfaceId current_surface_id = current_surface_id_;

[danakj, 2017/06/27 18:21:31]

give this a name that differs from the member var by more than a _.
to_destroy_surface_id is one option.

[Saman Sami, 2017/06/28 14:19:43]

Done.

+  current_surface_id_ = SurfaceId();
+  surface_manager_->DestroySurface(current_surface_id);
 }
 
 void CompositorFrameSinkSupport::SetNeedsBeginFrame(bool needs_begin_frame) {
@@ -96,8 +98,9 @@ void CompositorFrameSinkSupport::DidNotProduceFrame(const BeginFrameAck& ack) {
   // |has_damage| is not transmitted, but false by default.
   DCHECK(!ack.has_damage);
 
-  if (current_surface_)
-    surface_manager_->SurfaceModified(current_surface_->surface_id(), ack);
+  if (current_surface_id_.is_valid())
+    surface_manager_->SurfaceModified(current_surface_id_, ack);
+
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
 }
@@ -127,13 +130,11 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
     }
   }
 
-  std::unique_ptr<Surface> surface;
-  bool create_new_surface =
-      (!current_surface_ ||
-       local_surface_id != current_surface_->surface_id().local_surface_id());
-  if (!create_new_surface) {
-    surface = std::move(current_surface_);
-  } else {
+  Surface* current_surface =

[danakj, 2017/06/27 18:21:31]

give this a name that differs from the member variable by more than a _.
replacement_surface is one such option.

[Saman Sami, 2017/06/28 14:19:43]

But there is no current_surface_? There is only a current_surface_id_ and I
think current_surface is a reasonable name for a surface whose id is
current_surface_id_.

[danakj, 2017/06/28 15:59:45]

Ah that's true, I was reading the old code too.

+      surface_manager_->GetSurfaceForId(current_surface_id_);

[danakj, 2017/06/27 18:21:31]

can you use if/else to only call this when we would use it? Or maybe do this and
store that as previous_surface, then in the if/else you can assign
replacement_surface = previous_surface when we're not going to change it?

[Saman Sami, 2017/06/28 14:19:43]

I don't think I fully understand your suggestion. This variable will be used
when we queue a frame, unless you are worried about the invalid SurfaceInfo
case? That's an edge case and doesn't really happen. I'm also not sure how
assigning this to previous_surface instead of current_surface helps? Can you
elaborate? Thanks.

[danakj, 2017/06/28 15:59:45]

T* t = MakeSomeTea();
if (foo) {
  t = MakeSomeOtherTea();
}

vs

T* t;
if (!foo) {
  t = MakeSomeTea();
} else {
  t = MakeSomeOtherTea();
}

Because the re-assignment of current_surface is pretty far from here, it's
behaviour is harder to follow in the first way I think. Or here it'd be

Surface* prev_surface = GetSurfaceForId(current_surface_id_);
Surface* next_surface;
if (want to use prev surface still) {
  next_surface = prev_surface;
} else {
  .. build up a new surface ..
  next_surface = CreateSurface
}

This expresses the intent of what's going on and is a small change that improves
how it reads I think.

[Saman Sami, 2017/06/28 18:01:00]

Done.

+  Surface* prev_surface = current_surface;
+  if (!current_surface_id_.is_valid() ||
+      local_surface_id != current_surface_id_.local_surface_id()) {
     SurfaceId surface_id(frame_sink_id_, local_surface_id);
     gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size();
     float device_scale_factor = frame.metadata.device_scale_factor;
@@ -142,8 +143,8 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
     if (!surface_info.is_valid()) {
       TRACE_EVENT_INSTANT0("cc", "Invalid SurfaceInfo",
                            TRACE_EVENT_SCOPE_THREAD);
-      if (current_surface_)
-        DestroyCurrentSurface();
+      if (current_surface)
+        EvictCurrentSurface();
       ReturnedResourceArray resources;
       TransferableResource::ReturnResources(frame.resource_list, &resources);
       ReturnResources(resources);
@@ -151,12 +152,13 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
       return true;
     }
 
-    surface = CreateSurface(surface_info);
-    surface_manager_->SurfaceDamageExpected(surface->surface_id(),
+    current_surface = CreateSurface(surface_info);
+    current_surface_id_ = SurfaceId(frame_sink_id_, local_surface_id);
+    surface_manager_->SurfaceDamageExpected(current_surface->surface_id(),
                                             last_begin_frame_args_);
   }
 
-  bool result = surface->QueueFrame(
+  bool result = current_surface->QueueFrame(
       std::move(frame),
       base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
                  weak_factory_.GetWeakPtr()),
@@ -164,15 +166,14 @@ bool CompositorFrameSinkSupport::SubmitCompositorFrame(
                           weak_factory_.GetWeakPtr()));
 
   if (!result) {
-    surface_manager_->DestroySurface(std::move(surface));
+    EvictCurrentSurface();
     return false;
   }
 
-  if (current_surface_) {
-    surface->SetPreviousFrameSurface(current_surface_.get());
-    DestroyCurrentSurface();
+  if (prev_surface && prev_surface != current_surface) {
+    current_surface->SetPreviousFrameSurface(prev_surface);
+    surface_manager_->DestroySurface(prev_surface->surface_id());
   }
-  current_surface_ = std::move(surface);
 
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
@@ -288,9 +289,8 @@ void CompositorFrameSinkSupport::Init(SurfaceManager* surface_manager) {
 
 void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
   UpdateNeedsBeginFramesInternal();
-  if (current_surface_) {
-    surface_manager_->SurfaceDamageExpected(current_surface_->surface_id(),
-                                            args);
+  if (current_surface_id_.is_valid()) {
+    surface_manager_->SurfaceDamageExpected(current_surface_id_, args);
   }
   last_begin_frame_args_ = args;
   if (client_)
@@ -342,28 +342,29 @@ void CompositorFrameSinkSupport::UpdateNeedsBeginFramesInternal() {
     begin_frame_source_->RemoveObserver(this);
 }
 
-std::unique_ptr<Surface> CompositorFrameSinkSupport::CreateSurface(
+Surface* CompositorFrameSinkSupport::CreateSurface(

[Fady Samuel, 2017/06/26 20:46:43]

Would it be simpler if this was void?

[Saman Sami, 2017/06/26 20:56:24]

Then we would have to call GetSurfaceForId after creating the surface. This is
slightly more efficient and cleaner.

     const SurfaceInfo& surface_info) {
   seen_first_frame_activation_ = false;
   return surface_manager_->CreateSurface(weak_factory_.GetWeakPtr(),

[Fady Samuel, 2017/06/26 20:46:43]

Would it be simpler if this was void?

                                          surface_info);
 }
 
-void CompositorFrameSinkSupport::DestroyCurrentSurface() {
-  surface_manager_->DestroySurface(std::move(current_surface_));
-}
-
 void CompositorFrameSinkSupport::RequestCopyOfSurface(
     std::unique_ptr<CopyOutputRequest> copy_request) {
-  if (!current_surface_)
+  if (!current_surface_id_.is_valid())
     return;
-
-  DCHECK(current_surface_->compositor_frame_sink_support().get() == this);
-  current_surface_->RequestCopyOfOutput(std::move(copy_request));
+  Surface* current_surface =
+      surface_manager_->GetSurfaceForId(current_surface_id_);
+  DCHECK(current_surface->compositor_frame_sink_support().get() == this);

[danakj, 2017/06/27 18:21:31]

DCHECK_EQ

[Saman Sami, 2017/06/28 14:19:43]

Done.

+  current_surface->RequestCopyOfOutput(std::move(copy_request));
   BeginFrameAck ack;
   ack.has_damage = true;
-  if (current_surface_->HasActiveFrame())
-    surface_manager_->SurfaceModified(current_surface_->surface_id(), ack);
+  if (current_surface->HasActiveFrame())
+    surface_manager_->SurfaceModified(current_surface->surface_id(), ack);
+}
+
+Surface* CompositorFrameSinkSupport::GetCurrentSurfaceForTesting() {
+  return surface_manager_->GetSurfaceForId(current_surface_id_);
 }
 
 }  // namespace cc