Move DidCommitProvisionalLoad code from RenderView to RenderFrame.

Move DidCommitProvisionalLoad code from RenderView to RenderFrame.

BUG=304341
R=creis@chromium.org

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

[nasko, 2014-02-04 21:00:44]

Hey Charlie,
This CL is finally ready for reviewing. It moves handling of
DidCommitProvisionalLoad from RenderView(Host) to RenderFrame(Host).
Thanks in advance for reviewing it!
Nasko

[Charlie Reis, 2014-02-05 23:30:37]

Wow, I'm really excited about this.  I've listed some things I found below, but
I'm looking forward to having this code on RenderFrame{Host}.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:96: // the meantime, make it
public, so it can be called directly.
nit: no comma after public

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.cc:22: // to the
interestitial page code.
nit: interstitial

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator.h:14: struct
FrameHostMsg_DidCommitProvisionalLoad_Params;
nit: Alphabetize

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator.h:59: // The RenderFrameHostImpl has
navigated.
Let's say "has committed a navigation."

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:15: struct
FrameHostMsg_DidCommitProvisionalLoad_Params;
nit: Alphabetize

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:47: // A commit was processed in
|render_frame_host|.
A navigation was committed in...

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:57: // committed to the
navigation controller. Note that the navigation entry is
nit: NavigationController, NavigationEntry

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:59: // current navigation entry
is in the NavigationController at this point.
What do you mean by "current" and "in the NavigationController?"  Maybe we
should say that the NavigationController's last committed entry is for this
navigation?

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:31: namespace {
Why not put this in the namespace below, alongside GetNavigationType, etc?

It's fairly simple and only used in one place so far, so we could also just put
it inline instead.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:399: frame_tree_node_id =
source_node->frame_tree_node_id();
I think we can get frame_tree_node_id from render_frame_host->frame_tree_node()
now, right?  That's probably true even in the default case (no flag).  Maybe we
should DCHECK_EQ(params.frame_id, frame_tree_node_id), but it seems like we can
simplify this code.

A TODO might suffice if you want to keep this as a move, but I could see fixing
it in this CL.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:418: frame_tree_node_id =
pending_entry->frame_tree_node_id();
This part is probably still necessary, unlike the FindByFrameID call above.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:449:
render_frame_host->GetSiteInstance())->HasSite() &&
Let's make this easier to read by grabbing site_instance into a local variable.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:456: // RenderView::UpdateURL does
not set params.contents_mime_type.
Please preserve the rest of jungshik's comment.  (I also think it would be
cleaner to have this code in the PageTransitionIsMainFrame block a few lines up,
near TakeScreenshot, but perhaps that can be done in a follow-up CL.)

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

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:251: if
(!render_view_host_->CanCommitURL(validated_params.url)) {
Let's move CanCommitURL to RFH rather than calling it on RVH.  Also, I'm not
sure why I made it public on RVH-- perhaps it should be private.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:101: friend class
TestRenderFrameHost;
nit: Alphabetize.

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1437: // Called when the
renderer navigates.  For every frame loaded, we'll get this
We can remove this comment now that it's in RFH.

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:600: friend class
RenderFrameHostImpl;
nit: Please move above line 593.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2549: bool
WebContentsImpl::ShouldAssignSiteForURL(const GURL& url) {
Let's delete this now that you've moved it to NavigatorImpl.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2112:
view_->SetOverscrollControllerEnabled(delegate_->CanOverscrollContent());
Whew, this code is tricky.  Yes, I think moving this here is equivalent.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2858:
frame_tree_.root()->navigator()->DidNavigate(rfh, orig_params);
If we're just keeping this for TestWebContents::TestDidNavigateWithReferrer, can
we move this code there and remove WebContentsImpl::DidNavigate entirely?

https://codereview.chromium.org/135723003/diff/510001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/common/view_mes...
content/common/view_messages.h:26: #include
"content/public/common/frame_navigate_params.h"
Can this be removed now?

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1764: // Tell the embedding application
that the URL of the active page has changed
nit: End with period.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1788: if (extra_data) {
nit: No braces.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1811: // Make navigation state a part of
the FrameNavigate message so that commited
nit: FrameNavigate -> DidCommitProvisionalLoad
nit: the committed entry has

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1925: // TODO(creis): This whole method
should move to RenderFrame.
You can remove this TODO now.  :)  Also, let's use our own is_swapped_out()
method below rather than looking up this RenderFrame using FromWebFrame.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_view_impl.cc:3275: // TODO(nasko): Transition this code
to RenderFrameImpl.
Is anything blocking us from moving this code over as well?  Seems pretty
similar to the rest of the function.

https://codereview.chromium.org/135723003/diff/510001/content/test/test_rende...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/test/test_rende...
content/test/test_render_view_host.h:240: virtual void SendNavigate(int page_id,
const GURL& url) OVERRIDE;
Can you add a note saying these navigation methods are deprecated and will be
removed in favor of their counterparts in TestRenderFrameHost?  (We don't need
to keep them long term, right?)

In the meantime, is it possible to make them forward to the implementations in
TestRenderFrameHost rather than duplicating the code?

[nasko, 2014-02-06 01:55:13]

I've fixed all comments but two. The remaining part is fixing up the unit tests
(WebContentsImpl::DidNavigate and TestRenderFrameHost).

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:96: // the meantime, make it
public, so it can be called directly.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: no comma after public

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.cc:22: // to the
interestitial page code.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: interstitial

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator.h:14: struct
FrameHostMsg_DidCommitProvisionalLoad_Params;
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: Alphabetize

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator.h:59: // The RenderFrameHostImpl has
navigated.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Let's say "has committed a navigation."

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:15: struct
FrameHostMsg_DidCommitProvisionalLoad_Params;
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: Alphabetize

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:47: // A commit was processed in
|render_frame_host|.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> A navigation was committed in...

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:57: // committed to the
navigation controller. Note that the navigation entry is
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: NavigationController, NavigationEntry

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:59: // current navigation entry
is in the NavigationController at this point.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> What do you mean by "current" and "in the NavigationController?"  Maybe we
> should say that the NavigationController's last committed entry is for this
> navigation?

This is a comment that was just moved from WebContentsImpl.h, but I agree that
last committed entry makes more sense.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:31: namespace {
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Why not put this in the namespace below, alongside GetNavigationType, etc?
> 
> It's fairly simple and only used in one place so far, so we could also just
put
> it inline instead.

Moved to the namespace below. I think we will have more callsites for this, so
I'm going to keep it. It is one liner but makes readability of the code better,
since it avoids overly long lines.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:399: frame_tree_node_id =
source_node->frame_tree_node_id();
On 2014/02/05 23:30:37, Charlie Reis wrote:
> I think we can get frame_tree_node_id from
render_frame_host->frame_tree_node()
> now, right?  That's probably true even in the default case (no flag).  Maybe
we
> should DCHECK_EQ(params.frame_id, frame_tree_node_id), but it seems like we
can
> simplify this code.

I agree with the simpler code, so fixed. I don't think we can do the DCHECK in
this form though, as params.frame_id is the renderer allocated one and
frame_tree_node_id is the browser stable one.

> A TODO might suffice if you want to keep this as a move, but I could see
fixing
> it in this CL.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:418: frame_tree_node_id =
pending_entry->frame_tree_node_id();
On 2014/02/05 23:30:37, Charlie Reis wrote:
> This part is probably still necessary, unlike the FindByFrameID call above.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:449:
render_frame_host->GetSiteInstance())->HasSite() &&
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Let's make this easier to read by grabbing site_instance into a local
variable.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:456: // RenderView::UpdateURL does
not set params.contents_mime_type.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Please preserve the rest of jungshik's comment.  (I also think it would be
> cleaner to have this code in the PageTransitionIsMainFrame block a few lines
up,
> near TakeScreenshot, but perhaps that can be done in a follow-up CL.)

Done.

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

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:251: if
(!render_view_host_->CanCommitURL(validated_params.url)) {
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Let's move CanCommitURL to RFH rather than calling it on RVH.  Also, I'm not
> sure why I made it public on RVH-- perhaps it should be private.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:101: friend class
TestRenderFrameHost;
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: Alphabetize.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1437: // Called when the
renderer navigates.  For every frame loaded, we'll get this
On 2014/02/05 23:30:37, Charlie Reis wrote:
> We can remove this comment now that it's in RFH.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.h:600: friend class
RenderFrameHostImpl;
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: Please move above line 593.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2549: bool
WebContentsImpl::ShouldAssignSiteForURL(const GURL& url) {
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Let's delete this now that you've moved it to NavigatorImpl.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2112:
view_->SetOverscrollControllerEnabled(delegate_->CanOverscrollContent());
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Whew, this code is tricky.  Yes, I think moving this here is equivalent.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/common/view_mes...
content/common/view_messages.h:26: #include
"content/public/common/frame_navigate_params.h"
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Can this be removed now?

It required adding the header in other files, though it is now gone from here!

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1764: // Tell the embedding application
that the URL of the active page has changed
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1788: if (extra_data) {
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: No braces.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1811: // Make navigation state a part of
the FrameNavigate message so that commited
On 2014/02/05 23:30:37, Charlie Reis wrote:
> nit: FrameNavigate -> DidCommitProvisionalLoad
> nit: the committed entry has

Done.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_frame_impl.cc:1925: // TODO(creis): This whole method
should move to RenderFrame.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> You can remove this TODO now.  :)  Also, let's use our own is_swapped_out()
> method below rather than looking up this RenderFrame using FromWebFrame.

Done.

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/renderer/render...
content/renderer/render_view_impl.cc:3275: // TODO(nasko): Transition this code
to RenderFrameImpl.
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Is anything blocking us from moving this code over as well?  Seems pretty
> similar to the rest of the function.

The main thing is that I didn't want to add even more files to churn to an
already big CL. If you feel it belongs in this CL, I'll add it.

[nasko, 2014-02-06 20:07:59]

The last remaining issues should be now resolved.

Ready for another look.

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2858:
frame_tree_.root()->navigator()->DidNavigate(rfh, orig_params);
On 2014/02/05 23:30:37, Charlie Reis wrote:
> If we're just keeping this for TestWebContents::TestDidNavigateWithReferrer,
can
> we move this code there and remove WebContentsImpl::DidNavigate entirely?

Done.

https://codereview.chromium.org/135723003/diff/510001/content/test/test_rende...
File content/test/test_render_view_host.h (right):

https://codereview.chromium.org/135723003/diff/510001/content/test/test_rende...
content/test/test_render_view_host.h:240: virtual void SendNavigate(int page_id,
const GURL& url) OVERRIDE;
On 2014/02/05 23:30:37, Charlie Reis wrote:
> Can you add a note saying these navigation methods are deprecated and will be
> removed in favor of their counterparts in TestRenderFrameHost?  (We don't need
> to keep them long term, right?)
> 
> In the meantime, is it possible to make them forward to the implementations in
> TestRenderFrameHost rather than duplicating the code?

Done.

[Charlie Reis, 2014-02-06 21:49:06]

Fantastic.  LGTM with the nits below.

Great to see navigation moving over.  Keep 'em coming!

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/510001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:399: frame_tree_node_id =
source_node->frame_tree_node_id();
On 2014/02/06 01:55:13, nasko wrote:
> On 2014/02/05 23:30:37, Charlie Reis wrote:
> > I think we can get frame_tree_node_id from
> render_frame_host->frame_tree_node()
> > now, right?  That's probably true even in the default case (no flag).  Maybe
> we
> > should DCHECK_EQ(params.frame_id, frame_tree_node_id), but it seems like we
> can
> > simplify this code.
> 
> I agree with the simpler code, so fixed. I don't think we can do the DCHECK in
> this form though, as params.frame_id is the renderer allocated one and
> frame_tree_node_id is the browser stable one.
> 
> > A TODO might suffice if you want to keep this as a move, but I could see
> fixing
> > it in this CL.
> 

Ah, good point.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:94: //
RenderViewHostDelegate implementation:
We can remove this line, since it's no longer in RVHDelegate.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator.h:59: // The RenderFrameHostImpl has has
committed a navigation.
nit: has has

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:59: // NavigationController last
committed entry is for this navigation.
nit: NavigationController's

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:373: const
FrameHostMsg_DidCommitProvisionalLoad_Params& input_params) {
nit: Wrong indent (2 spaces too many)

https://codereview.chromium.org/135723003/diff/780001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1441: // action.
The rest of this comment can be removed as well.

[nasko, 2014-02-06 21:58:43]

Fixed nits.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_impl.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_impl.h:94: //
RenderViewHostDelegate implementation:
On 2014/02/06 21:49:07, Charlie Reis wrote:
> We can remove this line, since it's no longer in RVHDelegate.

Done.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator.h:59: // The RenderFrameHostImpl has has
committed a navigation.
On 2014/02/06 21:49:07, Charlie Reis wrote:
> nit: has has

Done.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:59: // NavigationController last
committed entry is for this navigation.
On 2014/02/06 21:49:07, Charlie Reis wrote:
> nit: NavigationController's

Done.

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:373: const
FrameHostMsg_DidCommitProvisionalLoad_Params& input_params) {
On 2014/02/06 21:49:07, Charlie Reis wrote:
> nit: Wrong indent (2 spaces too many)

Done.

https://codereview.chromium.org/135723003/diff/780001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/135723003/diff/780001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1441: // action.
On 2014/02/06 21:49:07, Charlie Reis wrote:
> The rest of this comment can be removed as well.

Done.

[nasko, 2014-02-06 22:10:59]

Committed patchset #12 manually as r249516 (presubmit successful).