Create FrameNavigationEntries for manual subframe navigations.

Create FrameNavigationEntries for manual subframe navigations.

Only affects --site-per-process mode.

BUG=236848
TEST=NavigationController* tests pass in --site-per-process mode.

Committed: https://crrev.com/96fc55084b605d88fca0fffa667e0b547cb80d9b
Cr-Commit-Position: refs/heads/master@{#334331}

[Charlie Reis, 2015-05-18 18:22:18]

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

[Charlie Reis, 2015-05-18 18:22:19]

Avi, can you take a look?  This should take care of the other common FNE cases.

[Avi (use Gerrit), 2015-05-19 14:52:44]

On 2015/05/18 18:22:19, Charlie Reis wrote:
> Avi, can you take a look?  This should take care of the other common FNE
cases.

On chat you said you wanted me to wait until you got a version that was green
everywhere. Let me know when you're ready for a look.

[Charlie Reis, 2015-05-19 16:07:11]

On 2015/05/19 14:52:44, Avi wrote:
> On 2015/05/18 18:22:19, Charlie Reis wrote:
> > Avi, can you take a look?  This should take care of the other common FNE
> cases.
> 
> On chat you said you wanted me to wait until you got a version that was green
> everywhere. Let me know when you're ready for a look.

Thanks.  I diagnosed the failures yesterday, and they're because of the
hard-coded NAVIGATION_TYPE_NEW_SUBFRAME hack we have for OOPIFs.  When we treat
auto-subframe navigations as NEW_SUBFRAME, there's no existing frame entry to
clone and replace, so my new code fails.

I think we'll need Nate's fix to get past that issue.  The rest of the CL is
essentially ready, though.  Can you review it now?  I'll just run the try jobs
again when Nate's fix is in.

[Avi (use Gerrit), 2015-05-19 16:34:13]

Fingers crossed. This LGTM but I feel weird giving my OK before we have green
bots, even if it's not your code failing.

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

https://codereview.chromium.org/1143653002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1118: // We
should create a new NavigationEntry with the same main frame URL.
My preferred phrasing would be "We should have created..." but I don't really
care either way.

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

https://codereview.chromium.org/1143653002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:516: parent_node ==
root_node());
I <3 DCHECK_IMPLIES.

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

https://codereview.chromium.org/1143653002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:43: //
|frame_navigation_entry|.  Pass nullptr for both for a complete clone.
I'm pondering this comment.

1. "with copies of each node's FNEs". We're not making copies, though, we're
sharing the the FNEs with ref-counted pointers, right?

2. "of the tree" meaning the tree rooted at this entry?

[Charlie Reis, 2015-05-19 17:59:24]

On 2015/05/19 16:34:13, Avi wrote:
> Fingers crossed. This LGTM but I feel weird giving my OK before we have green
> bots, even if it's not your code failing.

Thanks.  I'll give you a heads up once the bots are green (especially if any
further changes are required here).

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

https://codereview.chromium.org/1143653002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:1118: // We
should create a new NavigationEntry with the same main frame URL.
On 2015/05/19 16:34:13, Avi wrote:
> My preferred phrasing would be "We should have created..." but I don't really
> care either way.

Done.

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

https://codereview.chromium.org/1143653002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:43: //
|frame_navigation_entry|.  Pass nullptr for both for a complete clone.
On 2015/05/19 16:34:13, Avi wrote:
> I'm pondering this comment.
> 
> 1. "with copies of each node's FNEs". We're not making copies, though, we're
> sharing the the FNEs with ref-counted pointers, right?

We're not sharing FNEs just yet (hence the TODO below).  I decided to postpone
that to another CL since it affects cloning tabs, session restore, etc.  Easier
to just have each tree have its own set of FNEs for the moment, and then we can
start sharing them.

> 2. "of the tree" meaning the tree rooted at this entry?

Agreed, this part is confusing, since the comment is on TreeNode but is phrased
as the whole tree.  I've rephrased it.

[Avi (use Gerrit), 2015-05-19 19:36:05]

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

https://codereview.chromium.org/1143653002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:47:
NavigationEntryImpl::TreeNode* NavigationEntryImpl::TreeNode::CloneAndReplace(
Oh! If we're returning a new object, make the return value here a scoped_ptr so
that the ownership is clear from the function declaration.

https://codereview.chromium.org/1143653002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:62:
copy->children.push_back(
... and you can push_back a scoped_ptr onto a ScopedVector, so that works great.

[Charlie Reis, 2015-05-21 20:07:00]

Patchset #7 (id:120001) has been deleted

[Charlie Reis, 2015-05-21 20:09:33]

Good idea, thanks.  New patch with scoped_ptr up, and I'm separately
investigating a fix for https://crbug.com/464014 to fix the tests.

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

https://codereview.chromium.org/1143653002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:47:
NavigationEntryImpl::TreeNode* NavigationEntryImpl::TreeNode::CloneAndReplace(
On 2015/05/19 19:36:05, Avi wrote:
> Oh! If we're returning a new object, make the return value here a scoped_ptr
so
> that the ownership is clear from the function declaration.

Done.

https://codereview.chromium.org/1143653002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:62:
copy->children.push_back(
On 2015/05/19 19:36:05, Avi wrote:
> ... and you can push_back a scoped_ptr onto a ScopedVector, so that works
great.

Done.

[Avi (use Gerrit), 2015-05-21 21:01:56]

Like it. LGTM

[Charlie Reis, 2015-06-11 18:50:14]

Patchset #9 (id:180001) has been deleted

[Charlie Reis, 2015-06-11 19:00:49]

Avi, can you take another look, now that the CLs blocking this have landed? 
I'll run try jobs as soon as r196964 gets picked up in a Blink roll.  Thanks!

Changes since patch 7:

I re-enabled NavigationControllerBrowserTest's
NavigationTypeClassification_NewAndAutoSubframe since
https://codereview.chromium.org/1173513002/ has landed.

I restructured the FrameNavigationEntry tests in
NavigationControllerImplBrowserTest.  They cover the same cases but we now have
separate _AutoSubframe and _NewSubframe tests, instead of a single
_NewAndAutoSubframe test.

The NavigationEntryImpl::AddOrUpdateFrameEntry diff is a bit simpler because I
already landed MatchesFrame and FindFrameEntry in earlier CLs.

[Avi (use Gerrit), 2015-06-11 19:19:52]

LGTM

[Charlie Reis, 2015-06-13 05:03:20]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-06-13 05:03:21]

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/1143653002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2015-06-13 05:03:35]

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

[commit-bot: I haz the power, 2015-06-13 06:42:50]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-06-13 06:43:42]

Patchset 11 (id:??) landed as
https://crrev.com/96fc55084b605d88fca0fffa667e0b547cb80d9b
Cr-Commit-Position: refs/heads/master@{#334331}