Move ownership of source SiteInstance to the FrameNavigationEntry.

Move ownership of source SiteInstance to the FrameNavigationEntry.

BUG=606996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[nasko, 2016-05-04 15:13:47]

Description was changed from

==========
Move ownership of source SiteInstance to the FrameNavigationEntry.

BUG=606996
==========

to

==========
Move ownership of source SiteInstance to the FrameNavigationEntry.

BUG=606996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-05-04 16:20:52]

Patchset #1 (id:1) has been deleted

[nasko, 2016-05-04 16:49:31]

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

[nasko, 2016-05-04 16:49:32]

Hey Charlie,
Can you review this CL for me? It moves the source SiteInstance from
NavigationEntry to FrameNavigationEntry, so we don't end up conflicting for
multiple concurrent subframe navigations.
Thanks in advance!

[dcheng, 2016-05-04 17:01:45]

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:50: SiteInstanceImpl*
source_site_instance,
Random aside: it would be kind of nice if ownership taking parameters were
passed by scoped_refptr. That would make the downcasting quite a bit uglier
though.

[Charlie Reis, 2016-05-04 23:03:01]

This is great-- thanks!

Did we determine that it's not really visible to tests, given that only one
frame is going through RequestOpenURL or RequestTransferURL at a time?  (Just
curious if there's a test we could write to verify this.)

A few suggestions below.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:51: SiteInstanceImpl*
site_instance,
nit: Please list source_site_instance after site_instance, since site_instance
is more important and always present.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:91: // frame that initiated
the navigation.
nit: Let's mention that it is cleared after commit and only present on
renderer-initiated navigations, so people don't assume they can use it
otherwise.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:142: // This member is
cleared at commit time and not persisted.
nit: and is not

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:752:
entry->root_node()->frame_entry->set_source_site_instance(
Hmm.  I think I'd prefer to have a method on NavigationEntryImpl for this after
all, because it's unfortunate to expose TreeNode outside NavigationEntry.  I
kind of want to make that a private detail of NavigationEntry-- it looks like
root_node() is only used in tests these days.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:753:
static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
Just curious-- was there a reason not to move this parameter into
CreateNavigationEntry (and the FrameNavigationEntry constructor)?  Maybe that
had too many call sites?

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:895: // Update the
frame-specific PageState.
This comment belongs inside the conditional, since the else branch does a
full-page PageState rather than a frame-specifc one.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:726:
frame_entry->set_source_site_instance(source_site_instance);
I think we probably want to put this in the FrameNavigationEntry constructor if
possible, so that we don't forget to call this in random cases.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:179: // pieces of
non-persisted state, as documented on the members below.
Mention what |frame_entry| is, and that it can be null.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:200: SiteInstanceImpl*
source_site_instance,
As in FNE, let's list site_instance before source_site_instance.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:810: new
FrameNavigationEntry(node->unique_name(), -1, -1, nullptr, dest_url,
We need it here, too, in case AddOrUpdateFrameEntry fails.  (Making it a
constructor param would catch this case.)

[nasko, 2016-05-05 17:50:05]

Patchset #2 (id:40001) has been deleted

[nasko, 2016-05-05 17:51:33]

Review comments addressed. 

As far as testing goes, I haven't had a chance to think of creative ways of
testing this yet. This is just the refactor for now.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/frame_navigation_entry.h (right):

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:50: SiteInstanceImpl*
source_site_instance,
On 2016/05/04 17:01:45, dcheng wrote:
> Random aside: it would be kind of nice if ownership taking parameters were
> passed by scoped_refptr. That would make the downcasting quite a bit uglier
> though.

In this case we won't need to downcast, would we? It is already the Impl type.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:51: SiteInstanceImpl*
site_instance,
On 2016/05/04 23:03:01, Charlie Reis wrote:
> nit: Please list source_site_instance after site_instance, since site_instance
> is more important and always present.

Done.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:91: // frame that initiated
the navigation.
On 2016/05/04 23:03:01, Charlie Reis wrote:
> nit: Let's mention that it is cleared after commit and only present on
> renderer-initiated navigations, so people don't assume they can use it
> otherwise.

Done.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.h:142: // This member is
cleared at commit time and not persisted.
On 2016/05/04 23:03:01, Charlie Reis wrote:
> nit: and is not

Done.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:752:
entry->root_node()->frame_entry->set_source_site_instance(
On 2016/05/04 23:03:01, Charlie Reis wrote:
> Hmm.  I think I'd prefer to have a method on NavigationEntryImpl for this
after
> all, because it's unfortunate to expose TreeNode outside NavigationEntry.  I
> kind of want to make that a private detail of NavigationEntry-- it looks like
> root_node() is only used in tests these days.

Done.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:753:
static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
On 2016/05/04 23:03:01, Charlie Reis wrote:
> Just curious-- was there a reason not to move this parameter into
> CreateNavigationEntry (and the FrameNavigationEntry constructor)?  Maybe that
> had too many call sites?

It is content/public/ API. I don't think it is worthwhile adding at this point,
since I haven't seen anyone outside of content creating such a NavigationEntry.
There is usage of source_site_instance outside of content/, but it is passed
through the various params structs.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:895: // Update the
frame-specific PageState.
On 2016/05/04 23:03:01, Charlie Reis wrote:
> This comment belongs inside the conditional, since the else branch does a
> full-page PageState rather than a frame-specifc one.

Done.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:726:
frame_entry->set_source_site_instance(source_site_instance);
On 2016/05/04 23:03:01, Charlie Reis wrote:
> I think we probably want to put this in the FrameNavigationEntry constructor
if
> possible, so that we don't forget to call this in random cases.

Done.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:179: // pieces of
non-persisted state, as documented on the members below.
On 2016/05/04 23:03:01, Charlie Reis wrote:
> Mention what |frame_entry| is, and that it can be null.

Done.

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:200: SiteInstanceImpl*
source_site_instance,
On 2016/05/04 23:03:01, Charlie Reis wrote:
> As in FNE, let's list site_instance before source_site_instance.

Done.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:810: new
FrameNavigationEntry(node->unique_name(), -1, -1, nullptr, dest_url,
On 2016/05/04 23:03:01, Charlie Reis wrote:
> We need it here, too, in case AddOrUpdateFrameEntry fails.  (Making it a
> constructor param would catch this case.)

Done.

[Charlie Reis, 2016-05-05 22:20:00]

Thanks!  LGTM with nits.

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

https://codereview.chromium.org/1944013003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:753:
static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
On 2016/05/05 17:51:33, nasko (slow) wrote:
> On 2016/05/04 23:03:01, Charlie Reis wrote:
> > Just curious-- was there a reason not to move this parameter into
> > CreateNavigationEntry (and the FrameNavigationEntry constructor)?  Maybe
that
> > had too many call sites?
> 
> It is content/public/ API. I don't think it is worthwhile adding at this
point,
> since I haven't seen anyone outside of content creating such a
NavigationEntry.
> There is usage of source_site_instance outside of content/, but it is passed
> through the various params structs.

Acknowledged.

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:41: source_site_instance_,
url_, referrer_, page_state_,
Hmm.  NavigationEntryImpl::CloneAndReplace omits any fields that get cleared at
commit time, and I think that's probably the right thing to do here for
FrameNavigationEntry::Clone().  

We're losing the formatting from NavigationEntryImpl::CloneAndReplace where we
could put a comment for any omitted field (in order), but here we can just pass
null for the ones that get omitted.  Maybe we can preface this UpdateEntry call
with:
// Omit any fields cleared in ResetForCommit.

(I think we do need to have listed in UpdateEntry below, so that it gets set via
AddOrUpdateFrameEntry on transfers.)

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:50: SiteInstanceImpl*
site_instance,
Tangent for a different CL: Perhaps we should change this to scoped_refptr to be
consistent?

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:676:
frame_entry->set_source_site_instance(nullptr);
Yes, this seems fine for now.  If we end up with several things getting cleared
on frame_entry, maybe we can add a FrameNavigationEntry::ResetForCommit.

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:181: // the navigation. It
can be null if subframe navigation entries are not
I wish it were as simple as "if subframe navigation entries are not in use."  :)

It's also null in all the cases mentioned in https://crbug.com/608402, and
possibly others.  We probably just want to say that it can be null without
saying when, or that it's when we end up with no corresponding
FrameNavigationEntry for a commit.

[nasko, 2016-05-06 00:13:42]

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:41: source_site_instance_,
url_, referrer_, page_state_,
On 2016/05/05 22:20:00, Charlie Reis wrote:
> Hmm.  NavigationEntryImpl::CloneAndReplace omits any fields that get cleared
at
> commit time, and I think that's probably the right thing to do here for
> FrameNavigationEntry::Clone().  
> 
> We're losing the formatting from NavigationEntryImpl::CloneAndReplace where we
> could put a comment for any omitted field (in order), but here we can just
pass
> null for the ones that get omitted.  Maybe we can preface this UpdateEntry
call
> with:
> // Omit any fields cleared in ResetForCommit.
> 
> (I think we do need to have listed in UpdateEntry below, so that it gets set
via
> AddOrUpdateFrameEntry on transfers.)

Done.

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_navigation_entry.cc:50: SiteInstanceImpl*
site_instance,
On 2016/05/05 22:20:00, Charlie Reis wrote:
> Tangent for a different CL: Perhaps we should change this to scoped_refptr to
be
> consistent?

My preference is different CL, as I want to keep this one to be move only CL.

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:676:
frame_entry->set_source_site_instance(nullptr);
On 2016/05/05 22:20:00, Charlie Reis wrote:
> Yes, this seems fine for now.  If we end up with several things getting
cleared
> on frame_entry, maybe we can add a FrameNavigationEntry::ResetForCommit.

Acknowledged.

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

https://codereview.chromium.org/1944013003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:181: // the navigation. It
can be null if subframe navigation entries are not
On 2016/05/05 22:20:00, Charlie Reis wrote:
> I wish it were as simple as "if subframe navigation entries are not in use." 
:)
> 
> It's also null in all the cases mentioned in https://crbug.com/608402, and
> possibly others.  We probably just want to say that it can be null without
> saying when, or that it's when we end up with no corresponding
> FrameNavigationEntry for a commit.

Done.

[nasko, 2016-05-06 15:43:56]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-05-06 15:43:57]

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/1944013003/#ps80001
(title: "Another round of review comments addressed.")

[commit-bot: I haz the power, 2016-05-06 15:44:04]

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

[commit-bot: I haz the power, 2016-05-06 17:01:22]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-05-06 17:02:31]

Description was changed from

==========
Move ownership of source SiteInstance to the FrameNavigationEntry.

BUG=606996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move ownership of source SiteInstance to the FrameNavigationEntry.

BUG=606996
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-05-06 17:02:32]

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