VizDevTools: Use unique_ptrs for the FrameSinkElements

FrameSinkElement creation is moved into a function that creates the
unique_ptr and adds it to the map of elements. Removing elements from
this map now deletes them.

The FrameSinkElements need to be owned by DOMAgentViz because it
receives all of the FrameSinkObserver events. Whereas for Views, each
ViewElement, WidgetElement, and WindowElement is its own Observer, so
centralized ownership in DOMAgentAura doesn't make as much sense.

Elements are no longer inserted into |frame_sink_elements_| by
BuildTreeForFrameSink(), so we don't need to static_cast pointers any
more. Also make variable names in BuildTreeForFrameSink() clearer.

Bug: 816802
Change-Id: Ieec4db482ffb3ff8a740eb559c1c3d9ee9a80705
Reviewed-on: https://chromium-review.googlesource.com/c/1346910
Commit-Queue: Sean Gilhuly <sgilhuly@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613202}

components/ui_devtools/ui_element.cc

@@ -17,8 +17,10 @@ static int node_ids = 0;
 }  // namespace
 
 UIElement::~UIElement() {
-  for (auto* child : children_)
-    delete child;
+  if (owns_children_) {
+    for (auto* child : children_)
+      delete child;
+  }
   children_.clear();
 }
 

components/ui_devtools/ui_element.h

