PlzNavigate: track pending commits

PlzNavigate: track pending commits

This CL makes it so that RenderFrameHostManager::GetFramehostForNavigation will
no longer rely on cross_navigation_pending_ when browser-side navigation is enabled. Instead, we will track if commits are pending either in the current or the speculative RFH.

BUG=439423
Committed: https://crrev.com/5bf36119961044dd6c01b16310862bfceb080058
Cr-Commit-Position: refs/heads/master@{#325639}

[clamy, 2015-03-30 12:49:25]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, nasko@chromium.org

[clamy, 2015-03-30 12:50:57]

@nasko, carlosk: PTAL

This makes it so that we only set cross_navigation_pending to true when the
navigation is ready to commit. Apparently all tests but one were fine with that,
and it seems to be a more appropriate use of the boolean.

https://codereview.chromium.org/1048463004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_unittest.cc:2663:
contents->GetMainFrame()->PrepareForCommit();
Decoupling the CommitPendingNavigation in two was necessary to get to the state
where we are sure to have a cross-site navigation pending in PlzNavigate.

[nasko, 2015-03-30 22:35:22]

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:832: if (for_commit)
Can't this be replaced by an earlier check for existence of speculative RFH?
Alternatively, it is called from only two places, the one passing in false
already is resetting it to false after this call.
It will be ideal if we don't have to add this new parameter to the method.

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.h:392: // owned by this
manager. ¬for_commit¬ is true if it is the final call in the
nit: The characters around "for_commit" aren't something that displays correctly
here. Are you using quotes or pipes for it?

[clamy, 2015-03-31 12:57:47]

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

[clamy, 2015-03-31 12:58:18]

Thanks! Adding creis@ since nasko@ is ooo.

@creis: PTAL.

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:832: if (for_commit)
On 2015/03/30 22:35:22, nasko (out until 04-13) wrote:
> Can't this be replaced by an earlier check for existence of speculative RFH?
> Alternatively, it is called from only two places, the one passing in false
> already is resetting it to false after this call.
> It will be ideal if we don't have to add this new parameter to the method.

I'm now resetting it to false in BeginNavigation (the check on the speculative
RFH would not cover all possible cases).

[carlosk, 2015-03-31 13:01:32]

Thanks.

https://chromiumcodereview.appspot.com/1048463004/diff/1/content/browser/fram...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1048463004/diff/1/content/browser/fram...
content/browser/frame_host/render_frame_host_manager.cc:832: if (for_commit)
On 2015/03/30 22:35:22, nasko (out until 04-13) wrote:
> Can't this be replaced by an earlier check for existence of speculative RFH?
> Alternatively, it is called from only two places, the one passing in false
> already is resetting it to false after this call.
> It will be ideal if we don't have to add this new parameter to the method.

I think the explicit parameter is a better choice than the implicit knowledge
that this method will only be called twice. And even if relying on this fact I
don't think there's a way to detect if |cross_navigation_pending_| should be
true or false without adding an extra member variable (bool
has_already_been_called).

So IMO the problem is in fact that BeginNavigation is also setting  to false.

In this case, there's a nit: instead of the if-check here, this could be
replaced by having this line:
cross_navigation_pending_ = for_commit;
inside the if-block above where the speculative RFH is created.

https://chromiumcodereview.appspot.com/1048463004/diff/1/content/browser/web_...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/1048463004/diff/1/content/browser/web_...
content/browser/web_contents/web_contents_impl_unittest.cc:2663:
contents->GetMainFrame()->PrepareForCommit();
On 2015/03/30 12:50:57, clamy wrote:
> Decoupling the CommitPendingNavigation in two was necessary to get to the
state
> where we are sure to have a cross-site navigation pending in PlzNavigate.

To make sure I understand what you mean: you are explaining why the
if-PlzNavigate-block here is still needed, right?

[nasko, 2015-03-31 14:56:12]

I dug a bit more into the usage of cross_navigation_pending_ and it is never
used for anything meaningful in PlzNavigate. As such, I think that PlzNavigate
paths should just leave that variable alone and never touch it.

What initiated this discussion was the desire to send Stop IPC message to the
speculative RFH if it has sent the Commit message to the renderer process, but
hasn't received a DidCommitProvisionalLoad message yet. This interval of time
should probably be tracked by an independent boolean, which is PlzNavigate
specific. This will allow cleaner delineation between the two navigation
codepaths and can allow us to enforce invariants through DCHECKs.

[clamy, 2015-03-31 15:02:16]

The problem there is that all unit tests that check cross_navigation_pending_
will start failing when PlzNavigate is enabled if we don't set it.

That said, I think it would be good to track that a commit is pending, either in
the current RFH or the speculative one. We're having issues with subtle edge
cases there, and that could probably help.

[Charlie Reis, 2015-03-31 23:13:55]

