Clone children of FrameNavigationEntries for in-page navigations.

Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[Charlie Reis, 2016-07-20 05:20:09]

Description was changed from

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681
TEST=See bug for repro steps
==========

to

==========
Clone children of FrameNavigationEntries for in-page navigations.

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

[Charlie Reis, 2016-07-20 05:45:36]

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

[commit-bot: I haz the power, 2016-07-20 05:45:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 06:07:24]

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

[commit-bot: I haz the power, 2016-07-20 06:07:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Charlie Reis, 2016-07-25 21:48:35]

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

[commit-bot: I haz the power, 2016-07-25 21:48:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-25 22:53:00]

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

[commit-bot: I haz the power, 2016-07-25 22:53:00]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-07-26 05:17:03]

Description was changed from

==========
Clone children of FrameNavigationEntries for in-page navigations.

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

to

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-07-26 06:03:07]

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

[commit-bot: I haz the power, 2016-07-26 06:03:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 07:20:44]

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

[commit-bot: I haz the power, 2016-07-26 07:20:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-07-26 14:49:07]

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

[commit-bot: I haz the power, 2016-07-26 14:49:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-07-26 14:49:29]

Description was changed from

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-07-26 14:53:14]

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

[Charlie Reis, 2016-07-26 14:53:15]

avi@: Can you review?  This fixes two of the remaining navigation layout tests
for UseSubframeNavigationEntries mode.

boliu@: Just giving you a heads up about the LoadDataWithBaseURL test
expectation fix.

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:300: //
TODO(boliu): These expectations maybe incorrect due to crbug.com/561034.
boliu@: These were fixed by cloning the last committed entry for in-page
navigations in RendererDidNavigateToNewPage.  Before, there was no pending entry
for the renderer-initiated navigation, so we created a new NavEntry from
scratch.  Now we preserve this state from the last committed entry, which seems
better.

[Avi (use Gerrit), 2016-07-26 15:03:58]

lgtm

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1062: if (is_in_page &&
GetLastCommittedEntry()) {
Curious: how would we be in-page but not have a last-committed entry? Are you
thinking about horrible about:blank silliness?

Creating an about:blank entry for an empty nav session can't come soon enough.

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:158: index = (i + j) %
ftn_child_count;
Cute, and actually decently easy to reason about.

[boliu, 2016-07-26 15:17:28]

lg2m

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:302:
EXPECT_EQ(history_url, entry->GetVirtualURL());
this is repeated twice?

[commit-bot: I haz the power, 2016-07-26 15:24:04]

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

[commit-bot: I haz the power, 2016-07-26 15:24:05]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-07-26 15:30:46]

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1062: if (is_in_page &&
GetLastCommittedEntry()) {
On 2016/07/26 15:03:58, Avi wrote:
> Curious: how would we be in-page but not have a last-committed entry? Are you
> thinking about horrible about:blank silliness?
> 
> Creating an about:blank entry for an empty nav session can't come soon enough.

Yeah, exactly.  PushState on the initial blank page would get here.  And yes,
I'm looking forward to having a NavEntry for that case, once we fight through
all the test failures.

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

https://codereview.chromium.org/2161393002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:302:
EXPECT_EQ(history_url, entry->GetVirtualURL());
On 2016/07/26 15:17:27, boliu wrote:
> this is repeated twice?

Oops, that was supposed to be GetHistoryURLForDataURL().  Thanks!

[Charlie Reis, 2016-07-26 15:30:51]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-07-26 15:30:52]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2161393002/#ps100001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-07-26 15:31:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 17:06:09]

Description was changed from

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-26 17:06:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-26 17:11:14]

Description was changed from

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clone children of FrameNavigationEntries for in-page navigations.

BUG=623681, 522193, 561034
TEST=See bug for repro steps
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-07-26 17:11:16]

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