VizDevTools: Providing updates of a FrameSink tree

FrameSinkElement implements FrameSinkObserver interface and handles all
the events for updating the tree.

Bug: 816802
Change-Id: I6552281672cda18f611c65971aa56eea7aa13a00
Reviewed-on: https://chromium-review.googlesource.com/1113018
Commit-Queue: Yi Xu <yiyix@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584290}

components/ui_devtools/ui_element.h

@@ -31,6 +31,7 @@ class UI_DEVTOOLS_EXPORT UIElement {
   int node_id() const { return node_id_; };
   std::string GetTypeName() const;
   UIElement* parent() const { return parent_; };
+  void set_parent(UIElement* parent) { parent_ = parent; };
   UIElementDelegate* delegate() const { return delegate_; };
   UIElementType type() const { return type_; };
   const std::vector<UIElement*>& children() const { return children_; };

components/ui_devtools/viz_views/dom_agent_viz.cc

@@ -12,6 +12,43 @@
 
 namespace ui_devtools {
 
+// Updating logic for FrameSinks:
+//
+// 1. Creating. We register a FrameSinkId, create a CompositorFrameSink and if a
+// CompositorFrameSink is not a root we register hierarchy from the parent of
+// this FrameSink to this CompositorFrameSink. When we register a FrameSinkId,
+// we check if its corresponding element is already in the tree. If not, we
+// attach it to the RootElement which serves as the root of the
+// CompositorFrameSink tree. In this state the CompositorFrameSink is considered
+// unembedded and it is a sibling of RootCompositorFrameSinks. If it is present
+// in a tree we just change the properties (|is_registered_| or
+// |is_client_connected_|). These events don't know anything about the hierarchy
+// so we don't change it. When we get OnRegisteredHierarchy from parent to child
+// the corresponding elements must already be present in a tree. The usual state
+// is: child is attached to RootElement and now we will detach it from the
+// RootElement and attach to the real parent. During handling this event we
+// actually delete the subtree of RootElement rooted from child and create a new
+// subtree of parent. This potentially could be more efficient, but if we just
+// switch necessary pointers we must send a notification to the frontend so it
+// can update the UI. Unfortunately, this action involves deleting a node from
+// the backend list of UI Elements (even when it is still alive) and trying to
+// delete it once again (for instance, when we close a corresponding tab) causes
+// crash.
+//
+// 2. Deleting. We unregister hierarchy, destroy a CompositorFrameSink and
+// invalidate a FrameSinkId. When we invalidate an FrameSinkId or destroy a
+// FrameSink we check if it's the last action that has to happen with the
+// corresponding element. For example, if the element has |is_client_connected_|
+// = true and |is_registered_| = true and we get a |OnDestroyedFrameSink| event
+// we just set |is_client_connected_| = false, but don't remove it from a tree,
+// because its FrameSinkId is still registered, so it's not completely dead. But
+// when after that we get |OnInvalidatedFrameSinkId| we set |is_registered_| =
+// false and since both these fields are false we can go ahead and remove the
+// node from the tree. When we get OnUnregisteredHierarchy we assume the nodes
+// are still present in a tree, so we do the same work as we did in registering
+// case. Only here we move a subtree of parent rooted from child to the
+// RootElement. Obviously, now the child will be in detached state.
+
 using namespace ui_devtools::protocol;
 
 DOMAgentViz::DOMAgentViz(viz::FrameSinkManagerImpl* frame_sink_manager)
@@ -21,8 +58,115 @@ DOMAgentViz::~DOMAgentViz() {
   Clear();
 }
 
+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,
+                             /*is_registered=*/true,
+                             /*is_client_connected=*/false),
+        element_root()->children().empty() ? nullptr
+                                           : element_root()->children().back());
+  }
+  registered_frame_sink_ids_to_is_connected_[frame_sink_id] = false;
+}
+
+void DOMAgentViz::OnInvalidatedFrameSinkId(
+    const viz::FrameSinkId& frame_sink_id) {
+  auto it = frame_sink_elements_.find(frame_sink_id);
+  DCHECK(it != frame_sink_elements_.end());
+
+  FrameSinkElement* element = it->second;
+  registered_frame_sink_ids_to_is_connected_.erase(frame_sink_id);
+  element->SetRegistered(false);
+
+  // If a FrameSink is destroyed and invalidated we should remove it from the
+  // tree.
+  RemoveFrameSinkSubtree(element);
+  DestroyChildSubtree(element->parent(), element);
+}
+
+void DOMAgentViz::OnCreatedCompositorFrameSink(
+    const viz::FrameSinkId& frame_sink_id,
+    bool is_root) {
+  auto frame_sink_element = frame_sink_elements_.find(frame_sink_id);
+  DCHECK(frame_sink_element != frame_sink_elements_.end());
+  // The corresponding element is already present in a tree, so we
+  // should update its |is_client_connected_| and |is_root_| properties.
+  frame_sink_element->second->SetClientConnected(true);
+  frame_sink_element->second->SetRoot(is_root);
+
+  registered_frame_sink_ids_to_is_connected_[frame_sink_id] = true;
+}
+
+void DOMAgentViz::OnDestroyedCompositorFrameSink(
+    const viz::FrameSinkId& frame_sink_id) {
+  auto it = frame_sink_elements_.find(frame_sink_id);
+  DCHECK(it != frame_sink_elements_.end());
+
+  FrameSinkElement* element = it->second;
+  element->SetClientConnected(false);
+
+  // If the FrameSink is still registered, update its status on is_connected.
+  if (base::ContainsKey(registered_frame_sink_ids_to_is_connected_,
+                        frame_sink_id)) {
+    registered_frame_sink_ids_to_is_connected_[frame_sink_id] = false;
+    return;
+  }
+
+  // If a FrameSink is invalidated and destroyed we should remove it from the
+  // tree.
+  RemoveFrameSinkSubtree(element);
+  DestroyChildSubtree(element->parent(), element);
+}
+
+void DOMAgentViz::OnRegisteredFrameSinkHierarchy(
+    const viz::FrameSinkId& parent_frame_sink_id,
+    const viz::FrameSinkId& child_frame_sink_id) {
+  // At this point these elements must be present in a tree.
+  // We should detach a child from its current parent and attach to the new
+  // parent.
+  auto it_parent = frame_sink_elements_.find(parent_frame_sink_id);
+  auto it_child = frame_sink_elements_.find(child_frame_sink_id);
+  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;
+
+  // TODO: Add support for |child| to have multiple parents.
+  if (child->parent())
+    child->parent()->RemoveChild(child);
+
+  new_parent->AddChild(child);
+  child->set_parent(new_parent);
+}
+
+void DOMAgentViz::OnUnregisteredFrameSinkHierarchy(
+    const viz::FrameSinkId& parent_frame_sink_id,
+    const viz::FrameSinkId& child_frame_sink_id) {
+  // At this point these elements must be present in a tree.
+  // We should detach a child from its current parent and attach to the
+  // RootElement since it wasn't destroyed yet.
+  auto it_parent = frame_sink_elements_.find(parent_frame_sink_id);
+  auto it_child = frame_sink_elements_.find(child_frame_sink_id);
+  DCHECK(it_parent != frame_sink_elements_.end());
+  DCHECK(it_child != frame_sink_elements_.end());
+
+  FrameSinkElement* child = it_child->second;
+  FrameSinkElement* parent = it_parent->second;
+
+  parent->RemoveChild(child);
+  // TODO: Add support for |child| to have multiple parents: only adds |child|
+  // to RootElement if all parents of |child| are unregistered.
+  child->set_parent(element_root());
+}
+
 std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForFrameSink(
-    UIElement* frame_sink_element,
+    FrameSinkElement* frame_sink_element,
     const viz::FrameSinkId& frame_sink_id) {
   frame_sink_elements_.emplace(frame_sink_id, frame_sink_element);
   std::unique_ptr<Array<DOM::Node>> children = Array<DOM::Node>::create();
@@ -31,14 +175,15 @@ std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForFrameSink(
   // subtree. So we iterate through |frame_sink_element|'s children and
   // recursively build the subtree for them.
   for (auto& child : frame_sink_manager_->GetChildrenByParent(frame_sink_id)) {
-    bool is_registered = registered_frame_sink_ids_.find(child) !=
-                         registered_frame_sink_ids_.end();
-    bool is_client_connected = client_connected_frame_sinks_.find(child) !=
-                               client_connected_frame_sinks_.end();
+    bool is_registered = base::ContainsKey(
+        registered_frame_sink_ids_to_is_connected_, frame_sink_id);
+    bool is_client_connected =
+        is_registered &&
+        registered_frame_sink_ids_to_is_connected_.find(frame_sink_id)->second;
 
-    UIElement* f_s_element = new FrameSinkElement(
-        child, frame_sink_manager_, this, frame_sink_element, false /*is_root*/,
-        is_registered, is_client_connected);
+    FrameSinkElement* f_s_element = new FrameSinkElement(
+        child, frame_sink_manager_, this, frame_sink_element,
+        /*is_root=*/false, is_registered, is_client_connected);
 
     children->addItem(BuildTreeForFrameSink(f_s_element, child));
     frame_sink_element->AddChild(f_s_element);
@@ -51,10 +196,12 @@ std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForFrameSink(
 
 protocol::Response DOMAgentViz::enable() {
   InitFrameSinkSets();
+  frame_sink_manager_->AddObserver(this);
   return protocol::Response::OK();
 }
 
 protocol::Response DOMAgentViz::disable() {
+  frame_sink_manager_->RemoveObserver(this);
   Clear();
   return DOMAgent::disable();
 }
@@ -62,43 +209,40 @@ protocol::Response DOMAgentViz::disable() {
 std::vector<UIElement*> DOMAgentViz::CreateChildrenForRoot() {
   std::vector<UIElement*> children;
 
-  // Step 1. Add created RootFrameSinks and detached FrameSinks.
-  for (auto& frame_sink_id : client_connected_frame_sinks_) {
-    const viz::CompositorFrameSinkSupport* support =
-        frame_sink_manager_->GetFrameSinkForId(frame_sink_id);
-    // Do nothing if it's a non-detached non-root FrameSink.
-    if (support && !support->is_root() &&
-        attached_frame_sinks_.find(frame_sink_id) !=
-            attached_frame_sinks_.end())
-      continue;
-
-    bool is_registered = registered_frame_sink_ids_.find(frame_sink_id) !=
-                         registered_frame_sink_ids_.end();
-    bool is_root = support && support->is_root();
-
-    UIElement* frame_sink_element = new FrameSinkElement(
-        frame_sink_id, frame_sink_manager_, this, element_root(), is_root,
-        is_registered, true /*is_client_connected*/);
-    children.push_back(frame_sink_element);
-  }
+  // Add created RootFrameSinks and detached FrameSinks.
+  for (auto& frame_sink_id : registered_frame_sink_ids_to_is_connected_) {
+    if (frame_sink_id.second) {
+      const viz::CompositorFrameSinkSupport* support =
+          frame_sink_manager_->GetFrameSinkForId(frame_sink_id.first);
+      // Do nothing if it's a non-detached non-root FrameSink.
+      if (support && !support->is_root() &&
+          attached_frame_sinks_.find(frame_sink_id.first) !=
+              attached_frame_sinks_.end()) {
+        continue;
+      }
+
+      bool is_root = support && support->is_root();
+
+      UIElement* frame_sink_element = new FrameSinkElement(
+          frame_sink_id.first, frame_sink_manager_, this, element_root(),
+          is_root,
+          /*is_registered=*/true, /*is_client_connected=*/true);
+      children.push_back(frame_sink_element);
+
+      // Add registered but not created FrameSinks. If a FrameSinkId was
+      // registered but not created we don't really know whether it's a root or
+      // not. And we don't know any information about the hierarchy. Therefore
+      // we process FrameSinks that are in the correct state first and only
+      // after that we process registered but not created FrameSinks. We
+      // consider them unembedded as well.
+    } else {
+      UIElement* frame_sink_element = new FrameSinkElement(
+          frame_sink_id.first, frame_sink_manager_, this, element_root(),
+          /*is_root=*/false, /*is_registered=*/true,
+          /*is_client_connected=*/false);
 
-  // Step 2. Add registered but not created FrameSinks. If a FrameSinkId was
-  // registered but not created we don't really know whether it's a root or not.
-  // And we don't know any information about the hierarchy. Therefore we process
-  // FrameSinks that are in the correct state first and only after that we
-  // process registered but not created FrameSinks. We consider them unembedded
-  // as well.
-  for (auto& frame_sink_id : registered_frame_sink_ids_) {
-    if (client_connected_frame_sinks_.find(frame_sink_id) !=
-        client_connected_frame_sinks_.end())
-      continue;
-
-    UIElement* frame_sink_element = new FrameSinkElement(
-        frame_sink_id, frame_sink_manager_, this, element_root(),
-        false /*is_root*/, true /*is_registered*/,
-        false /*is_client_connected*/);
-
-    children.push_back(frame_sink_element);
+      children.push_back(frame_sink_element);
+    }
   }
 
   return children;
@@ -107,35 +251,55 @@ std::vector<UIElement*> DOMAgentViz::CreateChildrenForRoot() {
 std::unique_ptr<DOM::Node> DOMAgentViz::BuildTreeForUIElement(
     UIElement* ui_element) {
   if (ui_element->type() == UIElementType::FRAMESINK) {
-    return BuildTreeForFrameSink(ui_element,
-                                 FrameSinkElement::From(ui_element));
+    viz::FrameSinkId frame_sink_id = FrameSinkElement::From(ui_element);
+    bool is_registered = base::ContainsKey(
+        registered_frame_sink_ids_to_is_connected_, frame_sink_id);
+    bool is_client_connected =
+        registered_frame_sink_ids_to_is_connected_.find(frame_sink_id)->second;
+    FrameSinkElement* frame_sink_element = new FrameSinkElement(
+        frame_sink_id, frame_sink_manager_, this, nullptr,
+        /*is_root=*/false, is_registered, is_client_connected);
+
+    return BuildTreeForFrameSink(frame_sink_element, frame_sink_id);
   }
   return nullptr;
 }
 
+void DOMAgentViz::DestroyChildSubtree(UIElement* parent, UIElement* child) {
+  std::vector<UIElement*> direct_children = child->children();
+  for (auto* kid : direct_children)
+    DestroyChildSubtree(child, kid);
+  parent->RemoveChild(child);
+  std::unique_ptr<UIElement> to_destroy(child);
+}
+
 void DOMAgentViz::Clear() {
   attached_frame_sinks_.clear();
-  client_connected_frame_sinks_.clear();
   frame_sink_elements_.clear();
-  registered_frame_sink_ids_.clear();
+  registered_frame_sink_ids_to_is_connected_.clear();
 }
 
 void DOMAgentViz::InitFrameSinkSets() {
   for (auto& entry : frame_sink_manager_->GetRegisteredFrameSinkIds())
-    registered_frame_sink_ids_.insert(entry);
-  for (auto& entry : frame_sink_manager_->GetCreatedFrameSinkIds())
-    client_connected_frame_sinks_.insert(entry);
+    registered_frame_sink_ids_to_is_connected_[entry] = false;
+  for (auto& entry : frame_sink_manager_->GetCreatedFrameSinkIds()) {
+    DCHECK(
+        base::ContainsKey(registered_frame_sink_ids_to_is_connected_, entry));
+    registered_frame_sink_ids_to_is_connected_[entry] = true;
+  }
 
   // Init the |attached_frame_sinks_| set. All RootFrameSinks and accessible
   // from roots are attached. All the others are detached.
-  for (auto& frame_sink_id : client_connected_frame_sinks_) {
-    const viz::CompositorFrameSinkSupport* support =
-        frame_sink_manager_->GetFrameSinkForId(frame_sink_id);
-    // Start only from roots.
-    if (!support || !support->is_root())
-      continue;
-
-    SetAttachedFrameSink(frame_sink_id);
+  for (auto& frame_sink_id : registered_frame_sink_ids_to_is_connected_) {
+    if (frame_sink_id.second) {
+      const viz::CompositorFrameSinkSupport* support =
+          frame_sink_manager_->GetFrameSinkForId(frame_sink_id.first);
+      // Start only from roots.
+      if (!support || !support->is_root())
+        continue;
+
+      SetAttachedFrameSink(frame_sink_id.first);
+    }
   }
 }
 
@@ -145,4 +309,26 @@ void DOMAgentViz::SetAttachedFrameSink(const viz::FrameSinkId& frame_sink_id) {
     SetAttachedFrameSink(child);
 }
 
+void DOMAgentViz::RemoveFrameSinkSubtree(UIElement* root) {
+  // We may come across the case where we've got the event to delete the
+  // FrameSink, but we haven't got events to delete its children. We should
+  // detach all its children and attach them to RootElement and then delete the
+  // node we were asked for.
+  std::vector<viz::FrameSinkId> children;
+  frame_sink_elements_.erase(FrameSinkElement::From(root));
+  for (auto* child : root->children()) {
+    RemoveFrameSinkElement(child);
+    child->set_parent(element_root());
+  }
+
+  if (root->parent())
+    root->parent()->RemoveChild(root);
+}
+
+void DOMAgentViz::RemoveFrameSinkElement(UIElement* element) {
+  frame_sink_elements_.erase(FrameSinkElement::From(element));
+  for (auto* child : element->children())
+    RemoveFrameSinkElement(child);
+}
+
 }  // namespace ui_devtools

components/ui_devtools/viz_views/dom_agent_viz.h

@@ -10,21 +10,37 @@
 #include "components/ui_devtools/DOM.h"
 #include "components/ui_devtools/dom_agent.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
+#include "components/viz/service/frame_sinks/frame_sink_observer.h"
 
 namespace viz {
 class FrameSinkManagerImpl;
 }
 
 namespace ui_devtools {
+class FrameSinkElement;
 
-class DOMAgentViz : public DOMAgent {
+class DOMAgentViz : public viz::FrameSinkObserver, public DOMAgent {
  public:
   explicit DOMAgentViz(viz::FrameSinkManagerImpl* frame_sink_manager);
   ~DOMAgentViz() override;
 
+  // viz::FrameSinkObserver:
+  void OnRegisteredFrameSinkId(const viz::FrameSinkId& frame_sink_id) override;
+  void OnInvalidatedFrameSinkId(const viz::FrameSinkId& frame_sink_id) override;
+  void OnCreatedCompositorFrameSink(const viz::FrameSinkId& frame_sink_id,
+                                    bool is_root) override;
+  void OnDestroyedCompositorFrameSink(
+      const viz::FrameSinkId& frame_sink_id) override;
+  void OnRegisteredFrameSinkHierarchy(
+      const viz::FrameSinkId& parent_frame_sink_id,
+      const viz::FrameSinkId& child_frame_sink_id) override;
+  void OnUnregisteredFrameSinkHierarchy(
+      const viz::FrameSinkId& parent_frame_sink_id,
+      const viz::FrameSinkId& child_frame_sink_id) override;
+
  private:
   std::unique_ptr<protocol::DOM::Node> BuildTreeForFrameSink(
-      UIElement* frame_sink_element,
+      FrameSinkElement* frame_sink_element,
       const viz::FrameSinkId& frame_sink_id);
 
   // DOM::Backend:
@@ -36,6 +52,10 @@ class DOMAgentViz : public DOMAgent {
   std::unique_ptr<protocol::DOM::Node> BuildTreeForUIElement(
       UIElement* ui_element) override;
 
+  // Removes the |child| subtree from the |parent| subtree and destroys every
+  // element in the |child| subtree.
+  void DestroyChildSubtree(UIElement* parent, UIElement* child);
+
   // Every time the frontend disconnects we don't destroy DOMAgent so once we
   // establish the connection again we need to clear the FrameSinkId sets
   // because they may carry obsolete data. Then we initialize these with alive
@@ -50,13 +70,33 @@ class DOMAgentViz : public DOMAgent {
   // Mark a FrameSink that has |frame_sink_id| and all its subtree as attached.
   void SetAttachedFrameSink(const viz::FrameSinkId& frame_sink_id);
 
-  // These sets are used to create and update the DOM tree.
-  base::flat_set<viz::FrameSinkId> registered_frame_sink_ids_;
-  base::flat_set<viz::FrameSinkId> client_connected_frame_sinks_;
-
-  // This is used to track created FrameSinkElements and will be used for
-  // updates in a FrameSink tree.
-  base::flat_map<viz::FrameSinkId, UIElement*> frame_sink_elements_;
+  // We delete the FrameSinkElements in the subtree rooted at |root| from
+  // |frame_sink_elements_| and attach all its children to the root_element().
+  void RemoveFrameSinkSubtree(UIElement* root);
+
+  // Remove FrameSinkElements for subtree rooted at |element| from the tree
+  // |frame_sink_elements_|.
+  void RemoveFrameSinkElement(UIElement* element);
+
+  // These sets are used to create and update the DOM tree. We add/remove
+  // registered FrameSinks to |registered_frame_sink_ids_to_is_connected_| when
+  // FrameSink is registered/invalidated and initialize the bool to false to
+  // indicate that the FrameSink is not created yet. Then when a |frame_sink| is
+  // created/destroyed, we update |registered_frame_sink_ids_to_is_connected_|
+  // of |frame_sink| true or false accordingly. When we get events
+  // registered/unregistered hierarchy we don't change these sets because we
+  // detach a subtree from one node and attach it to another node and the list
+  // of registered/created FrameSinkIds doesn't change.
+  // TODO(yiyix): Removes this map because it saves duplicated information as
+  // |frame_sink_elements_|.
+  base::flat_map<viz::FrameSinkId, bool>
+      registered_frame_sink_ids_to_is_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_;
 
   // This is used to denote attached FrameSinks.
   base::flat_set<viz::FrameSinkId> attached_frame_sinks_;

components/ui_devtools/viz_views/frame_sink_element.h

@@ -26,6 +26,17 @@ class FrameSinkElement : public UIElement {
                    bool is_client_connected);
   ~FrameSinkElement() override;
 
+  // Used by DOMAgentViz on updates when element is already present
+  // in a tree but its properties need to be changed.
+  void SetRegistered(bool is_registered) { is_registered_ = is_registered; }
+  void SetClientConnected(bool is_client_connected) {
+    is_client_connected_ = is_client_connected;
+  }
+  void SetRoot(bool is_root) { is_root_ = is_root; }
+
+  bool is_registered() const { return is_registered_; }
+  bool is_client_connected() const { return is_client_connected_; }
+
   // UIElement:
   std::vector<std::pair<std::string, std::string>> GetCustomProperties()
       const override;