On 2015/03/31 15:02:16, clamy wrote:
> The problem there is that all unit tests that check cross_navigation_pending_
> will start failing when PlzNavigate is enabled if we don't set it.
> 
> That said, I think it would be good to track that a commit is pending, either
in
> the current RFH or the speculative one. We're having issues with subtle edge
> cases there, and that could probably help.

Yes, tracking whether there's a commit pending seems reasonable to me.

For reference, cross_navigation_pending_ looks like something I should remove.  
1) In non-PlzNavigate code, it corresponds exactly to pending_render_frame_host_
being non-null.
2) In PlzNavigate, the comment on its declaration says it's not used, so
hopefully we can remove the places that set it.
3) In test code, we can change TestWebContents::cross_navigation_pending() to
return whether pending_render_frame_host_ is non-null.  In PlzNavigate, maybe we
can implement it with the new thing you're discussing?

Don't feel the need to do that cleanup in this CL, but I would recommend not
relying on cross_navigation_pending_ in PlzNavigate if possible (so that I can
remove it).

https://codereview.chromium.org/1048463004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:2667:
contents->GetPendingMainFrame()->SendNavigate(0, kUrl);
Let's avoid using 0 for the page ID, since that tends to cause future tests to
fail.  (We're having to clean up similar stuff as we go.)

[clamy, 2015-04-02 12:52:36]

Thanks! We now no longer set/unset cross_navigation_pending_ in PlzNavigate.
Instead I introduced two booleans that track whether we have a commit pending in
the current/speculative RFH. I also added DCHECKs on commit to check that the
commit was expected. This caused a whole lot of tests to fail when ran with
PlzNavigate enabled, so I had to add the relevant helper functions that were
missing from them.

https://codereview.chromium.org/1048463004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:2667:
contents->GetPendingMainFrame()->SendNavigate(0, kUrl);
On 2015/03/31 23:13:55, Charlie Reis wrote:
> Let's avoid using 0 for the page ID, since that tends to cause future tests to
> fail.  (We're having to clean up similar stuff as we go.)

Done.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3030: //
TODO(nasko): Until OnNavigate is moved to RenderFrameHost, we need
I removed this TODO as it seemed no longer relevant.

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

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1449:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
While I'm modifying these lines, do you want me to ensure that we don't send a
value of 0 for page_ids? I think on a previous CL you asked me to avoid doing
that.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:393: // is assured to
commit a navigation and is no longer speculative.
I reintroduced the |is_for_commit| boolean, as it is not possible to set
pending_commit_current_frame_ to false after a call to
GetFrameHostForNavigation. While a commit in the speculative RFH will have been
canceled by destroying the RFH, this is not the case for a pending commit in the
current RFH.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:711: bool
pending_commit_current_frame_;
With some sufficiently bad interleaving of requests it is possible to have
commits pending in both the current and speculative RFH, hence the use of two
different booleans.

https://codereview.chromium.org/1048463004/diff/80001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/1048463004/diff/80001/content/test/test_rende...
content/test/test_render_frame_host.cc:239: if (IsWaitingForBeforeUnloadACK())
This is necessary if PrepareForCommit is called on a subframe.

[Charlie Reis, 2015-04-03 06:02:59]

Thanks for moving away from cross_navigation_pending_.  I'm looking forward to
removing that.

Some questions about the new booleans below.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (left):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3030: //
TODO(nasko): Until OnNavigate is moved to RenderFrameHost, we need
On 2015/04/02 12:52:35, clamy wrote:
> I removed this TODO as it seemed no longer relevant.

Acknowledged.

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

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:712:
main_test_rfh()->PrepareForCommit();
The changes in this file were just because of the new DCHECKs, right?

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1449:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, url1);
On 2015/04/02 12:52:35, clamy wrote:
> While I'm modifying these lines, do you want me to ensure that we don't send a
> value of 0 for page_ids? I think on a previous CL you asked me to avoid doing
> that.

No need.  Let's just make sure that new code does the right thing.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:468:
DCHECK(pending_commit_current_frame_);
This appears to be the only place that pending_commit_current_frame_ is used.  I
don't see the benefit of tracking it solely for a DCHECK here.  It's better to
avoid adding state when we can, since it's easy for it to become wrong due to
races, etc.  Please remove it.

(I'd like to say the same about pending_commit_speculative_frame_, but I see the
benefit to being able to check it while testing.  Would it work to create a
pending_commit_ on TestRenderFrameHost and manage it solely in test code
instead?  That might let you enforce both of these DCHECKs when
TestRenderFrameHost commits.)

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:475:
pending_commit_current_frame_ = false;
What if pending_commit_speculative_frame_ was true?  It's not clear why one is
reset here and not the other.

(You can ignore this if you remove this field.)

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:838: if (is_for_commit)
{
This is only used at the end of the method and doesn't affect the execution of
the code before it.  That means we could put it in a separate method that is
called afterward in the case of commit, rather than passing a boolean to turn
this piece on or off.

I'd much prefer that, since it reduces the number of tasks this method is
responsible for and makes it easier to understand.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:854:
pending_commit_speculative_frame_ = false;
This is where your comment about it being impossible to cancel
pending_commit_current_frame_ would belong, but I'd rather remove that field
entirely.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:393: // is assured to
commit a navigation and is no longer speculative.
On 2015/04/02 12:52:35, clamy wrote:
> I reintroduced the |is_for_commit| boolean, as it is not possible to set
> pending_commit_current_frame_ to false after a call to
> GetFrameHostForNavigation. While a commit in the speculative RFH will have
been
> canceled by destroying the RFH, this is not the case for a pending commit in
the
> current RFH.

I don't see how that makes this boolean necessary, though.  See my comments in
the .cc file.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:626: // PlzNavigate: not
used.
Great!  I'll plan to remove it after your CL lands.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:710: // handle it, but
before the renderer committed the provisional load,
nit: has committed
nit: end with period.

[clamy, 2015-04-07 16:29:21]

Thanks! I moved the state tracking to RenderFrameHost, which should make it
clearer.

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

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:712:
main_test_rfh()->PrepareForCommit();
On 2015/04/03 06:02:58, Charlie Reis wrote:
> The changes in this file were just because of the new DCHECKs, right?

Yes, because the DCHECKs exposed that we were not using helper functions when we
should have been.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:468:
DCHECK(pending_commit_current_frame_);
On 2015/04/03 06:02:58, Charlie Reis wrote:
> This appears to be the only place that pending_commit_current_frame_ is used. 
I
> don't see the benefit of tracking it solely for a DCHECK here.  It's better to
> avoid adding state when we can, since it's easy for it to become wrong due to
> races, etc.  Please remove it.
> 
> (I'd like to say the same about pending_commit_speculative_frame_, but I see
the
> benefit to being able to check it while testing.  Would it work to create a
> pending_commit_ on TestRenderFrameHost and manage it solely in test code
> instead?  That might let you enforce both of these DCHECKs when
> TestRenderFrameHost commits.)

So I ended up moving the state tracking to RenderFrameHostImpl (rather than
TestRenderFrameHost), for two reasons:
1) I think it is clearer when we set it.
2) This allow us to do some checking in browser tests as well.

