cc: Move ownership of surfaces to SurfaceManager

cc/surfaces/surface_manager.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_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 27 matching lines @@
       new DirectSurfaceReferenceFactory(weak_factory_.GetWeakPtr());
 }
 
 SurfaceManager::~SurfaceManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Remove all temporary references on shutdown.
   temporary_references_.clear();
   temporary_reference_ranges_.clear();
 
-  GarbageCollectSurfaces();
-
-  for (SurfaceDestroyList::iterator it = surfaces_to_destroy_.begin();
-       it != surfaces_to_destroy_.end();
-       ++it) {
-    UnregisterSurface((*it)->surface_id());
-  }
   surfaces_to_destroy_.clear();
+  surface_map_.clear();

[danakj, 2017/06/21 21:24:09]

Why do we clear these in the destructor here? Their own destructors do this.
Please write comments explaining why we need to do these clear()s explicitly
here.

[Saman Sami, 2017/06/26 20:44:05]

We actually don't need them so I'll remove them.

 }
 
 #if DCHECK_IS_ON()
 std::string SurfaceManager::SurfaceReferencesToString() {
   std::stringstream str;
   SurfaceReferencesToStringImpl(root_surface_id_, "", &str);
   // Temporary references will have an asterisk in front of them.
   for (auto& map_entry : temporary_references_)
     SurfaceReferencesToStringImpl(map_entry.first, "* ", &str);
 
   return str.str();
 }
 #endif
 
 void SurfaceManager::SetDependencyTracker(
     SurfaceDependencyTracker* dependency_tracker) {
   dependency_tracker_ = dependency_tracker;
 }
 
 void SurfaceManager::RequestSurfaceResolution(Surface* pending_surface) {
   if (dependency_tracker_)
     dependency_tracker_->RequestSurfaceResolution(pending_surface);
 }
 
