Resetting FrameTreeNode must remove child FrameTreeNodes from global map.

Resetting FrameTreeNode must remove child FrameTreeNodes from global map.

BUG=451099, 455060
Committed: https://crrev.com/aeca57bc739e9531b1652abc4b782f530a1dcc90
Cr-Commit-Position: refs/heads/master@{#316106}

[nasko, 2015-02-12 01:05:09]

Patchset #1 (id:1) has been deleted

[nasko, 2015-02-12 01:06:58]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2015-02-12 01:06:58]

Hey Charlie,
Can you review this CL for me? It is a simple thing I've missed that needs to be
fixed. The first patchset is one way of solving it. 
Alternative is to move the map to FrameTreeNode and the static Find* function to
be on the FTN. I don't have a strong preference for which one.
Thanks in advance!
Nasko

[Charlie Reis, 2015-02-12 17:12:43]

Would RemoveFrame be an improvement?

If there's issues with that, I'm not opposed to moving the global map to FTN.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:79: // This method only removes the
FrameTreeNode from the global map of ids
It feels a bit odd to have a public method for modifying a private global map. 
It would be nice to hide this behind RemoveFrame.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:46:
frame_tree_->RemoveNodeFromMap(frame_tree_node_id_);
Why not frame_tree->RemoveFrame(this)?  I'm guessing that we're missing the
FrameDeleted notification in this case as well, and RemoveFrame would handle
that.

(We may have to skip that call for the main frame, based on the NOTREACHED in
Remove Frame.)

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:258: // Add a couple of child
frmaes to the main frame.
nit: drop "of"
nit: frames

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:270: // Crash the renderer
nit: End with period.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:278: 
nit: Too many blank lines.

[nasko, 2015-02-12 23:21:08]

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:79: // This method only removes the
FrameTreeNode from the global map of ids
On 2015/02/12 17:12:43, Charlie Reis wrote:
> It feels a bit odd to have a public method for modifying a private global map.

> It would be nice to hide this behind RemoveFrame.

Done.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:46:
frame_tree_->RemoveNodeFromMap(frame_tree_node_id_);
On 2015/02/12 17:12:43, Charlie Reis wrote:
> Why not frame_tree->RemoveFrame(this)?  I'm guessing that we're missing the
> FrameDeleted notification in this case as well, and RemoveFrame would handle
> that.
> 
> (We may have to skip that call for the main frame, based on the NOTREACHED in
> Remove Frame.)

Done.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_unittest.cc (right):

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:258: // Add a couple of child
frmaes to the main frame.
On 2015/02/12 17:12:43, Charlie Reis wrote:
> nit: drop "of"
> nit: frames

Done.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:270: // Crash the renderer
On 2015/02/12 17:12:43, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/917793006/diff/20001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_unittest.cc:278: 
On 2015/02/12 17:12:43, Charlie Reis wrote:
> nit: Too many blank lines.

Done.

[Charlie Reis, 2015-02-12 23:26:07]

Great!  LGTM with nits.

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:97: // that nodes are properly
remove from the tree and notifications are sent.
nit: removed

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:99: // will be a noop and will not
result in traversing twice through the list.
nit: no-op
nit: s/traversing twice through/repeatedly traversing/

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:100: for (const auto& child :
old_children) {
nit: No braces needed.

[nasko, 2015-02-12 23:30:39]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[nasko, 2015-02-12 23:30:51]

Nits fixed.

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:97: // that nodes are properly
remove from the tree and notifications are sent.
On 2015/02/12 23:26:07, Charlie Reis wrote:
> nit: removed

Done.

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:99: // will be a noop and will not
result in traversing twice through the list.
On 2015/02/12 23:26:07, Charlie Reis wrote:
> nit: no-op
> nit: s/traversing twice through/repeatedly traversing/

Done.

https://codereview.chromium.org/917793006/diff/40001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.cc:100: for (const auto& child :
old_children) {
On 2015/02/12 23:26:07, Charlie Reis wrote:
> nit: No braces needed.

Done.

[nasko, 2015-02-12 23:30:54]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-02-12 23:31:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/917793006/60001

[commit-bot: I haz the power, 2015-02-13 00:50:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-02-13 00:51:28]

Patchset 3 (id:??) landed as
https://crrev.com/aeca57bc739e9531b1652abc4b782f530a1dcc90
Cr-Commit-Position: refs/heads/master@{#316106}