Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
429603002:
PlzNavigate: Add a static CommitNavigation method in FrameTreeNode (Closed)
DescriptionAdd 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
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed Nasko's comments #
Total comments: 2
Patch Set 3 : Only lookup for the FrameTreeNode #
Total comments: 2
Patch Set 4 : Changed the DCHECKs in CHECKs #Messages
Total messages: 17 (0 generated)
@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).
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.
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.
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.
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.
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.
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.
Thanks! LGTM with Nasko's comment.
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.
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/429603002/60001
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_...)
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/clamy@chromium.org/429603002/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as 289513 |
