Convert mustash use surface references.

services/ui/surfaces/display_compositor.cc

Index: services/ui/surfaces/display_compositor.cc
diff --git a/services/ui/surfaces/display_compositor.cc b/services/ui/surfaces/display_compositor.cc
index f8126c0e681c3edbb2958125bf80a217a0bff3bc..f796ab6099d3cdacc8b08b71f467798afe1200ba 100644
--- a/services/ui/surfaces/display_compositor.cc
+++ b/services/ui/surfaces/display_compositor.cc
@@ -14,32 +14,84 @@ DisplayCompositor::DisplayCompositor(
   manager_.AddObserver(this);
 }
 
-void DisplayCompositor::AddSurfaceReference(
-    const cc::SurfaceId& surface_id,
-    const cc::SurfaceSequence& surface_sequence) {
-  cc::Surface* surface = manager_.GetSurfaceForId(surface_id);
-  if (!surface) {
-    LOG(ERROR) << "Attempting to add dependency to nonexistent surface "
-               << surface_id.ToString();
+void DisplayCompositor::AddRootSurfaceReference(const cc::SurfaceId& child_id) {
+  AddSurfaceReference(manager_.GetRootSurfaceId(), child_id);
+}
+
+void DisplayCompositor::AddSurfaceReference(const cc::SurfaceId& parent_id,
+                                            const cc::SurfaceId& child_id) {
+  // If there are no temporary references then we can just add and return.
+  if (temp_references_.count(child_id.frame_sink_id()) == 0) {

[Fady Samuel, 2016/11/18 03:46:06]

nit: store temp_references_.find(child_id.frame_sink_id()) in a temporary
variable instead so that we don't need to look up the table twice?

[kylechar, 2016/11/18 17:24:10]

Done.

+    manager_.AddSurfaceReference(parent_id, child_id);
+    return;
+  }
+
+  // Get the vector<LocalFrameId> for the appropriate FrameSinkId and look for
+  // child_id.local_frame_id() in that vector. If found, there is a temporary
+  // reference to |child_id|.
+  auto& refs = temp_references_[child_id.frame_sink_id()];
+  auto ref_iter =
+      std::find(refs.begin(), refs.end(), child_id.local_frame_id());

[Fady Samuel, 2016/11/18 03:46:06]

This looks expensive but in practice I think the size of this vector will
usually be one unless the client is faster than the window server. Maybe a
comment indicating this?

[kylechar, 2016/11/18 17:24:10]

I can add one if you really want, but I'm not sure that level of detail is
necessary here.

+
+  if (ref_iter == refs.end()) {
+    manager_.AddSurfaceReference(parent_id, child_id);
     return;
   }
-  surface->AddDestructionDependency(surface_sequence);
+
+  // If the real reference is from the top level root then we don't want to add
+  // new reference then remove temporary reference, since they're the same.
+  if (parent_id != manager_.GetRootSurfaceId()) {
+    manager_.AddSurfaceReference(parent_id, child_id);
+    manager_.RemoveSurfaceReference(manager_.GetRootSurfaceId(), child_id);
+  }
+
+  // Remove temporary references for earlier frames.

[Fady Samuel, 2016/11/18 03:46:06]

Add a comment about the reasoning for why this is safe to do.

[kylechar, 2016/11/18 17:24:11]

Done.

+  for (auto iter = refs.begin(); iter != ref_iter; ++iter) {
+    cc::SurfaceId id = cc::SurfaceId(child_id.frame_sink_id(), *iter);
+    manager_.RemoveSurfaceReference(manager_.GetRootSurfaceId(), id);
+  }
+
+  refs.erase(refs.begin(), ++ref_iter);

[Fady Samuel, 2016/11/18 03:46:06]

This is removing the temp ref up to the |child_id| in the vector.

[kylechar, 2016/11/18 17:24:11]

Done.

+  if (refs.empty())
+    temp_references_.erase(child_id.frame_sink_id());
 }
 
-void DisplayCompositor::ReturnSurfaceReferences(
-    const cc::FrameSinkId& frame_sink_id,
-    const std::vector<uint32_t>& sequences) {
-  std::vector<uint32_t> sequences_copy(sequences);
-  manager_.DidSatisfySequences(frame_sink_id, &sequences_copy);
+void DisplayCompositor::RemoveRootSurfaceReference(
+    const cc::SurfaceId& child_id) {
+  RemoveSurfaceReference(manager_.GetRootSurfaceId(), child_id);
+}
+
+void DisplayCompositor::RemoveSurfaceReference(const cc::SurfaceId& parent_id,
+                                               const cc::SurfaceId& child_id) {
+  manager_.RemoveSurfaceReference(parent_id, child_id);
 }
 
 DisplayCompositor::~DisplayCompositor() {
+  // Remove all temporary references on shutdown.
+  for (auto& map_entry : temp_references_) {
+    const cc::FrameSinkId& frame_sink_id = map_entry.first;
+    for (auto& local_frame_id : map_entry.second) {
+      manager_.RemoveSurfaceReference(

[Fady Samuel, 2016/11/18 03:46:06]

So this can cause multiple GCes right? I guess that's not a problem right now
but could be a problem in the future when we return multiple surface references.
Maybe a TODO somewhere?

[kylechar, 2016/11/18 17:24:10]

Done.

+          manager_.GetRootSurfaceId(),
+          cc::SurfaceId(frame_sink_id, local_frame_id));
+    }
+  }
   manager_.RemoveObserver(this);
 }
 
 void DisplayCompositor::OnSurfaceCreated(const cc::SurfaceId& surface_id,
                                          const gfx::Size& frame_size,
                                          float device_scale_factor) {
+  // We can get into a situation where multiple CompositorFrames arrive for a
+  // CompositorFrameSink before the DisplayCompositorClient can add any
+  // references for the frame. When the second frame with a new size arrives,
+  // the first will be destroyed and then if there are no references it will be
+  // deleted during surface GC. A temporary reference, removed when a real
+  // reference is received, is added to prevent this from happening.
+  manager_.AddSurfaceReference(manager_.GetRootSurfaceId(), surface_id);
+  temp_references_[surface_id.frame_sink_id()].push_back(
+      surface_id.local_frame_id());
+
   if (client_)
     client_->OnSurfaceCreated(surface_id, frame_size, device_scale_factor);
 }