-std::unique_ptr<Surface> SurfaceManager::CreateSurface(
+Surface* SurfaceManager::CreateSurface(
     base::WeakPtr<CompositorFrameSinkSupport> compositor_frame_sink_support,
     const SurfaceInfo& surface_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(surface_info.is_valid());
   DCHECK(compositor_frame_sink_support);
   DCHECK_EQ(surface_info.id().frame_sink_id(),
             compositor_frame_sink_support->frame_sink_id());
 
   // If no surface with this SurfaceId exists, simply create the surface and
   // return.
-  auto surface_iter = surface_map_.find(surface_info.id());
-  if (surface_iter == surface_map_.end()) {
-    auto surface =
+  auto it = surface_map_.find(surface_info.id());
+  if (it == surface_map_.end()) {
+    surface_map_[surface_info.id()] =
         base::MakeUnique<Surface>(surface_info, compositor_frame_sink_support);
-    surface_map_[surface->surface_id()] = surface.get();
-    return surface;
+    return surface_map_[surface_info.id()].get();
   }
 
-  // If a surface with this SurfaceId exists and it's not marked as destroyed,
-  // we should not receive a request to create a new surface with the same
-  // SurfaceId.
-  DCHECK(surface_iter->second->destroyed());
-
-  // If a surface with this SurfaceId exists and it's marked as destroyed,
-  // it means it's in the garbage collector's queue. We simply take it out of
-  // the queue and reuse it.
-  auto it =
-      std::find_if(surfaces_to_destroy_.begin(), surfaces_to_destroy_.end(),
-                   [&surface_info](const std::unique_ptr<Surface>& surface) {
-                     return surface->surface_id() == surface_info.id();
-                   });
-  DCHECK(it != surfaces_to_destroy_.end());
-  std::unique_ptr<Surface> surface = std::move(*it);
-  surfaces_to_destroy_.erase(it);
-  surface->set_destroyed(false);
+  // If a surface with this SurfaceId exists, it must be marked as destroyed.
+  // Otherwise, we wouldn't receive a request to reuse the same SurfaceId.
+  // Remove the surface out of the garbage collector's queue and reuse it.
+  Surface* surface = it->second.get();
+  DCHECK(IsMarkedForDestruction(surface_info.id()));
+  surfaces_to_destroy_.erase(surface_info.id());
   DCHECK_EQ(compositor_frame_sink_support.get(),
             surface->compositor_frame_sink_support().get());
   return surface;
 }
 
-void SurfaceManager::DestroySurface(std::unique_ptr<Surface> surface) {
+void SurfaceManager::DestroySurface(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  surface->set_destroyed(true);
+  DCHECK(surface_map_.count(surface_id));
+  if (IsMarkedForDestruction(surface_id))
+    return;
   for (auto& observer : observer_list_)
-    observer.OnSurfaceDestroyed(surface->surface_id());
-  surfaces_to_destroy_.push_back(std::move(surface));
+    observer.OnSurfaceDestroyed(surface_id);
+  surfaces_to_destroy_.insert(surface_id);
   GarbageCollectSurfaces();
 }
 
 void SurfaceManager::SurfaceWillDraw(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (auto& observer : observer_list_)
     observer.OnSurfaceWillDraw(surface_id);
 }
 
 void SurfaceManager::RequireSequence(const SurfaceId& surface_id,
@@ skipping 94 matching lines @@
 }
 
 void SurfaceManager::GarbageCollectSurfaces() {
   if (surfaces_to_destroy_.empty())
     return;
 
   SurfaceIdSet reachable_surfaces = using_surface_references()
                                         ? GetLiveSurfacesForReferences()
                                         : GetLiveSurfacesForSequences();
 
-  std::vector<std::unique_ptr<Surface>> surfaces_to_delete;
+  std::vector<SurfaceId> surfaces_to_delete;
 
   // Delete all destroyed and unreachable surfaces.
   for (auto iter = surfaces_to_destroy_.begin();
        iter != surfaces_to_destroy_.end();) {
-    SurfaceId surface_id = (*iter)->surface_id();
-    if (reachable_surfaces.count(surface_id) == 0) {
-      UnregisterSurface(surface_id);
-      surfaces_to_delete.push_back(std::move(*iter));
+    if (reachable_surfaces.count(*iter) == 0) {
+      surfaces_to_delete.push_back(*iter);
       iter = surfaces_to_destroy_.erase(iter);
     } else {
       ++iter;
     }
   }
 
   // ~Surface() draw callback could modify |surfaces_to_destroy_|.
-  surfaces_to_delete.clear();
+  for (const SurfaceId& surface_id : surfaces_to_delete)
+    DestroySurfaceInternal(surface_id);
 }
 
 SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForReferences() {
   DCHECK(using_surface_references());
 
   SurfaceIdSet reachable_surfaces;
 
   // Walk down from the root and mark each SurfaceId we encounter as reachable.
   std::queue<SurfaceId> surface_queue;
   surface_queue.push(root_surface_id_);
@@ skipping 23 matching lines @@
   // Simple mark and sweep GC.
   // TODO(jbauman): Reduce the amount of work when nothing needs to be
   // destroyed.
   std::vector<SurfaceId> live_surfaces;
   std::unordered_set<SurfaceId, SurfaceIdHash> live_surfaces_set;
 
   // GC roots are surfaces that have not been destroyed, or have not had all
   // their destruction dependencies satisfied.
   for (auto& map_entry : surface_map_) {
     const SurfaceId& surface_id = map_entry.first;
-    Surface* surface = map_entry.second;
+    Surface* surface = map_entry.second.get();
     surface->SatisfyDestructionDependencies(&satisfied_sequences_,
                                   framesink_manager_.GetValidFrameSinkIds());
 
-    if (!surface->destroyed() || surface->GetDestructionDependencyCount() > 0) {
+    if (!IsMarkedForDestruction(surface_id) ||
+        surface->GetDestructionDependencyCount() > 0) {
       live_surfaces_set.insert(surface_id);
       live_surfaces.push_back(surface_id);
     }
   }
 
   // Mark all surfaces reachable from live surfaces by adding them to
   // live_surfaces and live_surfaces_set.
   for (size_t i = 0; i < live_surfaces.size(); i++) {
-    Surface* surf = surface_map_[live_surfaces[i]];
+    Surface* surf = surface_map_[live_surfaces[i]].get();
     DCHECK(surf);
 
     const auto& children = GetSurfacesReferencedByParent(surf->surface_id());
     for (const SurfaceId& id : children) {
       if (live_surfaces_set.count(id))
         continue;
 
       Surface* surf2 = GetSurfaceForId(id);
       if (surf2) {
         live_surfaces.push_back(id);
@@ skipping 136 matching lines @@
     const FrameSinkId& child_frame_sink_id) {
   framesink_manager_.UnregisterFrameSinkHierarchy(parent_frame_sink_id,
                                                 child_frame_sink_id);
 }
 
 Surface* SurfaceManager::GetSurfaceForId(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   if (it == surface_map_.end())
     return nullptr;
-  return it->second;
+  return it->second.get();
 }
 
 bool SurfaceManager::SurfaceModified(const SurfaceId& surface_id,
                                      const BeginFrameAck& ack) {
   CHECK(thread_checker_.CalledOnValidThread());
   bool changed = false;
   for (auto& observer : observer_list_)
     changed |= observer.OnSurfaceDamaged(surface_id, ack);
   return changed;
 }
@@ skipping 37 matching lines @@
     dependency_tracker_->OnSurfaceDiscarded(surface);
 }
 
 void SurfaceManager::SurfaceDamageExpected(const SurfaceId& surface_id,
                                            const BeginFrameArgs& args) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (auto& observer : observer_list_)
     observer.OnSurfaceDamageExpected(surface_id, args);
 }
 
-void SurfaceManager::UnregisterSurface(const SurfaceId& surface_id) {
+void SurfaceManager::DestroySurfaceInternal(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   DCHECK(it != surface_map_.end());
+  std::unique_ptr<Surface> doomed = std::move(it->second);
   surface_map_.erase(it);
   RemoveAllSurfaceReferences(surface_id);
 }
 
 #if DCHECK_IS_ON()
 void SurfaceManager::SurfaceReferencesToStringImpl(const SurfaceId& surface_id,
                                                    std::string indent,
                                                    std::stringstream* str) {
   *str << indent;
 
   // Print the current line for |surface_id|.
   Surface* surface = GetSurfaceForId(surface_id);
   if (surface) {
     *str << surface->surface_id().ToString();
-    *str << (surface->destroyed() ? " destroyed" : " live");
+    *str << (IsMarkedForDestruction(surface_id) ? " destroyed" : " live");
 
     if (surface->HasPendingFrame()) {
       // This provides the surface size from the root render pass.
       const CompositorFrame& frame = surface->GetPendingFrame();
       *str << " pending "
            << frame.render_pass_list.back()->output_rect.size().ToString();
     }
 
     if (surface->HasActiveFrame()) {
       // This provides the surface size from the root render pass.
       const CompositorFrame& frame = surface->GetActiveFrame();
       *str << " active "
            << frame.render_pass_list.back()->output_rect.size().ToString();
     }
   } else {
     *str << surface_id;
   }
   *str << "\n";
 
   // If the current surface has references to children, sort children and print
   // references for each child.
   for (const SurfaceId& child_id : GetSurfacesReferencedByParent(surface_id))
     SurfaceReferencesToStringImpl(child_id, indent + "  ", str);
 }
 #endif  // DCHECK_IS_ON()
 
+bool SurfaceManager::IsMarkedForDestruction(const SurfaceId& surface_id) {
+  return surfaces_to_destroy_.count(surface_id) != 0;
+}
+
 }  // namespace cc