Note that I don't mind moving it to TestRenderFrameHost when we get closer to
launching, but I think that having DCHECKS in place for tracking whether commits
were expected would help us in the development process.

[Charlie Reis, 2015-04-07 17:04:48]

Thanks.  I've only looked at RFHI so far, but a few questions there.

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:468:
DCHECK(pending_commit_current_frame_);
On 2015/04/07 16:29:21, clamy wrote:
> On 2015/04/03 06:02:58, Charlie Reis wrote:
> > This appears to be the only place that pending_commit_current_frame_ is
used. 
> I
> > don't see the benefit of tracking it solely for a DCHECK here.  It's better
to
> > avoid adding state when we can, since it's easy for it to become wrong due
to
> > races, etc.  Please remove it.
> > 
> > (I'd like to say the same about pending_commit_speculative_frame_, but I see
> the
> > benefit to being able to check it while testing.  Would it work to create a
> > pending_commit_ on TestRenderFrameHost and manage it solely in test code
> > instead?  That might let you enforce both of these DCHECKs when
> > TestRenderFrameHost commits.)
> 
> So I ended up moving the state tracking to RenderFrameHostImpl (rather than
> TestRenderFrameHost), for two reasons:
> 1) I think it is clearer when we set it.
> 2) This allow us to do some checking in browser tests as well.
> 
> Note that I don't mind moving it to TestRenderFrameHost when we get closer to
> launching, but I think that having DCHECKS in place for tracking whether
commits
> were expected would help us in the development process.

Hmm, ok.  It would be nice to not need this in shipping builds of Chrome, but I
don't see a good way to pull that off.  Not even #if defined(UNIT_TEST) can help
us there.  And I agree that having the DCHECK coverage in browser_tests is good,
so TestRFH won't help.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
This doesn't seem right to me.

If the renderer process does a in-page navigation (fragment, pushState, etc),
we'll get a DidCommitProvisionalLoad without going through CommitNavigation,
right?

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
I don't understand how this would happen.  When would a frame get swapped out
after a call to CommitNavigation?

[clamy, 2015-04-08 11:56:23]

Thanks!

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
On 2015/04/07 17:04:47, Charlie Reis wrote:
> This doesn't seem right to me.
> 
> If the renderer process does a in-page navigation (fragment, pushState, etc),
> we'll get a DidCommitProvisionalLoad without going through CommitNavigation,
> right?

I think we want to inform the browser that we're doing a synchronous navigation
first (through BeginNavigation). This allows to cancel an ongoing navigation if
needed. Then the Navigator can see we don't need to go through the network stack
and inform the appropriate RFH to expect a commit.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
On 2015/04/07 17:04:47, Charlie Reis wrote:
> I don't understand how this would happen.  When would a frame get swapped out
> after a call to CommitNavigation?

