Ensure the NavigationHandle's nav entry ID is updated during transfers.

Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[Charlie Reis, 2016-03-09 21:26:58]

Description was changed from

==========
Ensure the pending entry matches before using it to determine replacement.

We can compare IDs with the NavigationHandle to avoid confusion here.

BUG=593153
TEST=See bug for repro steps.
==========

to

==========
Ensure the pending entry matches before using it to determine replacement.

We can compare IDs with the NavigationHandle to avoid confusion here.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-11 00:19:33]

Description was changed from

==========
Ensure the pending entry matches before using it to determine replacement.

We can compare IDs with the NavigationHandle to avoid confusion here.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure the pending entry matches before using it to determine replacement.

We can compare IDs with the NavigationHandle to avoid confusion here.

This also requires updating the NavigationHandle's pending NavEntry ID
during transfers, and avoiding going straight to RendererDidNavigate in
unit tests.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-11 00:22:08]

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

[Charlie Reis, 2016-03-11 00:22:09]

nasko@: Can I ask you to review this?  The main fix is in RendererDidNavigate,
but it required some other NavigationHandle related changes as well.  I also
removed all direct calls to RendererDidNavigate in unit tests.

csharrison@: Can you also review the NavigationHandle updates?  I think they'll
likely help in the other ways we use pending_nav_entry_id as well, at least
until we don't have pending entries for transfers anymore.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2837: void
DoReplaceStateWhilePending(Shell* shell,
Amusingly, this is almost exactly the test I needed, except I want the pending
navigation to have replacement and the in-page navigation to create a new entry
(the reverse of this test).

[Charlie Harrison, 2016-03-11 15:15:14]

Changes look good! Thanks!

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:859: // like
cross-process client redirects or cross-process reloads.
Maybe reference location.replace(<cross-site>). I was initially confused reading
this comment about why you would ever need a cross-process reload.

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:870:
pending_entry_->GetUniqueID() == handle->pending_nav_entry_id() &&
Suggestion: This logic is duplicated in a bunch of places. It might be nice to
consolidate it. I'm mainly thinking of the check in
|RendererDidNavigateToNewPage|. Up to you though, it's arguably less clean to
thread a bool through that function.

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:133: void
set_pending_nav_entry_id(int nav_entry_id) {
If this is only required for transfer navigations, maybe name it something like
UpdateEntryIdForTransfer? This makes the intention clear without having to find
the comment.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1122: // If there's a current
NavigationHandle, update its pending NavEntry ID.
Good catch.

[nasko, 2016-03-11 17:10:06]

Mostly nits and one question on tests. Otherwise it looks good.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:300: class
LoadCommittedDetailsObserver : public WebContentsObserver {
Can't we use FrameNavigateParamsCapturer which is used in other unit tests?

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1887:
LoadCommittedDetailsObserver observer(contents());
nit: I'd move this closer to the commit it is observing, since it isn't used to
check anything for the first commit (line 1890).

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1956:
LoadCommittedDetailsObserver observer(contents());
nit: Same nit as above, closer to the commit it observes.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:299: // Update the
pending NavigationEntry ID on the handle as well.
nit: Empty line before the comment makes it a bit more readable.

[Charlie Reis, 2016-03-11 20:12:29]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[Charlie Reis, 2016-03-11 20:12:49]

Thanks!  PTAL.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:859: // like
cross-process client redirects or cross-process reloads.
On 2016/03/11 15:15:14, csharrison wrote:
> Maybe reference location.replace(<cross-site>). I was initially confused
reading
> this comment about why you would ever need a cross-process reload.

Done.

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:870:
pending_entry_->GetUniqueID() == handle->pending_nav_entry_id() &&
On 2016/03/11 15:15:14, csharrison wrote:
> Suggestion: This logic is duplicated in a bunch of places. It might be nice to
> consolidate it. I'm mainly thinking of the check in
> |RendererDidNavigateToNewPage|. Up to you though, it's arguably less clean to
> thread a bool through that function.

Good idea.  I wrote a helper function for it.

We can now be sure that the handle isn't null in the
RendererDidNavigateToNewPage case as well, thanks to the unit test changes.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:300: class
LoadCommittedDetailsObserver : public WebContentsObserver {
On 2016/03/11 17:10:06, nasko (slow) wrote:
> Can't we use FrameNavigateParamsCapturer which is used in other unit tests?

That one is defined for a browsertest and spins a message loop while waiting for
the events to occur.  I think that's more complexity than we need here, where
the unit test drives the message and knows when it can check details().

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1887:
LoadCommittedDetailsObserver observer(contents());
On 2016/03/11 17:10:06, nasko (slow) wrote:
> nit: I'd move this closer to the commit it is observing, since it isn't used
to
> check anything for the first commit (line 1890).

Done.

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:1956:
LoadCommittedDetailsObserver observer(contents());
On 2016/03/11 17:10:06, nasko (slow) wrote:
> nit: Same nit as above, closer to the commit it observes.

Done.

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:133: void
set_pending_nav_entry_id(int nav_entry_id) {
On 2016/03/11 15:15:14, csharrison wrote:
> If this is only required for transfer navigations, maybe name it something
like
> UpdateEntryIdForTransfer? This makes the intention clear without having to
find
> the comment.

Done.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:299: // Update the
pending NavigationEntry ID on the handle as well.
On 2016/03/11 17:10:06, nasko (slow) wrote:
> nit: Empty line before the comment makes it a bit more readable.

Done.

[commit-bot: I haz the power, 2016-03-11 20:13:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1777903003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1777903003/80001

[nasko, 2016-03-11 20:21:56]

LGTM with a nit.

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

https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:300: class
LoadCommittedDetailsObserver : public WebContentsObserver {
On 2016/03/11 20:12:48, Charlie Reis (slow til 3-15) wrote:
> On 2016/03/11 17:10:06, nasko (slow) wrote:
> > Can't we use FrameNavigateParamsCapturer which is used in other unit tests?
> 
> That one is defined for a browsertest and spins a message loop while waiting
for
> the events to occur.  I think that's more complexity than we need here, where
> the unit test drives the message and knows when it can check details().

Acknowledged.

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

https://codereview.chromium.org/1777903003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1126:
pending_entry_index_ == -1 &&
nit: Wouldn't this fit on the previous line? git cl format might be helpful for
these types of fixes.

[Charlie Harrison, 2016-03-11 20:23:47]

On 2016/03/11 at 20:21:56, nasko wrote:
> LGTM with a nit.
> 
>
https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1777903003/diff/60001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl_unittest.cc:300: class
LoadCommittedDetailsObserver : public WebContentsObserver {
> On 2016/03/11 20:12:48, Charlie Reis (slow til 3-15) wrote:
> > On 2016/03/11 17:10:06, nasko (slow) wrote:
> > > Can't we use FrameNavigateParamsCapturer which is used in other unit
tests?
> > 
> > That one is defined for a browsertest and spins a message loop while waiting
for
> > the events to occur.  I think that's more complexity than we need here,
where
> > the unit test drives the message and knows when it can check details().
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1777903003/diff/80001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/1777903003/diff/80001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl.cc:1126:
pending_entry_index_ == -1 &&
> nit: Wouldn't this fit on the previous line? git cl format might be helpful
for these types of fixes.

lgtm

[Charlie Reis, 2016-03-11 21:11:37]

Fixed.  Sadly, there's a test failure on Android involving back/forward after
prerendering.  I'll have to look into it, but that might delay this...

https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

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

https://codereview.chromium.org/1777903003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1126:
pending_entry_index_ == -1 &&
On 2016/03/11 20:21:56, nasko (slow) wrote:
> nit: Wouldn't this fit on the previous line? git cl format might be helpful
for
> these types of fixes.

Done.

[Charlie Reis, 2016-03-13 22:24:54]

Patchset #8 (id:140001) has been deleted

[Charlie Reis, 2016-03-13 22:51:50]

Patchset #8 (id:160001) has been deleted

[Charlie Reis, 2016-03-14 18:42:23]

Description was changed from

==========
Ensure the pending entry matches before using it to determine replacement.

We can compare IDs with the NavigationHandle to avoid confusion here.

This also requires updating the NavigationHandle's pending NavEntry ID
during transfers, and avoiding going straight to RendererDidNavigate in
unit tests.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-14 18:48:50]

Since the Android prerendering thing seems subtle, I decided to move the actual
fix and test for location.replace to my larger
https://codereview.chromium.org/1794513003/ CL.  (This CL was just intended to
fix the old approach so that we could assert the new approach agreed in
practice.  There's no strong reason to fix the old way-- let's just move to the
new way, now that we know it's better.)

The remaining things here are the cleanup for NavigationHandle to make it
correct after transfers and never null at commit time.  Nasko and Charles, can
you take another look?

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

https://codereview.chromium.org/1777903003/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl_unittest.cc:430:
EXPECT_EQ(url_1, controller.GetVisibleEntry()->GetURL());
I also included changes to this test to list the expected value before the
actual value, since the test output was wrong when it failed in a previous
version of this CL.

[Charlie Harrison, 2016-03-14 18:55:33]

still lgtm

[Charlie Reis, 2016-03-14 21:23:45]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-14 21:24:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1777903003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1777903003/180001

[commit-bot: I haz the power, 2016-03-14 21:29:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-14 21:29:24]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-03-14 21:30:37]

Still LGTM.

[Charlie Reis, 2016-03-14 21:34:02]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-03-14 21:34:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1777903003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1777903003/180001

[commit-bot: I haz the power, 2016-03-14 21:39:28]

Description was changed from

==========
Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-14 21:39:29]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-03-14 21:40:16]

Description was changed from

==========
Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ensure the NavigationHandle's nav entry ID is updated during transfers.

The pending_nav_entry_id is now correct after transfers, and the handle
is always present at commit (thanks to fixes to unit tests).

This is part of a fix for matching the pending entry with the handle
during navigations with replacement.

BUG=593153
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b4dc9337cacb2152671ea1bcfbcbd236d6fbff20
Cr-Commit-Position: refs/heads/master@{#381081}
==========

[commit-bot: I haz the power, 2016-03-14 21:40:17]

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