PlzNavigate: change behavior when navigating with a non-live RFH

PlzNavigate: change behavior when navigating with a non-live RFH.

To fix a few tests the current behavior when navigating from a non-live RFH was changed so that the new, speculative one is immediately set as active matching what is done in RenderFrameHostManager::UpdateStateForNavigate. This replaced the behavior of simply bringing the current RFH back to life.

With this change 6 tests are fixed for PlzNavigate:

    RenderFrameHostManagerTest.WebUI
    WebContentsImplTest.NTPViewSource
    RenderFrameHostManagerTest.NewTabPageProcesses
    WebContentsImplTest.CrossSiteBoundariesAfterCrash
    WebContentsImplTest.CrossSiteComparesAgainstCurrentPage
    WebContentsImplTest.ShowInterstitialFromBrowserNewNavigationProceed

BUG=439423
Committed: https://crrev.com/085854f7049b11ce205a88427bbfc6abdbe021e7
Cr-Commit-Position: refs/heads/master@{#314543}

[carlosk, 2015-02-02 10:17:02]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, fdegans@chromium.org, nasko@chromium.org

[carlosk, 2015-02-02 10:17:02]

nasko@, clamy@: PTAL.

fdegans@: FYI.

Simple but significant change (IMO) as it replaces a behavior ported from the
current navigation implementation -- bringing the active RFH back to live if
dead -- with another similar one -- immediately replacing a dead RFH with the
speculative one -- as they can't co-exist under PlzNavigate. More details in the
CL description.

[clamy, 2015-02-02 10:43:41]

Thanks! I was a bit concerned at first with the security implications of the
renderer being swapped in there, but it seems to be what the current
implementation is doing in UpdateStateForNavigate, so lgtm.

[clamy, 2015-02-02 10:46:44]

Could you also rename the issue:
PlzNavigate: change behavior when navigating with a non-live RFH

And in the description:
Remove the last sentence and replace with:
This matches what is done in RenderFrameHostManager::UpdateStateForNavigate.

Thanks.

[carlosk, 2015-02-02 10:58:19]

On 2015/02/02 10:46:44, clamy wrote:
> Could you also rename the issue:
> PlzNavigate: change behavior when navigating with a non-live RFH
> 
> And in the description:
> Remove the last sentence and replace with:
> This matches what is done in RenderFrameHostManager::UpdateStateForNavigate.
> 
> Thanks.

Done (a little differently). Thanks.

[nasko, 2015-02-02 18:32:31]

LGTM

[carlosk, 2015-02-02 19:21:20]

A new PS is up. Sorry about the late change but I just wanted to improve a bit
one of the tests fixed with this change.

[carlosk, 2015-02-02 19:48:54]

Yet another CL not with the WebUI verification reinstated (I read too quickly
and replaced with a RFH check). Thanks to nasko@ for quickly picking that
mistake.

[nasko, 2015-02-02 21:20:40]

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:240: WebUIImpl*
speculative_web_ui() const {
nit: Since this is used only in tests, let's add a suffix to indicate it.

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:241: return
should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
Should the regular web_ui_ be returned in a method asking for the speculative?

[carlosk, 2015-02-03 10:06:27]

Thanks.

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:240: WebUIImpl*
speculative_web_ui() const {
On 2015/02/02 21:20:40, nasko wrote:
> nit: Since this is used only in tests, let's add a suffix to indicate it.

Done.

https://codereview.chromium.org/892123002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:241: return
should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
On 2015/02/02 21:20:40, nasko wrote:
> Should the regular web_ui_ be returned in a method asking for the speculative?

Yes when |should_reuse_web_ui_| is true. It is the exact equivalent of how
|pending_web_ui()| just above works. The difference from the speculative
implementation to the pending one is that when the former has to reuse the
current WebUI it sets this boolean to true, while the latter makes a weak
pointer copy of it into |pending_and_current_web_ui_|.

[clamy, 2015-02-03 10:59:11]

https://chromiumcodereview.appspot.com/892123002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/892123002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:241: return
should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
On 2015/02/03 10:06:26, carlosk wrote:
> On 2015/02/02 21:20:40, nasko wrote:
> > Should the regular web_ui_ be returned in a method asking for the
speculative?
> 
> Yes when |should_reuse_web_ui_| is true. It is the exact equivalent of how
> |pending_web_ui()| just above works. The difference from the speculative
> implementation to the pending one is that when the former has to reuse the
> current WebUI it sets this boolean to true, while the latter makes a weak
> pointer copy of it into |pending_and_current_web_ui_|.

Maybe you could precise this in the comment?

[carlosk, 2015-02-04 11:02:47]

Patchset #5 (id:80001) has been deleted

[carlosk, 2015-02-04 11:03:25]

Thanks!

https://chromiumcodereview.appspot.com/892123002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/892123002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:241: return
should_reuse_web_ui_ ? web_ui_.get() : speculative_web_ui_.get();
On 2015/02/03 10:59:11, clamy wrote:
> On 2015/02/03 10:06:26, carlosk wrote:
> > On 2015/02/02 21:20:40, nasko wrote:
> > > Should the regular web_ui_ be returned in a method asking for the
> speculative?
> > 
> > Yes when |should_reuse_web_ui_| is true. It is the exact equivalent of how
> > |pending_web_ui()| just above works. The difference from the speculative
> > implementation to the pending one is that when the former has to reuse the
> > current WebUI it sets this boolean to true, while the latter makes a weak
> > pointer copy of it into |pending_and_current_web_ui_|.
> 
> Maybe you could precise this in the comment?

Done.

[carlosk, 2015-02-04 11:03:31]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2015-02-04 11:03:49]

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

[commit-bot: I haz the power, 2015-02-04 12:08:17]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-02-04 12:09:14]

Patchset 5 (id:??) landed as
https://crrev.com/085854f7049b11ce205a88427bbfc6abdbe021e7
Cr-Commit-Position: refs/heads/master@{#314543}