If the current RFH was supposed to commit a navigation, but for some reason it
took a lot of time, which allowed a new cross-site navigation to commit in the
meantime. Then the current RFH gets swapped out before sending
DidCommitProvisionalLoad.

[carlosk, 2015-04-08 16:35:20]

The change does look better with the responsibility being moved to the RFH. But
I would really like if at some point with PlzNavigate we could unambiguously
track each navigation (ID or unique communication chain or something) so that we
don't have to worry anymore about what is the right RFH to commit.

https://chromiumcodereview.appspot.com/1048463004/diff/120001/content/browser...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/1048463004/diff/120001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:752:
main_test_rfh()->SendNavigate(0, kExistingURL1);
For these cases (here and elsewhere) where a complete browser-initiated
navigation is made without any checks in between the 3 steps, to simplify things
we could also replace them with  TestWebContents::NavigateAndCommit.

Might be something for a following patch. WDYT?

https://chromiumcodereview.appspot.com/1048463004/diff/120001/content/browser...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2419:
contents()->GetMainFrame()->SendNavigateWithParams(&params);
In the same line as my previous comment, it seems these 3 calls could be
replaced with the new NavigateAndCommitRendererInitiated. This might be the case
in other places as well.

[Charlie Reis, 2015-04-08 16:45:27]

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
On 2015/04/08 11:56:23, clamy wrote:
> On 2015/04/07 17:04:47, Charlie Reis wrote:
> > This doesn't seem right to me.
> > 
> > If the renderer process does a in-page navigation (fragment, pushState,
etc),
> > we'll get a DidCommitProvisionalLoad without going through CommitNavigation,
> > right?
> 
> I think we want to inform the browser that we're doing a synchronous
navigation
> first (through BeginNavigation). This allows to cancel an ongoing navigation
if
> needed. Then the Navigator can see we don't need to go through the network
stack
> and inform the appropriate RFH to expect a commit.

Ok, so the renderer will tell the browser via BeginNavigation, then later in the
same task commit the navigation via DidCommit.  That part seems ok.

However, it doesn't look like you're setting pending_commit_ to be true in that
case.  NavigatorImpl::BeginNavigation will get false from
NavigationRequest::BeginNavigation if it's a pushState, etc, and it doesn't tell
the current RFH to expect the commit.  It's only set to true in
CommitNavigation, which doesn't look like it's called here.  Am I missing
something?

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
On 2015/04/08 11:56:23, clamy wrote:
> On 2015/04/07 17:04:47, Charlie Reis wrote:
> > I don't understand how this would happen.  When would a frame get swapped
out
> > after a call to CommitNavigation?
> 
> If the current RFH was supposed to commit a navigation, but for some reason it
> took a lot of time, which allowed a new cross-site navigation to commit in the
> meantime. Then the current RFH gets swapped out before sending
> DidCommitProvisionalLoad.

I thought that the DidCommitProvisionalLoad IPC was sent in the same task as
receiving the OnCommitNavigation IPC, right?  That means that the renderer
process won't have a chance to see the FrameMsg_SwapOut request before sending
the DidCommit.  I think this will cause your DCHECK in DidCommit to fail.

(Looking closer, I see that there's an early return in
RenderFrameImpl::OnCommitNavigation if PrepareRenderViewForNavigation returns
false, but it's not sending a DidDropNavigation IPC the way we do in default
Chrome.  That seems potentially buggy-- don't we need to stop the spinner and
clean up the pending state if we ignore the navigation as stale?  We should
probably clear pending_commit_ in that case as well.)

It sounds like we probably want a test for this case, since it's not clear
whether it works.

[clamy, 2015-04-10 14:40:15]

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:752:
main_test_rfh()->SendNavigate(0, kExistingURL1);
On 2015/04/08 16:35:20, carlosk wrote:
> For these cases (here and elsewhere) where a complete browser-initiated
> navigation is made without any checks in between the 3 steps, to simplify
things
> we could also replace them with  TestWebContents::NavigateAndCommit.
> 
> Might be something for a following patch. WDYT?

Well, I won't do it in this patch which is already big enough, and I don't think
it is of very high priority.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2419:
contents()->GetMainFrame()->SendNavigateWithParams(&params);
On 2015/04/08 16:35:20, carlosk wrote:
> In the same line as my previous comment, it seems these 3 calls could be
> replaced with the new NavigateAndCommitRendererInitiated. This might be the
case
> in other places as well.

