PlzNavigate: Fix WebUI creation logic for both cross and same site navigation.

This CL is basically a 3 fixes in 1. I started off fixing the fact that
WebUIs were not properly being created when reusing the current
RenderFrameHost. But while doing that I found an issue in
RenderFrameHostManager::CreateSpeculativeRenderFrameHost where instead
of setting an instance member I was mistakenly shadowing it with a
same-named function member.

This last fix caused another test to crash because the WebUI reuse logic
was incorrect. Instead of checking if it should be reused when the
navigation SiteInstance matched the current one, it was being done when
they were different (what doesn't even makes sense).

Due to these changes I also had to update how RFHM::CleanUpNavigation
and UnsetSpeculativeRenderFrameHost work,so that the speculative WebUI
member is always cleaned up even if a speculative RFH does not exist.

Also renamed the speculative WebUI getter as it's not test-only anymore.

With these changes these tests were fixed when PlzNavigate is enabled:
    RenderFrameHostManagerTest.EnableWebUIWithSwappedOutOpener
    RenderFrameHostManagerTest.WebUIInNewTab

BUG=439423
Committed: https://crrev.com/1ae0930d2a6f0c8ecfe6bf60bbd2140303b122a2
Cr-Commit-Position: refs/heads/master@{#315006}

[carlosk, 2015-02-05 16:09:05]

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

[carlosk, 2015-02-05 16:09:05]

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

[carlosk, 2015-02-05 16:09:06]

nasko, clamy: PTAL.

fdegans: FYI.

Please find the CL explanations in the description and the few code comments.

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1738: 
I moved the 2 removed DCHECKs up here because it happened either way if the next
if-block was executed or not.

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1036:
EXPECT_TRUE(manager2->current_host()->IsRenderViewLive());
Added to actually check what's mentioned in the previous comment.

[clamy, 2015-02-05 16:54:27]

Thanks!

https://chromiumcodereview.appspot.com/877343010/diff/1/content/browser/frame...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/877343010/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.cc:719: // As site
instances are the same, check if the WebUI should be reused
nit: add a . at the end of the comment.

https://chromiumcodereview.appspot.com/877343010/diff/1/content/browser/frame...
content/browser/frame_host/render_frame_host_manager.cc:1284:
CHECK_NE(old_instance, new_instance);
Could you add a CHECK(!should_reuse_web_ui_) here? I don't think we want it to
get set to true by mistake.

[carlosk, 2015-02-05 17:07:33]

Thanks.

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:719: // As site
instances are the same, check if the WebUI should be reused
On 2015/02/05 16:54:27, clamy wrote:
> nit: add a . at the end of the comment.

Done.

https://codereview.chromium.org/877343010/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_manager.cc:1284:
CHECK_NE(old_instance, new_instance);
On 2015/02/05 16:54:27, clamy wrote:
> Could you add a CHECK(!should_reuse_web_ui_) here? I don't think we want it to
> get set to true by mistake.

Done.

[clamy, 2015-02-05 17:09:52]

Thanks! Lgtm

[clamy, 2015-02-05 17:10:17]

Ah could you also prefix the issue title with PlzNavigate?

[carlosk, 2015-02-05 17:26:25]

On 2015/02/05 17:10:17, clamy wrote:
> Ah could you also prefix the issue title with PlzNavigate?

Done.

[nasko, 2015-02-06 02:18:57]

In the future, please don't combine multiple bugfixes in one CL. They should be
separate, so it is easy to follow what the problem is and how it is fixed.

LGTM with a nit

https://codereview.chromium.org/877343010/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/877343010/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:719: // As site
instances are the same, check if the WebUI should be reused.
nit: s/site instance/SiteInstances/

[carlosk, 2015-02-06 10:35:05]

New patchsets have been uploaded after l-g-t-m from
clamy@chromium.org,nasko@chromium.org

[carlosk, 2015-02-06 10:37:58]

On 2015/02/06 02:18:57, nasko wrote:
> In the future, please don't combine multiple bugfixes in one CL. They should
be
> separate, so it is easy to follow what the problem is and how it is fixed.

Acknowledged! I did a few minor modifications that could come later.

But I think my description might be exaggerating a a bit: the 3 fixes are
interconnected and I didn't want separate submissions to cause a regression in
our tests (what would happen in case these were separate commits).

https://codereview.chromium.org/877343010/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/877343010/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:719: // As site
instances are the same, check if the WebUI should be reused.
On 2015/02/06 02:18:57, nasko wrote:
> nit: s/site instance/SiteInstances/

Done.

[carlosk, 2015-02-06 10:38:04]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2015-02-06 10:39:06]

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

[commit-bot: I haz the power, 2015-02-06 12:14:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-06 12:15:40]

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