Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.

Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3JXcWm8_zWBhEjaJtI

BUG=508850, 552253
Committed: https://crrev.com/35f35afd20ceee979839d2ad2689c1fd700743cc
Cr-Commit-Position: refs/heads/master@{#362385}

[carlosk, 2015-11-23 10:56:18]

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

[carlosk, 2015-11-23 10:56:19]

Oh well... The reland #1 [1] was reverted (now I'm glad I began counting). This
initial patch set is just a copy of what had landed.

The test failure that caused the revert is likely related to simultaneous
navigations (*) which triggered a DCHECK I added. I'll remove this DCHECK and
take a look at the code in search for more cases where it might cause issues
with simultaneous navigations.

(*): We allow two navigations to happen simultaneously on the same frame as long
as one is same-site and the other is cross-site.

[1] https://codereview.chromium.org/1426403006/

[carlosk, 2015-11-23 14:06:16]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org, nasko@chromium.org, creis@chromium.org,
dbeam@chromium.org
==========

[carlosk, 2015-11-23 14:07:22]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org, nasko@chromium.org, creis@chromium.org,
dbeam@chromium.org
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org,nasko@chromium.org,creis@chromium.org,dbeam@chromium.org
==========

[carlosk, 2015-11-23 14:07:45]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org,nasko@chromium.org,creis@chromium.org,dbeam@chromium.org
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org, nasko@chromium.org, creis@chromium.org,
dbeam@chromium.org
==========

[carlosk, 2015-11-23 15:27:40]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
TBR=clamy@chromium.org, nasko@chromium.org, creis@chromium.org,
dbeam@chromium.org
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
==========

[carlosk, 2015-11-23 16:20:58]

Patchset #4 (id:60001) has been deleted

[carlosk, 2015-11-25 16:57:48]

These changes should support simultaneous navigations properly. I added 4 new
RFHM unit tests to verify that is indeed the case. These tests are still broken
with PlzNavigate because it handled simultaneous navigations differently (it has
"less" of them) but I'd prefer to fix them later.

The main change on non-test code is that I extracted logic from CommitPending
into a new method, CommitPendingWebUI, which exclusively commits the pending
WebUI on the current RFH. Now each of them is very specific on what they: the
old one is used when the pending/speculative RFH commits and the new one when
the current RFH does.

I also fixed an issue in CommitPendingIfNecessary: if two simultaneous
navigation are ongoing and the same-site one commits with a WebUI, it would not
have called CommitPending. Now it does call the new CommitPendingWebUI.

Finally, I'm sorry but I mixed up my changes with a rebase at some point.
Luckily the files I touched are completely different from the ones changed by
the rebase, so when diff-ing between patch sets 1 and 5 you can focus
exclusively on:
- content/browser/frame_host/render_frame_host_manager.h
- content/browser/frame_host/render_frame_host_manager.cc
- content/browser/frame_host/render_frame_host_manager_unittest.cc

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:702:
CommitPendingWebUI();
At this point we have a same-site navigation committing while there is a
simultaneous cross-site navigation ongoing. Previously it wouldn't commit a
possibly existing pending WebUI of the current RFH but now it will.

[nasko, 2015-11-25 22:15:50]

Thanks for the added tests, they helped a lot in understanding what is going on!
Nits and one request to refactor how the helper functions are done in the unit
tests.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:702:
CommitPendingWebUI();
On 2015/11/25 16:57:48, carlosk wrote:
> At this point we have a same-site navigation committing while there is a
> simultaneous cross-site navigation ongoing. Previously it wouldn't commit a
> possibly existing pending WebUI of the current RFH but now it will.

Let's put this in a comment. It isn't clear by just reading the code, especially
since we continue on to cancel navigations.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2767:
RenderFrameHostImpl*& host1,
This is a strange parameter convention and disallowed by the style guide. Why
not use **? Or better - host1, manager, web_ui can be obtained before the call
to this method, so they don't need to be parameters, right? Then we can just
return one parameter out - host2.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2800: // Note:
simultaneous navigations are weird: there are two ongoing
nit: The note is a sentence, so it should start with capital letter.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2856:
RenderFrameHostImpl*& host1,
Same comment as the other method.

[Charlie Reis, 2015-11-26 07:02:51]

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:653: // Makes the pending
WebUI on the current RenderFrameHost to being active.
nit: s/to being active/active/

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:659: // cross-site
commit.
nit: "cross-site commit" probably isn't accurate.  We don't do RFH swaps for
most renderer-initiated cross-site commits.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2767:
RenderFrameHostImpl*& host1,
On 2015/11/25 22:15:50, nasko wrote:
> This is a strange parameter convention and disallowed by the style guide. Why
> not use **? Or better - host1, manager, web_ui can be obtained before the call
> to this method, so they don't need to be parameters, right? Then we can just
> return one parameter out - host2.

Agreed that this is awkward.  You might consider using a lambda, like
SitePerProcessBrowserTest.DocumentActiveElement?

[carlosk, 2015-11-26 10:58:03]

Thanks.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:702:
CommitPendingWebUI();
On 2015/11/25 22:15:50, nasko wrote:
> On 2015/11/25 16:57:48, carlosk wrote:
> > At this point we have a same-site navigation committing while there is a
> > simultaneous cross-site navigation ongoing. Previously it wouldn't commit a
> > possibly existing pending WebUI of the current RFH but now it will.
> 
> Let's put this in a comment. It isn't clear by just reading the code,
especially
> since we continue on to cancel navigations.

Done.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:653: // Makes the pending
WebUI on the current RenderFrameHost to being active.
On 2015/11/26 07:02:51, Charlie Reis wrote:
> nit: s/to being active/active/

Done.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:659: // cross-site
commit.
On 2015/11/26 07:02:51, Charlie Reis wrote:
> nit: "cross-site commit" probably isn't accurate.  We don't do RFH swaps for
> most renderer-initiated cross-site commits.

Done.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2767:
RenderFrameHostImpl*& host1,
On 2015/11/26 07:02:51, Charlie Reis wrote:
> On 2015/11/25 22:15:50, nasko wrote:
> > This is a strange parameter convention and disallowed by the style guide.
Why
> > not use **? Or better - host1, manager, web_ui can be obtained before the
call
> > to this method, so they don't need to be parameters, right? Then we can just
> > return one parameter out - host2.
> 
> Agreed that this is awkward.  You might consider using a lambda, like
> SitePerProcessBrowserTest.DocumentActiveElement?

Done. And lambdas are sweet! :)

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2800: // Note:
simultaneous navigations are weird: there are two ongoing
On 2015/11/25 22:15:50, nasko wrote:
> nit: The note is a sentence, so it should start with capital letter.