No because we call SendNavigateWithParams and not SendNavigate, and I don't want
to define a TestWebContents::NavigateAndCommitRendererInitiatedWithparams.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/08 11:56:23, clamy wrote:
> > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > This doesn't seem right to me.
> > > 
> > > If the renderer process does a in-page navigation (fragment, pushState,
> etc),
> > > we'll get a DidCommitProvisionalLoad without going through
CommitNavigation,
> > > right?
> > 
> > I think we want to inform the browser that we're doing a synchronous
> navigation
> > first (through BeginNavigation). This allows to cancel an ongoing navigation
> if
> > needed. Then the Navigator can see we don't need to go through the network
> stack
> > and inform the appropriate RFH to expect a commit.
> 
> Ok, so the renderer will tell the browser via BeginNavigation, then later in
the
> same task commit the navigation via DidCommit.  That part seems ok.
> 
> However, it doesn't look like you're setting pending_commit_ to be true in
that
> case.  NavigatorImpl::BeginNavigation will get false from
> NavigationRequest::BeginNavigation if it's a pushState, etc, and it doesn't
tell
> the current RFH to expect the commit.  It's only set to true in
> CommitNavigation, which doesn't look like it's called here.  Am I missing
> something?

No that it is not implemented yet. We do not currently support same-site
navigations properly.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/08 11:56:23, clamy wrote:
> > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > I don't understand how this would happen.  When would a frame get swapped
> out
> > > after a call to CommitNavigation?
> > 
> > If the current RFH was supposed to commit a navigation, but for some reason
it
> > took a lot of time, which allowed a new cross-site navigation to commit in
the
> > meantime. Then the current RFH gets swapped out before sending
> > DidCommitProvisionalLoad.
> 
> I thought that the DidCommitProvisionalLoad IPC was sent in the same task as
> receiving the OnCommitNavigation IPC, right?  That means that the renderer
> process won't have a chance to see the FrameMsg_SwapOut request before sending
> the DidCommit.  I think this will cause your DCHECK in DidCommit to fail.
> 
> (Looking closer, I see that there's an early return in
> RenderFrameImpl::OnCommitNavigation if PrepareRenderViewForNavigation returns
> false, but it's not sending a DidDropNavigation IPC the way we do in default
> Chrome.  That seems potentially buggy-- don't we need to stop the spinner and
> clean up the pending state if we ignore the navigation as stale?  We should
> probably clear pending_commit_ in that case as well.)
> 
> It sounds like we probably want a test for this case, since it's not clear
> whether it works.

No the DidCommitProvisionalLoad IPC is sent when the renderer gets data from the
browser from the stream request, which does not happen synchronously.

(Indeed we noticed that as well when trying to get PlzNavigate to send the
proper start/stop loading notifications. Fabrice is working on making the
start/stop loading work with PlzNavigate, so I expect him to fix that in a later
patch).

[Charlie Reis, 2015-04-10 23:04:01]

Thanks.  One nit and one more question about failing to commit.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
On 2015/04/10 14:40:15, clamy wrote:
> On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> > On 2015/04/08 11:56:23, clamy wrote:
> > > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > > This doesn't seem right to me.
> > > > 
> > > > If the renderer process does a in-page navigation (fragment, pushState,
> > etc),
> > > > we'll get a DidCommitProvisionalLoad without going through
> CommitNavigation,
> > > > right?
> > > 
> > > I think we want to inform the browser that we're doing a synchronous
> > navigation
> > > first (through BeginNavigation). This allows to cancel an ongoing
navigation
> > if
> > > needed. Then the Navigator can see we don't need to go through the network
> > stack
> > > and inform the appropriate RFH to expect a commit.
> > 
> > Ok, so the renderer will tell the browser via BeginNavigation, then later in
> the
> > same task commit the navigation via DidCommit.  That part seems ok.
> > 
> > However, it doesn't look like you're setting pending_commit_ to be true in
> that
> > case.  NavigatorImpl::BeginNavigation will get false from
> > NavigationRequest::BeginNavigation if it's a pushState, etc, and it doesn't
> tell
> > the current RFH to expect the commit.  It's only set to true in
> > CommitNavigation, which doesn't look like it's called here.  Am I missing
> > something?
> 
> No that it is not implemented yet. We do not currently support same-site
> navigations properly.

Ok.  If you're expecting this check to fail, please add a TODO here to fix it
(mentioning why it would fail).

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
On 2015/04/10 14:40:15, clamy wrote:
> On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> > On 2015/04/08 11:56:23, clamy wrote:
> > > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > > I don't understand how this would happen.  When would a frame get
swapped
> > out
> > > > after a call to CommitNavigation?
> > > 
> > > If the current RFH was supposed to commit a navigation, but for some
reason
> it
> > > took a lot of time, which allowed a new cross-site navigation to commit in
> the
> > > meantime. Then the current RFH gets swapped out before sending
> > > DidCommitProvisionalLoad.
> > 
> > I thought that the DidCommitProvisionalLoad IPC was sent in the same task as
> > receiving the OnCommitNavigation IPC, right?  That means that the renderer
> > process won't have a chance to see the FrameMsg_SwapOut request before
sending
> > the DidCommit.  I think this will cause your DCHECK in DidCommit to fail.
> > 
> > (Looking closer, I see that there's an early return in
> > RenderFrameImpl::OnCommitNavigation if PrepareRenderViewForNavigation
returns
> > false, but it's not sending a DidDropNavigation IPC the way we do in default
> > Chrome.  That seems potentially buggy-- don't we need to stop the spinner
and
> > clean up the pending state if we ignore the navigation as stale?  We should
> > probably clear pending_commit_ in that case as well.)
> > 
> > It sounds like we probably want a test for this case, since it's not clear
> > whether it works.
> 
> No the DidCommitProvisionalLoad IPC is sent when the renderer gets data from
the
> browser from the stream request, which does not happen synchronously.
> 
> (Indeed we noticed that as well when trying to get PlzNavigate to send the
> proper start/stop loading notifications. Fabrice is working on making the
> start/stop loading work with PlzNavigate, so I expect him to fix that in a
later
> patch).

