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

ash/common/devtools/ash_devtools_dom_agent.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/devtools/ash_devtools_dom_agent.h"
 
 #include "ash/common/wm_window.h"
 #include "components/ui_devtools/devtools_server.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
@@ skipping 83 matching lines @@
   Reset();
   return ui::devtools::protocol::Response::OK();
 }
 
 ui::devtools::protocol::Response AshDevToolsDOMAgent::getDocument(
     std::unique_ptr<ui::devtools::protocol::DOM::Node>* out_root) {
   *out_root = BuildInitialTree();
   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) {
   RemoveWindowNode(window, window->GetParent());
   AddWindowNode(window);
 }
 
 std::unique_ptr<DOM::Node> AshDevToolsDOMAgent::BuildTreeForWindow(
     ash::WmWindow* window) {
   std::unique_ptr<Array<DOM::Node>> children = Array<DOM::Node>::create();
@@ skipping 15 matching lines @@
 std::unique_ptr<ui::devtools::protocol::DOM::Node>
 AshDevToolsDOMAgent::BuildInitialTree() {
   std::unique_ptr<Array<DOM::Node>> children = Array<DOM::Node>::create();
   for (ash::WmWindow* window : shell_->GetAllRootWindows()) {
     children->addItem(BuildTreeForWindow(window));
   }
   return BuildNode("root", nullptr, std::move(children));
 }
 
 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()],
       prev_sibling ? window_to_node_id_map_[prev_sibling] : 0,
       BuildTreeForWindow(window));
 }
 
 void AshDevToolsDOMAgent::RemoveWindowNode(WmWindow* window,
                                            WmWindow* old_parent) {

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

Can you add a DCHECK that old_parent is non-null here as a
documentation/readability hint?

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

Done.

   window->RemoveObserver(this);
   WindowToNodeIdMap::iterator it = window_to_node_id_map_.find(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);
 }
 
 void AshDevToolsDOMAgent::RemoveObserverFromAllWindows() {
   for (auto& pair : window_to_node_id_map_)
     pair.first->RemoveObserver(this);
 }
 
 void AshDevToolsDOMAgent::Reset() {
   RemoveObserverFromAllWindows();
   window_to_node_id_map_.clear();
   node_ids = 1;
 }
 
 }  // namespace devtools
 }  // namespace ash