Fix bug where removed (but not deleted) windows are not reflected in the tree properly

ash/common/devtools/ash_devtools_dom_agent.cc

Index: ash/common/devtools/ash_devtools_dom_agent.cc
diff --git a/ash/common/devtools/ash_devtools_dom_agent.cc b/ash/common/devtools/ash_devtools_dom_agent.cc
index fca24efa4799afe22978610c1f5cd75c60405b37..1741ab306080d8adf40abb8982cca178f815b762 100644
--- a/ash/common/devtools/ash_devtools_dom_agent.cc
+++ b/ash/common/devtools/ash_devtools_dom_agent.cc
@@ -101,24 +101,29 @@ ui::devtools::protocol::Response AshDevToolsDOMAgent::getDocument(
   return ui::devtools::protocol::Response::OK();
 }
 
-// Need to remove node in OnWindowDestroying because the window parent reference
-// is gone in OnWindowDestroyed
-void AshDevToolsDOMAgent::OnWindowDestroying(WmWindow* window) {
-  RemoveWindowNode(window, window->GetParent());
+// Handles removing windows.
+void AshDevToolsDOMAgent::OnWindowTreeChanging(WmWindow* window,
+                                               const TreeChangeParams& params) {
+  // Only trigger this when window == params.old_parent.
+  // Also want to ignore rearranges or additions. Only removals are handled
+  // here. This is because OnWindowTreeChanged is only called if there is a
+  // new_parent. If there is no new_parent, we have to handle the removal
+  // of the node here. We only trigger this 0 or 1 times as an old_parent will
+  // either exist and only call this callback once, or not at all.
+  if (window == params.old_parent)

[sadrul, 2016/11/09 16:09:26]

Should you check that params.new_parent is nullptr?

[Sarmad Hashmi, 2016/11/09 17:51:30]

Updated comment. This handles the removing when reorganizing from
params.old_parent to params.new_parent. So, params.new_parent can be null or
not, either way we simply only remove the node here.

+    RemoveWindowNode(params.target, params.old_parent);
 }
 
+// Handles adding windows.
 void AshDevToolsDOMAgent::OnWindowTreeChanged(WmWindow* window,
                                               const TreeChangeParams& params) {
-  // Only trigger this when window == root window.
-  // Removals are handled on OnWindowDestroying.
-  if (window != window->GetRootWindow() || !params.new_parent)
-    return;
-  // If there is an old_parent + new_parent, then this window is being moved
-  // which requires a remove followed by an add. If only new_parent
-  // exists, then a new window is being created so only add is called.
-  if (params.old_parent)
-    RemoveWindowNode(params.target, params.old_parent);
-  AddWindowNode(params.target);
+  // Only trigger this when window == params.new_parent.
+  // If there is an old_parent + new_parent, then this window's node was
+  // removed in OnWindowTreeChanging and will now be added to the new_parent.
+  // If there is only a new_parent, OnWindowTreeChanging is never called and
+  // the window is only added here.
+  if (window == params.new_parent)
+    AddWindowNode(params.target);
 }
 
 void AshDevToolsDOMAgent::OnWindowStackingChanged(WmWindow* window) {
@@ -154,6 +159,7 @@ AshDevToolsDOMAgent::BuildInitialTree() {
 }
 
 void AshDevToolsDOMAgent::AddWindowNode(WmWindow* window) {
+  DCHECK(window_to_node_id_map_.count(window->GetParent()));
   WmWindow* prev_sibling = FindPreviousSibling(window);
   frontend()->childNodeInserted(
       window_to_node_id_map_[window->GetParent()],
@@ -168,7 +174,12 @@ void AshDevToolsDOMAgent::RemoveWindowNode(WmWindow* window,
   DCHECK(it != window_to_node_id_map_.end());
 
   int node_id = it->second;
-  int parent_id = old_parent ? window_to_node_id_map_[old_parent] : 0;
+  int parent_id = 0;
+  if (window != window->GetRootWindow()) {

[sadrul, 2016/11/09 16:09:26]

Is |window| ever the root-window here, i.e. are there cases when this condition
is not met? Because a root-window wouldn't have a parent, i.e. old_parent will
be null, and this function should not be called in those cases. Which means
|window| should never be the root window. If that is the case, this should
probably be a DCHECK() instead.

[Sarmad Hashmi, 2016/11/09 17:51:30]

Ah, thanks for catching this. Done. I'm going to deal with RootWindow add/remove
in future CLs using OnRootWindowAdded in ShellObserver (and OnWindowDestroying).

+    DCHECK(old_parent);
+    DCHECK(window_to_node_id_map_.count(old_parent));
+    parent_id = window_to_node_id_map_[old_parent];
+  }
 
   window_to_node_id_map_.erase(it);
   frontend()->childNodeRemoved(parent_id, node_id);