Oh.  Wouldn't all sorts of things potentially cancel the commit in that case? 
What if a Stop happened, triggering a DidFailProvisionalLoad?

[clamy, 2015-04-14 14:14:16]

Thanks! I removed the DCHECK, and did some rebasing as well.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:848:
DCHECK(pending_commit_);
On 2015/04/10 23:04:01, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/10 14:40:15, clamy wrote:
> > On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> > > On 2015/04/08 11:56:23, clamy wrote:
> > > > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > > > This doesn't seem right to me.
> > > > > 
> > > > > If the renderer process does a in-page navigation (fragment,
pushState,
> > > etc),
> > > > > we'll get a DidCommitProvisionalLoad without going through
> > CommitNavigation,
> > > > > right?
> > > > 
> > > > I think we want to inform the browser that we're doing a synchronous
> > > navigation
> > > > first (through BeginNavigation). This allows to cancel an ongoing
> navigation
> > > if
> > > > needed. Then the Navigator can see we don't need to go through the
network
> > > stack
> > > > and inform the appropriate RFH to expect a commit.
> > > 
> > > Ok, so the renderer will tell the browser via BeginNavigation, then later
in
> > the
> > > same task commit the navigation via DidCommit.  That part seems ok.
> > > 
> > > However, it doesn't look like you're setting pending_commit_ to be true in
> > that
> > > case.  NavigatorImpl::BeginNavigation will get false from
> > > NavigationRequest::BeginNavigation if it's a pushState, etc, and it
doesn't
> > tell
> > > the current RFH to expect the commit.  It's only set to true in
> > > CommitNavigation, which doesn't look like it's called here.  Am I missing
> > > something?
> > 
> > No that it is not implemented yet. We do not currently support same-site
> > navigations properly.
> 
> Ok.  If you're expecting this check to fail, please add a TODO here to fix it
> (mentioning why it would fail).

Considering that the DCHECK may fail for a certain number of not fully
understood reasons, I'm removing it.

https://codereview.chromium.org/1048463004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:952: // A commit is no
longer expected as the frame is swapped out.
On 2015/04/10 23:04:01, Charlie Reis (slow until 4-15) wrote:
> On 2015/04/10 14:40:15, clamy wrote:
> > On 2015/04/08 16:45:27, Charlie Reis (slow until 4-15) wrote:
> > > On 2015/04/08 11:56:23, clamy wrote:
> > > > On 2015/04/07 17:04:47, Charlie Reis wrote:
> > > > > I don't understand how this would happen.  When would a frame get
> swapped
> > > out
> > > > > after a call to CommitNavigation?
> > > > 
> > > > If the current RFH was supposed to commit a navigation, but for some
> reason
> > it
> > > > took a lot of time, which allowed a new cross-site navigation to commit
in
> > the
> > > > meantime. Then the current RFH gets swapped out before sending
> > > > DidCommitProvisionalLoad.
> > > 
> > > I thought that the DidCommitProvisionalLoad IPC was sent in the same task
as
> > > receiving the OnCommitNavigation IPC, right?  That means that the renderer
> > > process won't have a chance to see the FrameMsg_SwapOut request before
> sending
> > > the DidCommit.  I think this will cause your DCHECK in DidCommit to fail.
> > > 
> > > (Looking closer, I see that there's an early return in
> > > RenderFrameImpl::OnCommitNavigation if PrepareRenderViewForNavigation
> returns
> > > false, but it's not sending a DidDropNavigation IPC the way we do in
default
> > > Chrome.  That seems potentially buggy-- don't we need to stop the spinner
> and
> > > clean up the pending state if we ignore the navigation as stale?  We
should
> > > probably clear pending_commit_ in that case as well.)
> > > 
> > > It sounds like we probably want a test for this case, since it's not clear
> > > whether it works.
> > 
> > No the DidCommitProvisionalLoad IPC is sent when the renderer gets data from
> the
> > browser from the stream request, which does not happen synchronously.
> > 
> > (Indeed we noticed that as well when trying to get PlzNavigate to send the
> > proper start/stop loading notifications. Fabrice is working on making the
> > start/stop loading work with PlzNavigate, so I expect him to fix that in a
> later
> > patch).
> 
> Oh.  Wouldn't all sorts of things potentially cancel the commit in that case? 
> What if a Stop happened, triggering a DidFailProvisionalLoad?

