cc: Move ownership of surfaces to SurfaceManager

cc/surfaces/surface_manager.cc

Index: cc/surfaces/surface_manager.cc
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index bd783705d0b6aed8301efae03420e61ab1fe913c..ffa3cd456b19edf5cdc933b71e2fc36fd2ea4414 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -45,14 +45,8 @@ SurfaceManager::~SurfaceManager() {
   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();
 }
 
 #if DCHECK_IS_ON()
@@ -77,7 +71,7 @@ void SurfaceManager::RequestSurfaceResolution(Surface* pending_surface) {
     dependency_tracker_->RequestSurfaceResolution(pending_surface);
 }
 
-std::unique_ptr<Surface> SurfaceManager::CreateSurface(
+base::WeakPtr<Surface> SurfaceManager::CreateSurface(
     base::WeakPtr<CompositorFrameSinkSupport> compositor_frame_sink_support,
     const SurfaceInfo& surface_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -88,42 +82,32 @@ std::unique_ptr<Surface> SurfaceManager::CreateSurface(
 
   // 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()]->AsWeakPtr();
   }
 
-  // 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);
+  // 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(surface->destroyed());

[Fady Samuel, 2017/06/16 18:56:38]

Can we get rid of the destroy bit?

[Saman Sami, 2017/06/19 22:16:46]

Done.

+  surfaces_to_destroy_.erase(surface);
   surface->set_destroyed(false);
   DCHECK_EQ(compositor_frame_sink_support.get(),
             surface->compositor_frame_sink_support().get());
-  return surface;
+  return surface->AsWeakPtr();
 }
 
-void SurfaceManager::DestroySurface(std::unique_ptr<Surface> surface) {
+void SurfaceManager::DestroySurface(Surface* surface) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!surface->destroyed());
   surface->set_destroyed(true);
   for (auto& observer : observer_list_)
     observer.OnSurfaceDestroyed(surface->surface_id());
-  surfaces_to_destroy_.push_back(std::move(surface));
+  surfaces_to_destroy_.insert(surface);
   GarbageCollectSurfaces();
 }
 
@@ -232,15 +216,14 @@ void SurfaceManager::GarbageCollectSurfaces() {
                                         ? 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));
+      surfaces_to_delete.push_back(surface_id);
       iter = surfaces_to_destroy_.erase(iter);
     } else {
       ++iter;
@@ -248,7 +231,8 @@ void SurfaceManager::GarbageCollectSurfaces() {
   }
 
   // ~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() {
@@ -292,7 +276,7 @@ SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForSequences() {
   // 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());
 
@@ -305,7 +289,7 @@ SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForSequences() {
   // 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());
@@ -462,7 +446,7 @@ Surface* SurfaceManager::GetSurfaceForId(const SurfaceId& surface_id) {
   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,
@@ -520,7 +504,7 @@ void SurfaceManager::SurfaceDamageExpected(const SurfaceId& surface_id,
     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());