Fix a case where AXTreeSerializer didn't handle reparenting.

ui/accessibility/ax_tree_serializer.h

Index: ui/accessibility/ax_tree_serializer.h
diff --git a/ui/accessibility/ax_tree_serializer.h b/ui/accessibility/ax_tree_serializer.h
index f1cbfcea1053760d218d8686fe310273c6a0f7e5..8576433c2406e2d200db3f13f8b0974e2adb515b 100644
--- a/ui/accessibility/ax_tree_serializer.h
+++ b/ui/accessibility/ax_tree_serializer.h
@@ -292,36 +292,44 @@ void AXTreeSerializer<AXSourceNode>::SerializeChanges(
   // with the LCA.
   AXSourceNode lca = LeastCommonAncestor(node);
 
-  if (client_root_) {
-    bool need_delete = false;
-    if (tree_->IsValid(lca)) {
-      // Check for any reparenting within this subtree - if there is
-      // any, we need to delete and reserialize the whole subtree
-      // that contains the old and new parents of the reparented node.
-      if (AnyDescendantWasReparented(lca, &lca))
-        need_delete = true;
-    }
+  bool need_delete;
+  do {
+    need_delete = false;
+    if (client_root_) {
+      if (tree_->IsValid(lca)) {
+        // Check for any reparenting within this subtree - if there is
+        // any, we need to delete and reserialize the whole subtree
+        // that contains the old and new parents of the reparented node.

[David Tseng, 2015/02/27 19:00:47]

Mention something about what's happening with the lca and the relationship it
has with the return value of AnyDescendantReparented. Also, state the reason why
we're doing this repeatedly.

+        if (AnyDescendantWasReparented(lca, &lca))
+          need_delete = true;
+      }
 
-    if (!tree_->IsValid(lca)) {
-      // If there's no LCA, just tell the client to destroy the whole
-      // tree and then we'll serialize everything from the new root.
-      out_update->node_id_to_clear = client_root_->id;
-      Reset();
-    } else if (need_delete) {
-      // Otherwise, if we need to reserialize a subtree, first we need
-      // to delete those nodes in our client tree so that
-      // SerializeChangedNodes() will be sure to send them again.
-      out_update->node_id_to_clear = tree_->GetId(lca);
-      ClientTreeNode* client_lca = ClientTreeNodeById(tree_->GetId(lca));
-      CHECK(client_lca);
-      for (size_t i = 0; i < client_lca->children.size(); ++i) {
-        client_id_map_.erase(client_lca->children[i]->id);
-        DeleteClientSubtree(client_lca->children[i]);
-        delete client_lca->children[i];
+      if (!tree_->IsValid(lca)) {
+        // If there's no LCA, just tell the client to destroy the whole
+        // tree and then we'll serialize everything from the new root.
+        out_update->node_id_to_clear = client_root_->id;
+        Reset();
+      } else if (need_delete) {
+        // Otherwise, if we need to reserialize a subtree, first we need
+        // to delete those nodes in our client tree so that
+        // SerializeChangedNodes() will be sure to send them again.
+        out_update->node_id_to_clear = tree_->GetId(lca);
+        ClientTreeNode* client_lca = ClientTreeNodeById(tree_->GetId(lca));
+        CHECK(client_lca);
+
+        for (size_t i = 0; i < client_lca->children.size(); ++i) {

[David Tseng, 2015/02/27 19:00:46]

I actually think we should call DeleteClientSubtree directly here (even for the
merge) since it makes things a lot clearer.

+          client_id_map_.erase(client_lca->children[i]->id);
+          DeleteClientSubtree(client_lca->children[i]);

[David Tseng, 2015/02/27 19:19:43]

nit: not for this cl, but this really should be s/Subtree/Descendants as I
understand the usage of these terms.

+          delete client_lca->children[i];
+        }
+        client_lca->children.clear();
       }
-      client_lca->children.clear();
     }
-  }
+
+    // If we deleted a client subtree, we have to call
+    // AnyDescendantWasReparented again because the computed LCA may be
+    // different now.

[David Tseng, 2015/02/27 19:00:47]

Not sure I like this comment here. Maybe put it outside/above the loop and
rephrase accordingly.

+  } while (need_delete);
 
   // Serialize from the LCA, or from the root if there isn't one.
   if (!tree_->IsValid(lca))