Good point. I removed the DCHECK, and I wont reset pending_commit_ here.

[nasko, 2015-04-15 21:12:46]

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:458: // Note: In
PlzNavigate |cross_navigation_pending_| being false means there is
Since cross_navigation_pending_ is not used in PlzNavigate, this comment doesn't
seem valid any longer.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1572: host =
GetFrameHostForNavigation(manager, entry1);
It seems a bit strange to replace a "Navigate" call with a "GetRFH" call. I
think it is fine in this case, but want to get Charlie's opinion on it as well.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1948:
EXPECT_FALSE(contents()->CrossNavigationPending());
Why did we lose the string here?

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:452:
main_test_rfh()->PrepareForCommit();
What condition requires us to call PrepareForCommit on the main_test_rfh here?

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:815:
main_test_rfh()->PrepareForCommit();
This looks strange. We are preparing for commit the main_test_rfh, yet we call
TestDidNavigate with orig_rfh. Why is that?

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:941: //
SiteInstances, because we don't intercept WebKit navigations.
nit: Might as well s/WebKit/Blink/ as we are touching this code.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1231:
main_test_rfh()->PrepareForCommit();
Same issue as pointed out above, discrepancy between main_test_rfh and orig_rfh.

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
content/test/test_web_contents.h:52: bool CrossNavigationPending();
Shouldn't it be CrossSiteNavigationPending, since the navigation is cross-site,
not just "cross"?

[Charlie Reis, 2015-04-15 21:54:17]

Nasko had a few good catches, but otherwise LGTM with nits.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:671: // Used to track
whether a commit is expected in this frame.
Let's mention that this is only used in tests.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1572: host =
GetFrameHostForNavigation(manager, entry1);
On 2015/04/15 21:12:45, nasko wrote:
> It seems a bit strange to replace a "Navigate" call with a "GetRFH" call. I
> think it is fine in this case, but want to get Charlie's opinion on it as
well.

