Remove existing FrameNavigationEntry when new named frame is added.

content/browser/frame_host/navigation_entry_impl.cc

 // 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.
 
 #include "content/browser/frame_host/navigation_entry_impl.h"
 
 #include <stddef.h>
 
 #include <queue>
 #include <utility>
@@ skipping 80 matching lines @@
   for (auto& file : exploded_page_state.referenced_files)
     referenced_files->push_back(file);
 
   state->children.resize(node->children.size());
   for (size_t i = 0; i < node->children.size(); ++i) {
     RecursivelyGenerateFrameState(node->children[i], &state->children[i],
                                   referenced_files);
   }
 }
 
+using ParentingMap =
+    std::map<NavigationEntryImpl::TreeNode*, NavigationEntryImpl::TreeNode*>;

[Charlie Reis, 2016/07/29 17:21:36]

nit: Please document which parameter is which.

[nasko, 2016/08/01 20:42:16]

Removed in refactor.

+
+// Walk the ancestor chain for both the |frame_tree_node| and the
+// |node|. If a mismatch in unique name is detected, |node| needs to be removed.

[Charlie Reis, 2016/07/29 17:21:35]

nit: There's no |node| parameter.  :)

It's also unclear what to pass in here.  I think frame_tree_node is supposed to
be the match for parent_node, but that's not obvious without looking at the
implementation.

[nasko, 2016/08/01 20:42:16]

Done.

+void WalkAncestorsAndRemoveIfMismatchFound(

[Charlie Reis, 2016/07/29 17:21:36]

Can we make this just return whether the ancestors match, and let the other
function do the removal?  Such a helper would be simpler and might be useful in
other contexts.  Something like DoAncestorsMatch(frame_tree_node, tree_node,
parenting_map, root_node)?  Or InSameTreePosition?  (That order of params might
also make it clearer to pass: the first two are the ones being compared
directly, and the rest are the necessary context.)

[nasko, 2016/08/01 20:42:16]

Done.

+    const ParentingMap& parenting_map,
+    FrameTreeNode* frame_tree_node,
+    NavigationEntryImpl::TreeNode* root_node,
+    NavigationEntryImpl::TreeNode* node_to_remove,
+    NavigationEntryImpl::TreeNode* parent_node) {
+  FrameTreeNode* ftn = frame_tree_node;
+  NavigationEntryImpl::TreeNode* current_node = parent_node;
+  while (ftn && current_node) {
+    if (!current_node->MatchesFrame(ftn, current_node == root_node)) {

[Charlie Reis, 2016/07/29 17:21:36]

I apologize for MatchesFrame taking in is_root_tree_node.  :(  That's the only
reason we need to pass |root_node| here, right?  Hopefully we can remove that if
we prevent frames from changing their unique name after creation time.

[nasko, 2016/08/01 20:42:16]

Done.

+      auto it = std::find(parent_node->children.begin(),
+                          parent_node->children.end(), node_to_remove);
+      DCHECK(it != parent_node->children.end());

[Charlie Reis, 2016/07/29 17:21:35]

nit: DCHECK_NE, unless the compiler doesn't like it.

[nasko, 2016/08/01 20:42:16]

Compiler complains about it.

+      parent_node->children.erase(it);
+      return;

[Charlie Reis, 2016/07/29 17:21:36]

Both of these erase blocks could just be return false, eliminating the code
duplication.

[nasko, 2016/08/01 20:42:16]

Done.

+    }
+
+    auto parent_iterator = parenting_map.find(current_node);

[Charlie Reis, 2016/07/29 17:21:35]

Do we still need parenting_map as opposed to having parent pointers?  I'm trying
to understand why a parent pointer would be insufficient now.

(Bonus points: Having a parent pointer would mean we don't need
is_root_tree_node in MatchesFrame!)

[nasko, 2016/08/01 20:42:16]

Done.

+    if (parent_iterator == parenting_map.end()) {
+      auto it = std::find(parent_node->children.begin(),
+                          parent_node->children.end(), node_to_remove);
+      DCHECK(it != parent_node->children.end());
+      parent_node->children.erase(it);
+      return;
+    }
+    ftn = ftn->parent();
+    current_node = parent_iterator->second;
+  }
+}
+
 }  // namespace
 
 int NavigationEntryImpl::kInvalidBindings = -1;
 
 NavigationEntryImpl::TreeNode::TreeNode(FrameNavigationEntry* frame_entry)
     : frame_entry(frame_entry) {
 }
 
 NavigationEntryImpl::TreeNode::~TreeNode() {
 }
@@ skipping 684 matching lines @@
   NavigationEntryImpl::TreeNode* tree_node = FindFrameEntry(frame_tree_node);
   return tree_node ? tree_node->frame_entry.get() : nullptr;
 }
 
 void NavigationEntryImpl::ClearChildren(FrameTreeNode* frame_tree_node) {
   NavigationEntryImpl::TreeNode* tree_node = FindFrameEntry(frame_tree_node);
   if (tree_node)
     tree_node->children.clear();
 }
 
+void NavigationEntryImpl::ClearMatchingFrameEntries(
+    FrameTreeNode* frame_tree_node) {
+  DCHECK(!frame_tree_node->IsMainFrame());
+
+  int count = 0;
+  NavigationEntryImpl::TreeNode* node = nullptr;
+  NavigationEntryImpl::TreeNode* parent_node = nullptr;
+  std::queue<NavigationEntryImpl::TreeNode*> work_queue;
+  ParentingMap parenting_map;
+
+  work_queue.push(root_node());
+  parenting_map.insert(std::make_pair(root_node(), nullptr));
+
+  while (!work_queue.empty()) {
+    node = work_queue.front();
+    parent_node = parenting_map.find(node)->second;
+    work_queue.pop();
+
+    if (node->MatchesFrame(frame_tree_node, node == root_node())) {
+      WalkAncestorsAndRemoveIfMismatchFound(parenting_map,
+                                            frame_tree_node->parent(),
+                                            root_node(), node, parent_node);
+      ++count;
+      continue;
+    }
+
+    // Enqueue any children and keep looking.
+    for (auto* child : node->children) {
+      work_queue.push(child);
+      parenting_map.insert(std::make_pair(child, node));
+    }
+  }
+
+  DCHECK_GE(1, count);

[Charlie Reis, 2016/07/29 17:21:35]

I think this might be easier to read as DCHECK_LE(count, 1), even though they're
functionally equivalent.  I was reading the current one as "make sure we delete
at least 1" instead of "at most 1."

[nasko, 2016/08/01 20:42:16]

Done.

+}
+
 void NavigationEntryImpl::SetScreenshotPNGData(
     scoped_refptr<base::RefCountedBytes> png_data) {
   screenshot_ = png_data;
   if (screenshot_.get())
     UMA_HISTOGRAM_MEMORY_KB("Overscroll.ScreenshotSize", screenshot_->size());
 }
 
 GURL NavigationEntryImpl::GetHistoryURLForDataURL() const {
   return GetBaseURLForDataURL().is_empty() ? GURL() : GetVirtualURL();
 }
@@ skipping 10 matching lines @@
       return node;
 
     // Enqueue any children and keep looking.
     for (auto* child : node->children)
       work_queue.push(child);
   }
   return nullptr;
 }
 
 }  // namespace content