PlzNavigate: Properly set the pending entry in the NavigatorImpl unit test.

PlzNavigate: Trigger navigation via the NavigationController in the NavigatorImpl unit tests.

BUG=369661
TEST=tests should stay passing

Committed: https://crrev.com/73777224191ef2115df9956c794db4cedac08da3
Cr-Commit-Position: refs/heads/master@{#321392}

[Avi (use Gerrit), 2015-03-17 17:37:07]

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

[Avi (use Gerrit), 2015-03-17 17:37:07]

The problem here is that NavigatorImpl::RequestNavigation is called through
NavigateToPendingEntry which is called from WebContents::NavigateToPendingEntry
which is called from NavigationControllerImpl::NavigateToPendingEntry so it's
assumed that the pending entry is filled in at that point.

Since we're bypassing NavController, we need to make sure the pending entry is
filled in ourselves.

And we _can't_ do that because that's the private guts of NavController. So if
Camille needs to keep tinkering away in this manner, we need to expose control
over the pending entry for tests. Thus this CL.

Plus some fixes for the wrong page id being passed in SendNavigate calls.

[Charlie Reis, 2015-03-17 22:20:22]

Unfortunate that we need to expose this setter for testing code, but it does
look like it might be needed for PlzNavigate.  Seems reasonable to me so far.

Avi, can you clarify where it matters that the given entry is the
NavigationController's pending entry?  That seems right to me but I'm curious
where you ran into it.

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

https://codereview.chromium.org/1010243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl_unittest.cc:86:
static_cast<NavigatorImpl*>(node->navigator())->RequestNavigation(
The goal of this change is to make sure that |entry| is the pending entry at
this point, right?  Can you add a DCHECK_EQ(controller()->GetPendingEntry(),
entry) here, and maybe a comment for why it matters?

(Or Camille, does that check belong inside NavigatorImpl::RequestNavigation
itself?)

[Avi (use Gerrit), 2015-03-17 23:28:16]

Charlie—

I'm not clear what you mean by "clarify". I put in a comment, but if you're not
clear, with the new classifier, the "did commit" message has the unique id of
the corresponding nav entry, so we need to get our hands on the nav entry and
make sure that it's properly marked as the pending entry to make this all work.

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

https://codereview.chromium.org/1010243002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl_unittest.cc:86:
static_cast<NavigatorImpl*>(node->navigator())->RequestNavigation(
On 2015/03/17 22:20:22, Charlie Reis wrote:
> The goal of this change is to make sure that |entry| is the pending entry at
> this point, right?  Can you add a DCHECK_EQ(controller()->GetPendingEntry(),
> entry) here, and maybe a comment for why it matters?
> 
> (Or Camille, does that check belong inside NavigatorImpl::RequestNavigation
> itself?)

Done.

[Charlie Reis, 2015-03-17 23:39:40]

LGTM

[Avi (use Gerrit), 2015-03-17 23:49:14]

On 2015/03/17 23:39:40, Charlie Reis wrote:
> LGTM

Thank you.

Camille—what do you think?

[clamy, 2015-03-18 12:46:17]

Thanks! I'm just wondering if we can't use the NavigationController directly to
start the navigations in the PlzNavigate unit tests rather than having to expose
the pending entry. I'm not particular on calling RequestNavigation explicitly.

[Avi (use Gerrit), 2015-03-18 15:25:18]

On 2015/03/18 12:46:17, clamy wrote:
> Thanks! I'm just wondering if we can't use the NavigationController directly
to
> start the navigations in the PlzNavigate unit tests rather than having to
expose
> the pending entry. I'm not particular on calling RequestNavigation explicitly.

I was doing this because I didn't want to completely re-write your unit tests.
If you're inviting me to do so, OK, I will, because my classifier CL requires at
least this change.

Would you prefer a rewrite of the unit tests to directly call NavController
rather than the CL as it is now?

[clamy, 2015-03-18 15:38:52]

I'm thinking that in the long term it is cleaner not to have the setting of the
pending entry exposed in NavigationController, even if it is just for tests. We
wrote the unit tests that way because we were at the beginning of the project
and the design was less clear than it is now, but really we should use the
NavigationController to start the navigation in those tests.

[Avi (use Gerrit), 2015-03-18 18:54:11]

New version; please take a look.

This expands the existing SetPendingEntry call to always do the right thing, and
adds no new testing API.

[Charlie Reis, 2015-03-18 20:26:35]

This version also looks good to me, but I don't think it addresses Camille's
request.  She was saying we should avoid manually setting the pending entry, and
that seems reasonable to me.

Camille, maybe you could elaborate on how to use NavigationController instead of
RequestNavigation in the test, if that's what you were looking for?

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:428:
pending_entry_index_ = GetIndexOfEntry(entry);
Interesting that we didn't need this before, but I think it's because this was
only used for newly created NavigationEntries at first, when the index was
assumed to be -1.  I agree that this looks correct and more robust.

[Avi (use Gerrit), 2015-03-18 20:32:42]

On 2015/03/18 20:26:35, Charlie Reis wrote:
> This version also looks good to me, but I don't think it addresses Camille's
> request.  She was saying we should avoid manually setting the pending entry,
and
> that seems reasonable to me.

Yes, though I am still trying to find a middle ground of not having to poke
testing holes in the NavController and yet not having to rewrite all of the
tests.

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:428:
pending_entry_index_ = GetIndexOfEntry(entry);
On 2015/03/18 20:26:35, Charlie Reis wrote:
> Interesting that we didn't need this before, but I think it's because this was
> only used for newly created NavigationEntries at first, when the index was
> assumed to be -1.  I agree that this looks correct and more robust.

Yep.

[clamy, 2015-03-19 12:18:04]

What I had in mind was to rewrite the helper function to use
NavigationController::LoadURL, which I think should not lead to rewriting all
the tests (since NavigatorImpl::RequestNavigation can only be called following a
call to NavigationController::LoadURL in the first place).

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1010243002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:85:
static_cast<NavigatorImpl*>(node->navigator())->RequestNavigation(
What I had in mind is to replace this call to NavigatorImpl::RequestNavigation
by a call to NavigationController::LoadURL or
NavigationController::LoadURLWithParams. Both will properly set the pending
entry and then eventually call  NavigatorImpl::RequestNavigation. Also since it
is in the helper function you don't need to rewrite all the tests. It is
possible that the subframe test will fail below, but that's because there are
things we don't properly handle there, so it can be disabled until we do that.

[Avi (use Gerrit), 2015-03-19 14:51:16]

https://codereview.chromium.org/1010243002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1010243002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:781:
controller().ReloadIgnoringCache(false);
Note that ReloadType isn't a part of LoadURLParams, so I just switched to the
reload calls.

[clamy, 2015-03-19 16:34:52]

Thanks! Lgtm.

https://codereview.chromium.org/1010243002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1010243002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:781:
controller().ReloadIgnoringCache(false);
On 2015/03/19 14:51:16, Avi wrote:
> Note that ReloadType isn't a part of LoadURLParams, so I just switched to the
> reload calls.

Acknowledged.

[Avi (use Gerrit), 2015-03-19 17:44:25]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-03-19 17:44:26]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1010243002/#ps60001
(title: "simpler")

[commit-bot: I haz the power, 2015-03-19 17:44:52]

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

[commit-bot: I haz the power, 2015-03-19 18:04:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-19 18:05:27]

Patchset 4 (id:??) landed as
https://crrev.com/73777224191ef2115df9956c794db4cedac08da3
Cr-Commit-Position: refs/heads/master@{#321392}