I agree that the name of the helper function feels wrong.  (I'd go further and
say it doesn't seem right for this case.)

The helper function is actually calling Navigate in the non-PlzNavigate case. 
Can we find a name that conveys this and works for PlzNavigate as well?

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
content/test/test_web_contents.h:52: bool CrossNavigationPending();
On 2015/04/15 21:12:46, nasko wrote:
> Shouldn't it be CrossSiteNavigationPending, since the navigation is
cross-site,
> not just "cross"?

Yes, as long as we're updating the name, please fix this.  (It's always bothered
me.)  I think CrossProcessNavigationPending is probably better, since not all
cross-site navigations are cross-process.  (Technically it could end up in a new
SiteInstance but the same RenderProcessHost, but that's splitting hairs.)

(As I mentioned separately, I'll remove that cross_navigation_pending_ member
once this CL lands.)

[clamy, 2015-04-16 13:18:36]

Thanks!

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:671: // Used to track
whether a commit is expected in this frame.
On 2015/04/15 21:54:17, Charlie Reis wrote:
> Let's mention that this is only used in tests.

Done.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:458: // Note: In
PlzNavigate |cross_navigation_pending_| being false means there is
On 2015/04/15 21:12:45, nasko wrote:
> Since cross_navigation_pending_ is not used in PlzNavigate, this comment
doesn't
> seem valid any longer.

Done.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1572: host =
GetFrameHostForNavigation(manager, entry1);
On 2015/04/15 21:54:17, Charlie Reis wrote:
> On 2015/04/15 21:12:45, nasko wrote:
> > It seems a bit strange to replace a "Navigate" call with a "GetRFH" call. I
> > think it is fine in this case, but want to get Charlie's opinion on it as
> well.
> 
> I agree that the name of the helper function feels wrong.  (I'd go further and
> say it doesn't seem right for this case.)
> 
> The helper function is actually calling Navigate in the non-PlzNavigate case. 
> Can we find a name that conveys this and works for PlzNavigate as well?

I've renamed the function NavigateToEntry since it is what we are trying to
simulate there.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1948:
EXPECT_FALSE(contents()->CrossNavigationPending());
On 2015/04/15 21:12:45, nasko wrote:
> Why did we lose the string here?

Done.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:452:
main_test_rfh()->PrepareForCommit();
On 2015/04/15 21:12:46, nasko wrote:
> What condition requires us to call PrepareForCommit on the main_test_rfh here?

Calling TestDidNavigate. This dates from when I added a DCHECK to see if we
expect a commit. Since we had not performed the logic up until CommitNavigation
we would not expect a commit. This made me find a certain number of instances
where we do not properly exercise the navigation logic in PlzNavigate, hence why
I added these calls. Here it should be moved before the first test for
CrossNavigationPending (it's always false with PlzNavigate before we call
PrepareForCommit).

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:815:
main_test_rfh()->PrepareForCommit();
On 2015/04/15 21:12:46, nasko wrote:
> This looks strange. We are preparing for commit the main_test_rfh, yet we call
> TestDidNavigate with orig_rfh. Why is that?

I guess we should use orig_rfh. The thing is with PlzNavigate enabled, it does
not matter which RFH is used for PrepareForCommit as long as the FTN is the
right one (since all functions depend on the FTN rather than the RFH).

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:941: //
SiteInstances, because we don't intercept WebKit navigations.
On 2015/04/15 21:12:46, nasko wrote:
> nit: Might as well s/WebKit/Blink/ as we are touching this code.

Done.

https://codereview.chromium.org/1048463004/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1231:
main_test_rfh()->PrepareForCommit();
On 2015/04/15 21:12:46, nasko wrote:
> Same issue as pointed out above, discrepancy between main_test_rfh and
orig_rfh.

Done.

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1048463004/diff/180001/content/test/test_web_...
content/test/test_web_contents.h:52: bool CrossNavigationPending();
On 2015/04/15 21:54:17, Charlie Reis wrote:
> On 2015/04/15 21:12:46, nasko wrote:
> > Shouldn't it be CrossSiteNavigationPending, since the navigation is
> cross-site,
> > not just "cross"?
> 
> Yes, as long as we're updating the name, please fix this.  (It's always
bothered
> me.)  I think CrossProcessNavigationPending is probably better, since not all
> cross-site navigations are cross-process.  (Technically it could end up in a
new
> SiteInstance but the same RenderProcessHost, but that's splitting hairs.)
> 
> (As I mentioned separately, I'll remove that cross_navigation_pending_ member
> once this CL lands.)

Done.

[nasko, 2015-04-16 16:43:43]

The only thing outstanding is the inconsistent use of main_test_rfh() vs
orig_rfh in web_contents_impl_unittest. Let's ensure we use the same one in all
instances. It looks good otherwise.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:827:
main_test_rfh()->PrepareForCommit();
orig_rfh?

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:942:
main_test_rfh()->SendRendererInitiatedNavigationRequest(url2, true);
orig_rfh?

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1244:
main_test_rfh()->PrepareForCommit();
orig_rfh?

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1251:
main_test_rfh()->PrepareForCommit();
orig_rfh?

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1266:
main_test_rfh()->SendRendererInitiatedNavigationRequest(url, false);
orig_rfh?

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1352:
main_test_rfh()->PrepareForCommit();
orig_rfh?

[Charlie Reis, 2015-04-16 17:04:54]

Thanks for the fixes!  Still LGTM, pending Nasko's approval.

[clamy, 2015-04-17 13:15:38]

Thanks!

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:827:
main_test_rfh()->PrepareForCommit();
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:942:
main_test_rfh()->SendRendererInitiatedNavigationRequest(url2, true);
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1244:
main_test_rfh()->PrepareForCommit();
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1251:
main_test_rfh()->PrepareForCommit();
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1266:
main_test_rfh()->SendRendererInitiatedNavigationRequest(url, false);
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

https://codereview.chromium.org/1048463004/diff/220001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1352:
main_test_rfh()->PrepareForCommit();
On 2015/04/16 16:43:42, nasko wrote:
> orig_rfh?

Done.

[nasko, 2015-04-17 14:06:51]

There are just a few more instances of main_test_rfh usage. LGTM once changed.

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1289:
main_test_rfh()->PrepareForCommit();
orig_rfh

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1360:
main_test_rfh()->PrepareForCommit();
orig_rfh

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1383:
main_test_rfh()->PrepareForCommit();
orig_rfh

[clamy, 2015-04-17 14:12:49]

Thanks!

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1289:
main_test_rfh()->PrepareForCommit();
On 2015/04/17 14:06:51, nasko wrote:
> orig_rfh

Done.

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1360:
main_test_rfh()->PrepareForCommit();
On 2015/04/17 14:06:51, nasko wrote:
> orig_rfh

Done.

https://codereview.chromium.org/1048463004/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1383:
main_test_rfh()->PrepareForCommit();
On 2015/04/17 14:06:51, nasko wrote:
> orig_rfh

Done.

[clamy, 2015-04-17 14:12:57]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-17 14:12:58]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1048463004/#ps280001
(title: "Removed other occurences of main_test_rfh")

[commit-bot: I haz the power, 2015-04-17 14:13:12]

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

[commit-bot: I haz the power, 2015-04-17 15:23:43]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-17 15:25:22]

Patchset 15 (id:??) landed as
https://crrev.com/5bf36119961044dd6c01b16310862bfceb080058
Cr-Commit-Position: refs/heads/master@{#325639}

[Avi (use Gerrit), 2015-04-17 18:23:12]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2015-04-17 18:23:13]

https://codereview.chromium.org/1048463004/diff/280001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/1048463004/diff/280001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1475:
main_test_rfh()->NavigateAndCommitRendererInitiated(0, url2);
Is this right? We initiated the GoBack on line 1455 above, and that's
browser-initiated. In fact, all history navigations run through the browser.