Track all changed nodes during an update

ui/accessibility/ax_tree.cc

Index: ui/accessibility/ax_tree.cc
diff --git a/ui/accessibility/ax_tree.cc b/ui/accessibility/ax_tree.cc
index f4be46059453fa219f5dc61d4491d26c04433214..761e21d3ad905ccf259a5be229e5b7d0fd2eb178 100644
--- a/ui/accessibility/ax_tree.cc
+++ b/ui/accessibility/ax_tree.cc
@@ -36,6 +36,12 @@ struct AXTreeUpdateState {
   // end of Unserialize.
   std::set<AXNode*> pending_nodes;
 
+  // This is similar to above, but we store node ids here because this list gets
+  // generated before any nodes get created or re-used. Its purpose is to allow
+  // us to know what nodes will be updated so we can make more intelligent
+  // decisions about when to notify delegates of removals or reparenting.
+  std::set<int> changed_node_ids;
+
   // Keeps track of new nodes created during this update.
   std::set<AXNode*> new_nodes;
 
@@ -92,6 +98,14 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
   AXTreeUpdateState update_state;
   int32_t old_root_id = root_ ? root_->id() : 0;
 
+  // First, make a note of any nodes we will touch as part of this update
+  // including implicitly via parent/child relationships.
+  for (size_t i = 0; i < update.nodes.size(); ++i) {
+    update_state.changed_node_ids.insert(update.nodes[i].id);
+    for (size_t j = 0; j < update.nodes[i].child_ids.size(); ++j)
+      update_state.changed_node_ids.insert(update.nodes[i].child_ids[j]);
+  }
+
   if (update.has_tree_data)
     UpdateData(update.tree_data);
 
@@ -247,14 +261,18 @@ bool AXTree::UpdateNode(const AXNodeData& src,
 
 void AXTree::DestroySubtree(AXNode* node,
                             AXTreeUpdateState* update_state) {
-  if (delegate_)
+  if (delegate_ && (!update_state ||

[dmazzoni, 2016/09/06 22:49:44]

I don't think we support |update_state| being nullptr anywhere.
It's only a pointer because it's modifiable and because its
lifetime is less than the lifetime of the AXTree object.
I'd just make it a DCHECK instead.

[David Tseng, 2016/09/07 16:37:01]

Alright, done, DestroySubtree only gets called from Unserialize, so this appears
to be right.


Btw, are you ok with not notifying the delegate of a subtree deletion when the
AXTree is destroyed? I think that might be the last (hopefully) remaining corner
case.

+                    update_state->changed_node_ids.find(node->id()) ==
+                        update_state->changed_node_ids.end()))
     delegate_->OnSubtreeWillBeDeleted(this, node);
   DestroyNodeAndSubtree(node, update_state);
 }
 
 void AXTree::DestroyNodeAndSubtree(AXNode* node,
                                    AXTreeUpdateState* update_state) {
-  if (delegate_)
+  if (delegate_ && (!update_state ||
+                    update_state->changed_node_ids.find(node->id()) ==
+                        update_state->changed_node_ids.end()))
     delegate_->OnNodeWillBeDeleted(this, node);
   id_map_.erase(node->id());
   for (int i = 0; i < node->child_count(); ++i)