PlzNavigate: Add a static CommitNavigation method in FrameTreeNode

Add a static FrameTreeNode lookup method in FrameTreeNode

This CL adds a static FrameTree::GloballyFindByID that can be used by to find a FrameTreeNode given a frame tree node id.

BUG=376082
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289513

[clamy, 2014-07-29 13:47:50]

@nasko:PTAL
@davidben:FYI
This CL is dependent on https://chromiumcodereview.appspot.com/379143002/ (which
I hope should land soon). The goal is to provide a static method that the IO
thread can bind to inform the UI thread of navigation commit. When the other CL
lands, I also plan to modify the commit unit tests so that they use the static
function (and the full code path is exercised).

[nasko, 2014-07-30 08:36:16]

Just a couple of comments, otherwise it looks straightforward.

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
File content/browser/frame_host/frame_tree_node.cc (right):

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
content/browser/frame_host/frame_tree_node.cc:38: void
FrameTreeNode::CommitNavigationInNode(int64 frame_tree_node_id,
nit: the "InNode" part of the method is redundant, since this is already invoked
on a FrameTreeNode. It would make sense if it was a method on FrameTree.

This brings up another question. Does this belong to FrameTree or the FTN? I
don't have a strong preference, but it feels a bit awkward to call a method on
FTN without knowing which FTN it is about.

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
content/browser/frame_host/frame_tree_node.h:32: // Commits a navigation for
this node (in browser-driven navigation).
nit: Let's make sure this is updated once we agree on PlzNavigate labeling of
comments on the other CL.

[clamy, 2014-08-12 13:41:47]

Thanks! I have moved the method to FrameTree instead. I add/remove nodes from
the map when they are actually added/removed from the FrameTree instead of upon
construction/deletion, because I did not want to expose functions in FrameTree
to be called by FrameTreeNode constructor/destructor. I also removed the method
to find a FrameTreeNode by its id that was traversing the tree, since we now
have a hash map.

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
File content/browser/frame_host/frame_tree_node.cc (right):

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
content/browser/frame_host/frame_tree_node.cc:38: void
FrameTreeNode::CommitNavigationInNode(int64 frame_tree_node_id,
On 2014/07/30 08:36:16, nasko wrote:
> nit: the "InNode" part of the method is redundant, since this is already
invoked
> on a FrameTreeNode. It would make sense if it was a method on FrameTree.
> 
> This brings up another question. Does this belong to FrameTree or the FTN? I
> don't have a strong preference, but it feels a bit awkward to call a method on
> FTN without knowing which FTN it is about.

Done.

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
File content/browser/frame_host/frame_tree_node.h (right):

https://chromiumcodereview.appspot.com/429603002/diff/1/content/browser/frame...
content/browser/frame_host/frame_tree_node.h:32: // Commits a navigation for
this node (in browser-driven navigation).
On 2014/07/30 08:36:16, nasko wrote:
> nit: Let's make sure this is updated once we agree on PlzNavigate labeling of
> comments on the other CL.

Done.

[nasko, 2014-08-13 00:52:53]

LGTM with a couple of nits.

https://chromiumcodereview.appspot.com/429603002/diff/20001/content/browser/f...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/429603002/diff/20001/content/browser/f...
content/browser/frame_host/frame_tree.cc:35: FrameTreeNode*
FrameTreeNodeForGlobalId(int64 frame_tree_node_id) {
nit: The FTN id is global in all cases, so we can drop the "Global" part of the
name.

https://chromiumcodereview.appspot.com/429603002/diff/20001/content/browser/f...
content/browser/frame_host/frame_tree.cc:96:
std::make_pair(root_->frame_tree_node_id(), root_.get()));
Let's check here that we never insert duplicate entries. RF(H) uses that pattern
already.

[Charlie Reis, 2014-08-13 01:13:15]

Drive-by question:

Is it possible to just add a static way to look up a FrameTreeNode by ID, and
have the caller ask the FTN's render_manager() to commit?  There's currently no
navigation logic on FrameTree, and it's probably better not to introduce it.

[clamy, 2014-08-13 12:01:38]

Thanks! I modified the CL so that it now only adds a static lookup method, and
reintroduced the old one. I was worried that using the static one in all cases
could lead us to return pointers to FrameTreeNodes that are not in the specific
FrameTree we are concerned about.

[nasko, 2014-08-13 14:30:44]

https://chromiumcodereview.appspot.com/429603002/diff/40001/content/browser/f...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/429603002/diff/40001/content/browser/f...
content/browser/frame_host/frame_tree.cc:101: DCHECK(result.second);
nit: This should never happen, so let's make it CHECK.

[Charlie Reis, 2014-08-13 14:38:22]

Thanks!  LGTM with Nasko's comment.

[clamy, 2014-08-13 14:44:43]

Thanks!

https://chromiumcodereview.appspot.com/429603002/diff/40001/content/browser/f...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/429603002/diff/40001/content/browser/f...
content/browser/frame_host/frame_tree.cc:101: DCHECK(result.second);
On 2014/08/13 14:30:44, nasko wrote:
> nit: This should never happen, so let's make it CHECK.

Done.

[clamy, 2014-08-13 14:44:46]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-08-13 14:45:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/clamy@chromium.org/429603002/60001

[commit-bot: I haz the power, 2014-08-13 17:23:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-13 17:51:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-13 17:51:47]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[clamy, 2014-08-14 09:07:40]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-08-14 09:10:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/clamy@chromium.org/429603002/60001

[commit-bot: I haz the power, 2014-08-14 10:28:02]

Committed patchset #4 (60001) as 289513