Remove GetPendingSiteInstance from NavigationControllerDelegate.

Remove GetPendingSiteInstance from NavigationControllerDelegate.

It was previously needed to use a SiteInstance to locate a commit but this is no
longer true. This also moves the restore_type reset until commit time and changes
tests expectations accordingly.

BUG=369661
Committed: https://crrev.com/9caf66afae991a65c8cfa76322a15b8049365026
Cr-Commit-Position: refs/heads/master@{#341192}

[Charlie Reis, 2015-07-29 23:58:26]

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

[Charlie Reis, 2015-07-29 23:58:27]

This CL goes further than I expected-- it doesn't seem safe to leave the restore
type set after commit, and I don't want to lose some of those DCHECKs.

I'm ok with the premise, though.  I think we can probably get by without setting
the SiteInstance on the pending entry, as we discussed.

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

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:819:
DCHECK(pending_entry_index_ == -1 || pending_entry_->site_instance());
I'd prefer not to remove this DCHECK.  Can we add a condition allowing restored
entries to have a null SiteInstance instead?

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1161:
entry->site_instance() == rfh->GetSiteInstance());
This definitely needs to be kept-- a NavigationEntry should not change its
SiteInstance once it has one.  I've been thinking about upgrading it to a CHECK,
but I think my location.replace CL has to land first.

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1733:
pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);
When will this be cleared?  We don't want to leave the restore type set forever.

If we're not catching that today, we should add a test for it.  Maybe it can be
cleared in RendererDidNavigate after the SiteInstance DCHECK (around line 819)?

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

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2803:
our_controller.GetEntryAtIndex(0)->restore_type());
We still need this to be set to RESTORE_NONE at some point, either here or at
commit time.

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2834:
our_controller.GetEntryAtIndex(0)->restore_type());
We definitely need this check.

[Fabrice (no longer in Chrome), 2015-07-30 15:59:27]

Thanks for the review!
I uploaded a new patch set moving clearing the restore type to commit time.

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

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:819:
DCHECK(pending_entry_index_ == -1 || pending_entry_->site_instance());
On 2015/07/29 23:58:27, Charlie Reis wrote:
> I'd prefer not to remove this DCHECK.  Can we add a condition allowing
restored
> entries to have a null SiteInstance instead?

Done.
I moved the restore_type here too.

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1161:
entry->site_instance() == rfh->GetSiteInstance());
On 2015/07/29 23:58:27, Charlie Reis wrote:
> This definitely needs to be kept-- a NavigationEntry should not change its
> SiteInstance once it has one.  I've been thinking about upgrading it to a
CHECK,
> but I think my location.replace CL has to land first.
Done.

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1733:
pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);
On 2015/07/29 23:58:27, Charlie Reis wrote:
> When will this be cleared?  We don't want to leave the restore type set
forever.
> 
> If we're not catching that today, we should add a test for it.  Maybe it can
be
> cleared in RendererDidNavigate after the SiteInstance DCHECK (around line
819)?

We are catching it, I just removed the wrong checks in the tests. I restored
these and moved the restore_type after the DCHECK, as you suggested. Thanks!

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

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2803:
our_controller.GetEntryAtIndex(0)->restore_type());
On 2015/07/29 23:58:27, Charlie Reis wrote:
> We still need this to be set to RESTORE_NONE at some point, either here or at
> commit time.

I'm removing the check here, and restoring the check after commit under.

https://codereview.chromium.org/1268453003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:2834:
our_controller.GetEntryAtIndex(0)->restore_type());
On 2015/07/29 23:58:27, Charlie Reis wrote:
> We definitely need this check.

Yes, sorry. Done.

[Charlie Reis, 2015-07-30 16:55:14]

Thanks, that's better.  LGTM with nits.

Let's also update the CL description to remove "This also removes assorted
DCHECKS" and mention moving the restore_type reset until commit time.

We can use 369661 as the bug number, since that's what made this CL possible.

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

https://codereview.chromium.org/1268453003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:823:
pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE)
Let's simplify these to:

DCHECK(!pending_entry_ || pending_entry_->site_instance() || ...restore...);
if (pending_entry_ && pending_entry_->restore_type() != ...)
  ...set_restore_type...


Reasoning:
1) pending_entry_index_ isn't quite sufficient, even in the old check. 
pending_entry_ can be set even when the index is -1, which corresponds to a new
navigation.  Those won't be session restore cases, but they should have a
SiteInstance.
2) No need for the "== nullptr", since most of the code around here doesn't use
that (e.g., line 808, 871, etc).
3) The SiteInstance check in the if block isn't really necessary (though I agree
we had it before).  If it's a restore, we should clear the restore type, whether
it has a SiteInstance already or not.

[Fabrice (no longer in Chrome), 2015-07-30 17:18:41]

Thanks a lot!
New patchset, I'm a bit concerned about the windows test failure, which seems to
be an actual failure for once.

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

https://codereview.chromium.org/1268453003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:823:
pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE)
On 2015/07/30 16:55:14, Charlie Reis wrote:
> Let's simplify these to:
> 
> DCHECK(!pending_entry_ || pending_entry_->site_instance() || ...restore...);
> if (pending_entry_ && pending_entry_->restore_type() != ...)
>   ...set_restore_type...
> 
> 
> Reasoning:
> 1) pending_entry_index_ isn't quite sufficient, even in the old check. 
> pending_entry_ can be set even when the index is -1, which corresponds to a
new
> navigation.  Those won't be session restore cases, but they should have a
> SiteInstance.
> 2) No need for the "== nullptr", since most of the code around here doesn't
use
> that (e.g., line 808, 871, etc).
> 3) The SiteInstance check in the if block isn't really necessary (though I
agree
> we had it before).  If it's a restore, we should clear the restore type,
whether
> it has a SiteInstance already or not.

Done.
I find the explicit nullptr comparison to be easier to read and it seems some of
the more recent code and tests have adopted this - though I agree it's
semantically identical.

[Fabrice (no longer in Chrome), 2015-07-30 17:22:37]

The CQ bit was checked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-07-30 17:22:38]

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/1268453003/#ps100001
(title: "Review comments Mk2")

[commit-bot: I haz the power, 2015-07-30 17:23:18]

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

[Charlie Reis, 2015-07-30 17:24:32]

On 2015/07/30 17:18:41, Fabrice wrote:
> Thanks a lot!
> New patchset, I'm a bit concerned about the windows test failure, which seems
to
> be an actual failure for once.
> 

Does it repro for you locally with your patch and not without?  If so, we should
investigate.  Otherwise, let's see what the CQ says.

[Fabrice (no longer in Chrome), 2015-07-30 17:28:57]

The CQ bit was unchecked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-07-30 18:55:25]

The CQ bit was checked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-07-30 18:55:25]

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/1268453003/#ps120001
(title: "Restore pending_entry_index_")

[commit-bot: I haz the power, 2015-07-30 18:55:57]

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

[commit-bot: I haz the power, 2015-07-30 20:17:59]

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

[commit-bot: I haz the power, 2015-07-30 20:18:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Fabrice (no longer in Chrome), 2015-07-30 20:24:06]

The CQ bit was checked by fdegans@chromium.org

[commit-bot: I haz the power, 2015-07-30 20:24:37]

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

[commit-bot: I haz the power, 2015-07-30 21:10:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-30 21:11:21]

Patchset 7 (id:??) landed as
https://crrev.com/9caf66afae991a65c8cfa76322a15b8049365026
Cr-Commit-Position: refs/heads/master@{#341192}