Fix a case where AXTreeSerializer didn't handle reparenting.

ui/accessibility/ax_tree_serializer.h

 // Copyright 2013 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.
 
 #ifndef UI_ACCESSIBILITY_AX_TREE_SERIALIZER_H_
 #define UI_ACCESSIBILITY_AX_TREE_SERIALIZER_H_
 
 #include <set>
 
 #include "base/containers/hash_tables.h"
@@ skipping 274 matching lines @@
 }
 
 template<typename AXSourceNode>
 void AXTreeSerializer<AXSourceNode>::SerializeChanges(
     AXSourceNode node,
     AXTreeUpdate* out_update) {
   // If the node isn't in the client tree, we need to serialize starting
   // 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) {

[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();
+      }
     }
 
-    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];
-      }
-      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))
     lca = tree_->GetRoot();
   SerializeChangedNodes(lca, out_update);
 }
 
 template<typename AXSourceNode>
 void AXTreeSerializer<AXSourceNode>::DeleteClientSubtree(AXSourceNode node) {
   ClientTreeNode* client_node = ClientTreeNodeById(tree_->GetId(node));
@@ skipping 121 matching lines @@
   }
 
   // Serialize all of the new children, recursively.
   for (size_t i = 0; i < children_to_serialize.size(); ++i)
     SerializeChangedNodes(children_to_serialize[i], out_update);
 }
 
 }  // namespace ui
 
 #endif  // UI_ACCESSIBILITY_AX_TREE_SERIALIZER_H_