Done.

https://codereview.chromium.org/1472703004/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2856:
RenderFrameHostImpl*& host1,
On 2015/11/25 22:15:50, nasko wrote:
> Same comment as the other method.

Done.

[nasko, 2015-11-30 16:57:49]

Just one question on the unit test, otherwise I think it is good.

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3069:
EXPECT_FALSE(GetPendingFrameHost(manager));
Why do we have no speculative RFH before we've committed?

[carlosk, 2015-11-30 19:54:16]

Thanks.

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3069:
EXPECT_FALSE(GetPendingFrameHost(manager));
On 2015/11/30 16:57:49, nasko wrote:
> Why do we have no speculative RFH before we've committed?

We never have one in this navigation as it is a same-site. The current RFH is
being reused.

[Charlie Reis, 2015-11-30 23:52:54]

LGTM if Nasko's happy.

[nasko, 2015-12-01 00:52:14]

LGTM

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1472703004/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:3069:
EXPECT_FALSE(GetPendingFrameHost(manager));
On 2015/11/30 19:54:16, carlosk wrote:
> On 2015/11/30 16:57:49, nasko wrote:
> > Why do we have no speculative RFH before we've committed?
> 
> We never have one in this navigation as it is a same-site. The current RFH is
> being reused.

D'oh! Yes indeed!

[carlosk, 2015-12-01 08:47:14]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2015-12-01 08:48:06]

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

[commit-bot: I haz the power, 2015-12-01 10:55:47]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
==========

[commit-bot: I haz the power, 2015-12-01 10:55:48]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-12-01 10:56:36]

Description was changed from

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253
==========

to

==========
Reland #2 of: Move WebUI ownership from the RenderFrameHostManager to the
RenderFrameHost.
(Revert: https://codereview.chromium.org/1454883006)

This change refactors the current ownership of the WebUI instance,
moving it from the RenderFrameHostManager to the RenderFrameHost which
is directly associated with the WebUI.

The design document for this change is here:
https://docs.google.com/a/chromium.org/document/d/1jrge5ulZ1YtqSR3WkNXzrtxhd3...

BUG=508850, 552253

Committed: https://crrev.com/35f35afd20ceee979839d2ad2689c1fd700743cc
Cr-Commit-Position: refs/heads/master@{#362385}
==========

[commit-bot: I haz the power, 2015-12-01 10:56:37]

Patchset 6 (id:??) landed as
https://crrev.com/35f35afd20ceee979839d2ad2689c1fd700743cc
Cr-Commit-Position: refs/heads/master@{#362385}