@@ -39,6 +39,7 @@ class UI_DEVTOOLS_EXPORT UIElement {
   const std::vector<UIElement*>& children() const { return children_; }
   bool is_updating() const { return is_updating_; }
   void set_is_updating(bool is_updating) { is_updating_ = is_updating; }
+  void set_owns_children(bool owns_children) { owns_children_ = owns_children; }
 
   // |child| is inserted in front of |before|. If |before| is null, it
   // is inserted at the end. Parent takes ownership of the added child.
@@ -92,6 +93,7 @@ class UI_DEVTOOLS_EXPORT UIElement {
   UIElement* parent_;
   UIElementDelegate* delegate_;
   bool is_updating_ = false;
+  bool owns_children_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(UIElement);
 };

components/ui_devtools/viz_views/dom_agent_viz.cc

@@ -60,14 +60,11 @@ DOMAgentViz::~DOMAgentViz() {
 
 void DOMAgentViz::OnRegisteredFrameSinkId(
     const viz::FrameSinkId& frame_sink_id) {
-  if (!base::ContainsKey(frame_sink_elements_, frame_sink_id)) {
-    // If a FrameSink was just registered we don't know anything about
-    // hierarchy. So we should attach it to the RootElement.
-    element_root()->AddChild(
-        new FrameSinkElement(frame_sink_id, frame_sink_manager_, this,
-                             element_root(), /*is_root=*/false,
+  // If a FrameSink was just registered we don't know anything about
+  // hierarchy. So we should attach it to the RootElement.
+  element_root()->AddChild(
+      CreateFrameSinkElement(frame_sink_id, element_root(), /*is_root=*/false,
                              /*has_created_frame_sink=*/false));
-  }
 }
 
 void DOMAgentViz::OnInvalidatedFrameSinkId(
@@ -78,7 +75,7 @@ void DOMAgentViz::OnInvalidatedFrameSinkId(
   // A FrameSinkElement with |frame_sink_id| can only be invalidated after
   // being destroyed.
   DCHECK(!it->second->has_created_frame_sink());
-  DestroyElementAndRemoveSubtree(it->second);
+  DestroyElementAndRemoveSubtree(it->second.get());
 }
 
 void DOMAgentViz::OnCreatedCompositorFrameSink(
@@ -112,8 +109,8 @@ void DOMAgentViz::OnRegisteredFrameSinkHierarchy(
   DCHECK(it_parent != frame_sink_elements_.end());
   DCHECK(it_child != frame_sink_elements_.end());
 
-  FrameSinkElement* child = it_child->second;
-  FrameSinkElement* new_parent = it_parent->second;
+  FrameSinkElement* child = it_child->second.get();
+  FrameSinkElement* new_parent = it_parent->second.get();
 
   // TODO: Add support for |child| to have multiple parents.
   Reparent(new_parent, child);
@@ -128,7 +125,7 @@ void DOMAgentViz::OnUnregisteredFrameSinkHierarchy(
   auto it_child = frame_sink_elements_.find(child_frame_sink_id);
   DCHECK(it_child != frame_sink_elements_.end());
 
-  FrameSinkElement* child = it_child->second;
+  FrameSinkElement* child = it_child->second.get();
 
   // TODO: Add support for |child| to have multiple parents: only adds |child|
   // to RootElement if all parents of |child| are unregistered.
@@ -136,28 +133,27 @@ void DOMAgentViz::OnUnregisteredFrameSinkHierarchy(
 }
 
 std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForFrameSink(
-    FrameSinkElement* frame_sink_element,
-    const viz::FrameSinkId& frame_sink_id) {
-  frame_sink_elements_.emplace(frame_sink_id, frame_sink_element);
+    UIElement* parent_element,
+    const viz::FrameSinkId& parent_id) {
   std::unique_ptr<Array<DOM::Node>> children = Array<DOM::Node>::create();
 
   // Once the FrameSinkElement is created it calls this function to build its
-  // subtree. So we iterate through |frame_sink_element|'s children and
+  // subtree. So we iterate through |parent_element|'s children and
   // recursively build the subtree for them.
-  for (auto& child : frame_sink_manager_->GetChildrenByParent(frame_sink_id)) {
+  for (auto& child_id : frame_sink_manager_->GetChildrenByParent(parent_id)) {
     bool has_created_frame_sink =
-        !!frame_sink_manager_->GetFrameSinkForId(child);
+        !!frame_sink_manager_->GetFrameSinkForId(child_id);
 
-    FrameSinkElement* f_s_element = new FrameSinkElement(
-        child, frame_sink_manager_, this, frame_sink_element,
-        /*is_root=*/false, has_created_frame_sink);
+    FrameSinkElement* child_element = CreateFrameSinkElement(
+        child_id, parent_element, /*is_root=*/false, has_created_frame_sink);
 
-    children->addItem(BuildTreeForFrameSink(f_s_element, child));
-    frame_sink_element->AddChild(f_s_element);
+    children->addItem(BuildTreeForFrameSink(child_element, child_id));
+    parent_element->AddChild(child_element);
   }
+
   std::unique_ptr<DOM::Node> node =
-      BuildNode("FrameSink", frame_sink_element->GetAttributes(),
-                std::move(children), frame_sink_element->node_id());
+      BuildNode("FrameSink", parent_element->GetAttributes(),
+                std::move(children), parent_element->node_id());
   return node;
 }
 
@@ -169,13 +165,16 @@ protocol::Response DOMAgentViz::enable() {
 protocol::Response DOMAgentViz::disable() {
   frame_sink_manager_->RemoveObserver(this);
   Clear();
-  element_root()->ClearChildren();
   return DOMAgent::disable();
 }
 
 std::vector<UIElement*> DOMAgentViz::CreateChildrenForRoot() {
   std::vector<UIElement*> children;
 
+  // All of the FrameSinkElements and SurfaceElements are owned here, so make
+  // sure the root element doesn't delete our pointers.
+  element_root()->set_owns_children(false);
+
   // Find all elements that are not part of any hierarchy. This will be
   // FrameSinks that are either root, or detached.
   std::vector<viz::FrameSinkId> registered_frame_sinks =
@@ -195,11 +194,8 @@ std::vector<UIElement*> DOMAgentViz::CreateChildrenForRoot() {
         frame_sink_manager_->GetFrameSinkForId(frame_sink_id);
     bool is_root = support && support->is_root();
     bool has_created_frame_sink = !!support;
-    // TODO(sgilhuly): Use unique_ptr instead of new for the FrameSinkElements.
-    UIElement* frame_sink_element =
-        new FrameSinkElement(frame_sink_id, frame_sink_manager_, this,
-                             element_root(), is_root, has_created_frame_sink);
-    children.push_back(frame_sink_element);
+    children.push_back(CreateFrameSinkElement(frame_sink_id, element_root(),
+                                              is_root, has_created_frame_sink));
   }
 
   return children;
@@ -208,19 +204,13 @@ std::vector<UIElement*> DOMAgentViz::CreateChildrenForRoot() {
 std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForUIElement(
     UIElement* ui_element) {
   if (ui_element->type() == UIElementType::FRAMESINK) {
-    FrameSinkElement* frame_sink_element =
-        static_cast<FrameSinkElement*>(ui_element);
-    return BuildTreeForFrameSink(frame_sink_element,
-                                 frame_sink_element->frame_sink_id());
+    return BuildTreeForFrameSink(ui_element,
+                                 FrameSinkElement::From(ui_element));
   }
   return nullptr;
 }
 
 void DOMAgentViz::Clear() {
-  for (auto& entry : frame_sink_elements_) {
-    entry.second->ClearChildren();
-    delete entry.second;
-  }
   frame_sink_elements_.clear();
 }
 
@@ -238,7 +228,6 @@ void DOMAgentViz::DestroyElementAndRemoveSubtree(UIElement* element) {
 
 void DOMAgentViz::Reparent(UIElement* new_parent, UIElement* child) {
   DestroySubtree(child);
-  child->ClearChildren();
 
   // This removes the child element from the Node map. It has to be added with
   // null parent to recreate the entry.
@@ -250,10 +239,7 @@ void DOMAgentViz::Reparent(UIElement* new_parent, UIElement* child) {
 
 void DOMAgentViz::DestroyElement(UIElement* element) {
   if (element->type() == UIElementType::FRAMESINK) {
-    // TODO(sgilhuly): Use unique_ptr, so that the element doesn't need to be
-    // deleted manually.
     frame_sink_elements_.erase(FrameSinkElement::From(element));
-    delete element;
   } else {
     NOTREACHED();
   }
@@ -267,4 +253,16 @@ void DOMAgentViz::DestroySubtree(UIElement* element) {
   element->ClearChildren();
 }
 
+FrameSinkElement* DOMAgentViz::CreateFrameSinkElement(
+    const viz::FrameSinkId& frame_sink_id,
+    UIElement* parent,
+    bool is_root,
+    bool is_client_connected) {
+  DCHECK(!base::ContainsKey(frame_sink_elements_, frame_sink_id));
+  frame_sink_elements_[frame_sink_id] = std::make_unique<FrameSinkElement>(
+      frame_sink_id, frame_sink_manager_, this, parent, is_root,
+      is_client_connected);
+  return frame_sink_elements_[frame_sink_id].get();
+}
+
 }  // namespace ui_devtools

components/ui_devtools/viz_views/dom_agent_viz.h

@@ -40,8 +40,8 @@ class DOMAgentViz : public viz::FrameSinkObserver, public DOMAgent {
 
  private:
   std::unique_ptr<protocol::DOM::Node> BuildTreeForFrameSink(
-      FrameSinkElement* frame_sink_element,
-      const viz::FrameSinkId& frame_sink_id);
+      UIElement* parent_element,
+      const viz::FrameSinkId& parent_id);
 
   // DOM::Backend:
   protocol::Response enable() override;
@@ -77,11 +77,20 @@ class DOMAgentViz : public viz::FrameSinkObserver, public DOMAgent {
   // is preserved.
   void DestroySubtree(UIElement* element);
 
+  // Constructs a new FrameSinkElement with some default arguments, adds it to
+  // |frame_sink_elements_|, and returns its pointer.
+  FrameSinkElement* CreateFrameSinkElement(
+      const viz::FrameSinkId& frame_sink_id,
+      UIElement* parent,
+      bool is_root,
+      bool is_client_connected);
+
   // This is used to track created FrameSinkElements in a FrameSink tree. Every
   // time we register/invalidate a FrameSinkId, create/destroy a FrameSink,
   // register/unregister hierarchy we change this set, because these actions
   // involve deleting and adding elements.
-  base::flat_map<viz::FrameSinkId, FrameSinkElement*> frame_sink_elements_;
+  base::flat_map<viz::FrameSinkId, std::unique_ptr<FrameSinkElement>>
+      frame_sink_elements_;
 
   viz::FrameSinkManagerImpl* frame_sink_manager_;
 

components/ui_devtools/viz_views/frame_sink_element.cc

@@ -25,7 +25,11 @@ FrameSinkElement::FrameSinkElement(
       frame_sink_id_(frame_sink_id),
       frame_sink_manager_(frame_sink_manager),
       is_root_(is_root),
-      has_created_frame_sink_(has_created_frame_sink) {}
+      has_created_frame_sink_(has_created_frame_sink) {
+  // DOMAgentViz handles all of the FrameSink events, so it owns all of the
+  // FrameSinkElements.
+  set_owns_children(false);
+}
 
 FrameSinkElement::~FrameSinkElement() {}
 

components/ui_devtools/viz_views/surface_element.cc

@@ -24,6 +24,9 @@ SurfaceElement::SurfaceElement(const viz::SurfaceId& surface_id,
       frame_sink_manager_->surface_manager()->GetSurfaceForId(surface_id_);
   DCHECK(surface);
   surface_bounds_ = gfx::Rect(surface->size_in_pixels());
+  // DOMAgentViz handles all of the Surface events, so it owns all of the
+  // SurfaceElements.
+  set_owns_children(false);
 }
 
 SurfaceElement::~SurfaceElement() = default;