Start a renderer-initiated navigation before commit in RendererInitiatedPendingEntries

Start a renderer-initiated navigation before commit in RendererInitiatedPendingEntries

The RendererInitiatedPendingEntries test does a direct call to
Navigator::DidStartProvisionalLoad without setting up any state on the
RenderFrameHost object. This CL adds some initial steps to prepare the
necessary state for successful navigation.

BUG=

Committed: https://crrev.com/dd3c23aa1cfe0f09d5e5632bfe789f9ccb02585a
Cr-Commit-Position: refs/heads/master@{#331814}

[nasko, 2015-05-28 00:25:29]

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

[nasko, 2015-05-28 00:25:29]

Hey Charlie and Camille,
Can you review this CL for me? The RendererInitiatedPendingEntries test needs to
prep the state for calling into Navigator. It currently works somehow, but with
some of the changes I'm making to unit tests and swapped-out removal it will
start to fail.
Thanks in advance!
Nasko

[Charlie Reis, 2015-05-28 17:02:27]

LGTM if this is just for PlzNavigate.

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

https://codereview.chromium.org/1159043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3255:
main_test_rfh()->PrepareForCommit();
These two lines are only needed for PlzNavigate, right?  There's no interactions
with the browser process before DidStartProvisionalLoad by default, and the
BeforeUnload_ACK in PrepareForCommit isn't used for renderer-initiated
navigations.

[nasko, 2015-05-28 17:16:44]

This is not just for PlzNavigate. It is a break off from my bigger CL to fix
unit tests - https://codereview.chromium.org/1151973005/.

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

https://codereview.chromium.org/1159043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3255:
main_test_rfh()->PrepareForCommit();
On 2015/05/28 17:02:27, Charlie Reis wrote:
> These two lines are only needed for PlzNavigate, right?  There's no
interactions
> with the browser process before DidStartProvisionalLoad by default, and the
> BeforeUnload_ACK in PrepareForCommit isn't used for renderer-initiated
> navigations.

While they are fairly PlzNavigate specific, in my unit tests CL I've added
initialization of the RenderFrame as the first step in
SendRendererInitiatedNavigationRequest. I could manually do it, but it seemed
better to write it in a way that is compatible with PlzNavigate style
navigation.

I could drop the PrepareForCommit. In general I have been aiming for more
streamlined flow of calls that are consistent, logical and don't force you to
know all the intricate details of each call.

[Charlie Reis, 2015-05-28 17:40:19]

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

https://codereview.chromium.org/1159043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3255:
main_test_rfh()->PrepareForCommit();
On 2015/05/28 17:16:44, nasko wrote:
> On 2015/05/28 17:02:27, Charlie Reis wrote:
> > These two lines are only needed for PlzNavigate, right?  There's no
> interactions
> > with the browser process before DidStartProvisionalLoad by default, and the
> > BeforeUnload_ACK in PrepareForCommit isn't used for renderer-initiated
> > navigations.
> 
> While they are fairly PlzNavigate specific, in my unit tests CL I've added
> initialization of the RenderFrame as the first step in
> SendRendererInitiatedNavigationRequest. I could manually do it, but it seemed
> better to write it in a way that is compatible with PlzNavigate style
> navigation.

Ok.

> 
> I could drop the PrepareForCommit. In general I have been aiming for more
> streamlined flow of calls that are consistent, logical and don't force you to
> know all the intricate details of each call.

Hmm, there's a tradeoff here.  We're already running into problems where the
unit tests don't match reality, and this introduces another case
(BeforeUnload_ACK).  I'm ok with it in this case, though, since we need to be
able to handle that message and it does make writing the tests easier.

[nasko, 2015-05-28 17:43:02]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-05-28 17:43:19]

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

[commit-bot: I haz the power, 2015-05-28 17:48:55]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-05-28 17:49:54]

Patchset 1 (id:??) landed as
https://crrev.com/dd3c23aa1cfe0f09d5e5632bfe789f9ccb02585a
Cr-Commit-Position: refs/heads/master@{#331814}