Move out DidStartProvisionalLoad from WebContentsImpl into Navigator.

Move out DidStartProvisionalLoad from WebContentsImpl into Navigator.

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

[Charlie Reis, 2013-10-17 00:17:25]

Cool.  Just a few initial notes as we get going.

As we discussed, it probably makes sense to try to move a few things to a new
frame_host directory first, particularly breaking NavigationController's
dependency on WebContents.

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/frame_tree.h (right):

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/frame_tree.h:45: // session history for it and
NavigationObserver which dispatches
This doesn't seem to match the observer pattern, where multiple objects could
register to observe events dispatched by Navigator.  It sounds like you're using
it more like a delegate.

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/navigator.h (right):

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/navigator.h:21: NavigationControllerImpl*
nav_controller,
nit: This looks like it can fit on the previous line.

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/navigator.h:26: RenderFrameHost*
render_frame_host,
Same here.

https://codereview.chromium.org/26316005/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/navigator.h:35: NavigatorObserver* observer_;
Perhaps this should be an ObserverList, as in WebContentsImpl?

[awong, 2013-11-06 21:22:58]

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:47: }
nit: 
}  // namespace content

[Charlie Reis, 2013-11-07 01:22:42]

Thanks for taking this on.  Another round of comments below.

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:37: NavigationControllerImpl*
navigation_controller,
nit: Goes on previous line.

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

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:43: // Initializes the frame tree with a
NavigationController which keeps
If I understand correctly, InterstitialPages are going to have a FrameTree and a
Navigator that doesn't allow navigation after the first page commits.  I suppose
they still need a NavigationController to let them commit their first
navigation?

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:44: // session history for it and
NavigationObserver which dispatches
nit: NavigatorDelegate.

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:47: NavigationControllerImpl*
navigation_controller,
nit: Should go on previous line.

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.h:97: Navigator* navigator() {
Do we need this here?  I'm thinking we'll want to expose the root FrameTreeNode
for several reasons (e.g., from my RFHM CL).

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.h:103: Navigator navigator_;
From our discussion, let's change this to have the same Navigator object for
every frame, making this a scoped_refptr to keep it alive as long as any
FrameTreeNode is using it.

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_delegate.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_delegate.h:77: // Actual
navigation delegates.
What does this mean?

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/navigator.cc:27: void
Navigator::DidStartProvisionalLoad(
Reminder to update this from the latest state of DidStartProvisionalLoad when
you're ready to commit.

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:28: int64 frame_id,
Do we need to take in the frame_id and parent_frame_id if we have the
render_frame_host?  (It's ok if that's just temporary, until we switch over to
using browser-global FrameTreeNode IDs and renderer-specific RFH routing IDs.)

https://codereview.chromium.org/26316005/diff/133001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1971:
DidChangeLoadProgress(0);
This used to be behind a is_main_frame check.  Is there a reason to change it?

https://codereview.chromium.org/26316005/diff/133001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:434: RenderFrameHost*
render_frame_host) OVERRIDE;
Since every one of these should take in the RFH, let's make it the first
parameter.

Any reason to pass RenderFrameHost instead of RenderFrameHostImpl?  I predict a
lot of casts in the future.

https://codereview.chromium.org/26316005/diff/133001/content/public/browser/r...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/26316005/diff/133001/content/public/browser/r...
content/public/browser/render_frame_host.h:23: virtual RenderProcessHost*
GetProcess() const = 0;
Do we need this?  You can get the process from the SiteInstance.

Actually, I'm not entirely clear why we need to pass RenderFrameHosts to
Navigator.  Is Navigator going to become public, or could it just use
RenderFrameHostImpl?

[nasko, 2013-11-21 19:03:32]

I think this is ready for a round of reviews.

[Charlie Reis, 2013-11-21 21:59:32]

I'm not sure about set_render_frame_host, but the rest seems like a good start.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:59:
root_->set_render_frame_host(scoped_ptr<RenderFrameHostImpl>());
I'm not entirely clear why this is needed.  It doesn't seem related to this CL,
and I haven't found anything that depends on it so far.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.h:45: void set_render_frame_host(
I'm a bit uncomfortable about decoupling this from owns_render_frame_host_.  Do
we need this method?

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/interstitial_page_impl.h:71: virtual void
DidStartProvisionalLoad(
We shouldn't need these.  See my comment in navigator_delegate.h.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.cc:41: LOG(ERROR) <<
"Navigator::DidStartProvisionalLoad: " << url;
Don't forget to remove the LOG statements.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:40: // TODO(nasko): The RenderFrameHost
parameter should be replaced with
nit: RenderFrameHostImpl

Actually, I'm not sure I agree with this TODO.  We'll need to know which
RenderFrameHostImpl sent the message even when we have a RFHM on each node, in
case there's a race and the message didn't come from our current RFH.  I think
we have to stick with this contract.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:41: // FrameTreeNode, once we have
RenderFrameHostManager for each node in the
nit: have a

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:42: // frame tree.
nit: FrameTree.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:16: virtual void
DidStartProvisionalLoad(
This is where these methods should be documented.

Also, we should probably remove their equivalents from RenderViewHostDelegate,
right?

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:23: bool is_iframe_srcdoc) = 0;
These should have default implementations that do nothing, so that we don't need
to define them in places like InterstitialPageImpl.  (There's a precedent for
that in places like web_contents_delegate.h.)

https://codereview.chromium.org/26316005/diff/293001/content/browser/renderer...
File content/browser/renderer_host/test_render_view_host.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/renderer...
content/browser/renderer_host/test_render_view_host.h:374: TestRenderFrameHost*
test_main_rfh();
nit: main_test_rfh()
(More consistent with pending_test_rvh)

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1988: void
WebContentsImpl::NotifyProvisionalChangeToMainFrameUrl(
Why did we need to split this part out into its own notify method?

(Note: I've been talking with Timo about possibly removing
ProvisionalChangeToMainFrameUrl, since it's just a combination of
DidStartProvisionalLoadForFrame and DidRedirectProvisionalLoad.)

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:437: const GURL& url) OVERRIDE;
What about NotifyNavigationStateChanged?

[nasko, 2013-11-22 01:02:33]

Fixes for all comments.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:59:
root_->set_render_frame_host(scoped_ptr<RenderFrameHostImpl>());
On 2013/11/21 21:59:32, creis wrote:
> I'm not entirely clear why this is needed.  It doesn't seem related to this
CL,
> and I haven't found anything that depends on it so far.

This is incorrect here, disregard.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/frame_tree_node.h:45: void set_render_frame_host(
On 2013/11/21 21:59:32, creis wrote:
> I'm a bit uncomfortable about decoupling this from owns_render_frame_host_. 
Do
> we need this method?

Yes, we do. Since RFH is created after FTN, until we have RFHM we need to hand
the RFH to FTN. The code before this CL got away with RFH being created before
FTN.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/interstitial_page_impl.h:71: virtual void
DidStartProvisionalLoad(
On 2013/11/21 21:59:32, creis wrote:
> We shouldn't need these.  See my comment in navigator_delegate.h.

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.cc:41: LOG(ERROR) <<
"Navigator::DidStartProvisionalLoad: " << url;
On 2013/11/21 21:59:32, creis wrote:
> Don't forget to remove the LOG statements.

Yeah, those won't go in : ).

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:40: // TODO(nasko): The RenderFrameHost
parameter should be replaced with
On 2013/11/21 21:59:32, creis wrote:
> nit: RenderFrameHostImpl
> 
> Actually, I'm not sure I agree with this TODO.  We'll need to know which
> RenderFrameHostImpl sent the message even when we have a RFHM on each node, in
> case there's a race and the message didn't come from our current RFH.  I think
> we have to stick with this contract.

The reason for the TODO is previous discussions about structuring the API to
look like navigator->NavigateAt(FrameTreeNode). I'm happy to drop it, since you
make a good point about the possibility of race conditions, which means we need
to either pass RFH or FTN+routing id. We can make this decision as we come
closer to the final picture we want.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:41: // FrameTreeNode, once we have
RenderFrameHostManager for each node in the
On 2013/11/21 21:59:32, creis wrote:
> nit: have a

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:42: // frame tree.
On 2013/11/21 21:59:32, creis wrote:
> nit: FrameTree.

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:16: virtual void
DidStartProvisionalLoad(
On 2013/11/21 21:59:32, creis wrote:
> This is where these methods should be documented.
> 
> Also, we should probably remove their equivalents from RenderViewHostDelegate,
> right?

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:23: bool is_iframe_srcdoc) = 0;
On 2013/11/21 21:59:32, creis wrote:
> These should have default implementations that do nothing, so that we don't
need
> to define them in places like InterstitialPageImpl.  (There's a precedent for
> that in places like web_contents_delegate.h.)

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/renderer...
File content/browser/renderer_host/test_render_view_host.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/renderer...
content/browser/renderer_host/test_render_view_host.h:374: TestRenderFrameHost*
test_main_rfh();
On 2013/11/21 21:59:32, creis wrote:
> nit: main_test_rfh()
> (More consistent with pending_test_rvh)

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1988: void
WebContentsImpl::NotifyProvisionalChangeToMainFrameUrl(
On 2013/11/21 21:59:32, creis wrote:
> Why did we need to split this part out into its own notify method?
> 
> (Note: I've been talking with Timo about possibly removing
> ProvisionalChangeToMainFrameUrl, since it's just a combination of
> DidStartProvisionalLoadForFrame and DidRedirectProvisionalLoad.)

Done.

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/293001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:437: const GURL& url) OVERRIDE;
On 2013/11/21 21:59:32, creis wrote:
> What about NotifyNavigationStateChanged?

Done.

[awong, 2013-11-22 02:35:50]

skimmed it and API change looks quite nice!  I can look in more detail next week
tuesday (or maybe monday depending on jetlag).

Only had some small suggestion as a comment.

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
content/browser/frame_host/navigator.cc:79: delegate_->DidStartProvisionalLoad(
Random thought (take it or leave it)...do we want the delegate to have exactly
the same function name as what we're enacting here?  Or do we want something
like OnDidStartProvisionLoad or DidStartProvisionLoadComplete or something?

Mostly trying to avoid namespace collisions.

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:33: 
nit: spurious newline.

https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:435: virtual void
NotifyChangedNavigationState(InvalidateTypes changed_flags)
Break after the opening (?  IIRC, the rule is to never orphan OVERRIDE or const
on their own line.

[awong, 2013-11-22 02:36:33]

oh... LGTM

On 2013/11/22 02:35:50, awong wrote:
> skimmed it and API change looks quite nice!  I can look in more detail next
week
> tuesday (or maybe monday depending on jetlag).
> 
> Only had some small suggestion as a comment.
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
> File content/browser/frame_host/navigator.cc (right):
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
> content/browser/frame_host/navigator.cc:79:
delegate_->DidStartProvisionalLoad(
> Random thought (take it or leave it)...do we want the delegate to have exactly
> the same function name as what we're enacting here?  Or do we want something
> like OnDidStartProvisionLoad or DidStartProvisionLoadComplete or something?
> 
> Mostly trying to avoid namespace collisions.
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
> File content/browser/frame_host/navigator_delegate.h (right):
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
> content/browser/frame_host/navigator_delegate.h:33: 
> nit: spurious newline.
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.h:435: virtual void
> NotifyChangedNavigationState(InvalidateTypes changed_flags)
> Break after the opening (?  IIRC, the rule is to never orphan OVERRIDE or
const
> on their own line.

[Charlie Reis, 2013-11-22 06:52:20]

LGTM!

[nasko, 2013-11-22 15:19:30]

Addressed Albert's comments.

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
File content/browser/frame_host/navigator.cc (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
content/browser/frame_host/navigator.cc:79: delegate_->DidStartProvisionalLoad(
On 2013/11/22 02:35:51, awong wrote:
> Random thought (take it or leave it)...do we want the delegate to have exactly
> the same function name as what we're enacting here?  Or do we want something
> like OnDidStartProvisionLoad or DidStartProvisionLoadComplete or something?
> 
> Mostly trying to avoid namespace collisions.

Those are different classes, so I'm not sure I understand the name collision
argument. One of the things that makes code reading harder for me in the current
codebase is that each class has a slight variation of the name, so not quite
sure which does what. I prefer to keep it this way.

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/frame_ho...
content/browser/frame_host/navigator_delegate.h:33: 
On 2013/11/22 02:35:51, awong wrote:
> nit: spurious newline.

Done.

https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/483001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.h:435: virtual void
NotifyChangedNavigationState(InvalidateTypes changed_flags)
On 2013/11/22 02:35:51, awong wrote:
> Break after the opening (?  IIRC, the rule is to never orphan OVERRIDE or
const
> on their own line.

This was formatted by clang-format, but I've changed it.

[commit-bot: I haz the power, 2013-11-22 15:19:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/26316005/813001

[commit-bot: I haz the power, 2013-11-22 15:36:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-22 16:50:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/26316005/813001

[commit-bot: I haz the power, 2013-11-22 17:11:54]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[nasko, 2013-12-10 00:36:21]

Charlie,
Can you take another look at this CL? It has changed slightly since the last
time you looked at it. Patch set 16 has important change.
Thanks,
Nasko

[Charlie Reis, 2013-12-10 01:20:50]

Didn't see any major issues, just a few things below.

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:46: // RenderFrameHostManager for
each node in the frame tree.
nit: once RenderFrameHosts are created by RenderFrameHostManager.

(We already have a RFHM for each FrameTreeNode.)

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2653:
contents()->GetFrameTree()->root()->navigator();
This feels a bit awkward to have to reach this far in.  Should WebContentsImpl
have a navigator() accessor?

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:17: #include
"content/public/browser/site_instance.h"
No longer needed?

https://codereview.chromium.org/26316005/diff/1133001/content/browser/rendere...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/rendere...
content/browser/renderer_host/DEPS:26:
"+content/browser/frame_host/test_render_frame_host.h",
I don't understand this rule.  test_render_view_host.h lives in content/test,
not in content/browser/renderer_host.

https://codereview.chromium.org/26316005/diff/1133001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:447: 
nit: No blank line here.

[nasko, 2013-12-10 01:36:49]

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:46: // RenderFrameHostManager for
each node in the frame tree.
On 2013/12/10 01:20:51, creis wrote:
> nit: once RenderFrameHosts are created by RenderFrameHostManager.
> 
> (We already have a RFHM for each FrameTreeNode.)

Done.

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2653:
contents()->GetFrameTree()->root()->navigator();
On 2013/12/10 01:20:51, creis wrote:
> This feels a bit awkward to have to reach this far in.  Should WebContentsImpl
> have a navigator() accessor?

I'd rather not expose an accessor for now. This will cause code to assume one
Navigator per WebContents, which at later in time will become wrong. Unless you
feel strongly about it, I would leave it as is or name it something very
obvious, such as GetMainFrameNavigator or similar.

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:17: #include
"content/public/browser/site_instance.h"
On 2013/12/10 01:20:51, creis wrote:
> No longer needed?

Done.

https://codereview.chromium.org/26316005/diff/1133001/content/browser/rendere...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/rendere...
content/browser/renderer_host/DEPS:26:
"+content/browser/frame_host/test_render_frame_host.h",
On 2013/12/10 01:20:51, creis wrote:
> I don't understand this rule.  test_render_view_host.h lives in content/test,
> not in content/browser/renderer_host.

The test file moved while I was working on this, so it is no longer an issue.
Thanks for the good catch!

https://codereview.chromium.org/26316005/diff/1133001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:447: 
On 2013/12/10 01:20:51, creis wrote:
> nit: No blank line here.

Done.

[Charlie Reis, 2013-12-10 01:40:07]

LGTM

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/26316005/diff/1133001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2653:
contents()->GetFrameTree()->root()->navigator();
On 2013/12/10 01:36:50, nasko wrote:
> On 2013/12/10 01:20:51, creis wrote:
> > This feels a bit awkward to have to reach this far in.  Should
WebContentsImpl
> > have a navigator() accessor?
> 
> I'd rather not expose an accessor for now. This will cause code to assume one
> Navigator per WebContents, which at later in time will become wrong. Unless
you
> feel strongly about it, I would leave it as is or name it something very
> obvious, such as GetMainFrameNavigator or similar.

That's a good point.

[commit-bot: I haz the power, 2013-12-10 01:41:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/26316005/1153001

[commit-bot: I haz the power, 2013-12-10 05:52:21]

Change committed as 239683