PlzNavigate: move ownership of the NavigationRequest to the FrameTreeNode

PlzNavigate: move ownership of the NavigationRequest to the FrameTreeNode

This CL moves the ownership of NavigationRequests from a map in NavigatorImpl
to the FrameTreeNode. This ensures that NavigationRequests are unique per
FrameTreeNode and are also properly deleted on destruction of the
FrameTreeNode. It also makes it easier to have PlzNavigate adapt to the
refactoring of DidStart/StopLoading happening in
https://codereview.chromium.org/1080143003/.

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

[clamy, 2015-04-15 10:29:13]

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

[clamy, 2015-04-15 11:49:53]

@nasko, fdgans: PTAL
@carlosk: FYI

This moves the NavigationRequest to the FrameTreeNode. I know in the past we
avoided putting things in the FrameTreeNode, however I feel this makes the code
simpler and leads to a clarified lifetime for the NavigationRequest. Also, it
should make it easier to have PlzNavigate work with the Start/StopLoading
refactoring. And it fixes the issue Carlos mentioned about the extra proxy being
created, since we do not explicitly do a CleanUp on deletion of the node (no
need to as the RenderFrameHostManager is getting destroyed as well).

[Fabrice (no longer in Chrome), 2015-04-15 12:44:50]

Thanks!
Overall, this looks much cleaner. Couple of comments for now.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:14: #include
"content/browser/frame_host/navigation_request.h"
nit: I think we can forward-declare here.
We are also including render_frame_host_impl.h and forward-declaring
RenderFrameHostImpl when it does not seem necessary.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:726: // we properly cancel ongoing
navigations.
Is this comment still relevant? If yes, I think it needs to be rewritten.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:795:
frame_tree_node->SetNavigationRequest(navigation_request.Pass());
With the TODO line above, this is slightly confusing.
Maybe re-add a comment to make it clear it is not related?

[nasko, 2015-04-15 20:36:04]

I agree with Fabrice that this looks cleaner. Thanks for putting this CL
together! Mostly nits.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:188: // TODO(clamy): perform the
StopLoading logic.
Why does it matter whether this is a commit or not? If we are cancelling a
navigation, shouldn't we be executing the stop loading code?

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:214: // Owns a navigation happening
in this node until it is ready to commit (it
nit: s/a navigation happening/ongoing NavigationRequest/

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:215: // will then be owned by the
current RenderFrameHost).
nit: It might not be the current RFH, but the speculative or a new one. I'm not
sure what a better wording of it is, so probably use "the RenderFrameHost"
without specifying which one.

[clamy, 2015-04-16 12:12:31]

Thanks!

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:188: // TODO(clamy): perform the
StopLoading logic.
On 2015/04/15 20:36:03, nasko wrote:
> Why does it matter whether this is a commit or not? If we are cancelling a
> navigation, shouldn't we be executing the stop loading code?

On commit we reset the navigation request because the navigation is now owned by
the RFH. However this is not a cancelation, so no cleanup should be performed,
nor should the stop loading code executed (since the RFH is still loading).

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:14: #include
"content/browser/frame_host/navigation_request.h"
On 2015/04/15 12:44:50, Fabrice wrote:
> nit: I think we can forward-declare here.
> We are also including render_frame_host_impl.h and forward-declaring
> RenderFrameHostImpl when it does not seem necessary.

Done.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:214: // Owns a navigation happening
in this node until it is ready to commit (it
On 2015/04/15 20:36:04, nasko wrote:
> nit: s/a navigation happening/ongoing NavigationRequest/

Done.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.h:215: // will then be owned by the
current RenderFrameHost).
On 2015/04/15 20:36:04, nasko wrote:
> nit: It might not be the current RFH, but the speculative or a new one. I'm
not
> sure what a better wording of it is, so probably use "the RenderFrameHost"
> without specifying which one.

Done. I've also modified the comment to clarify the lifetime of the
NavigationRequest.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:726: // we properly cancel ongoing
navigations.
On 2015/04/15 12:44:50, Fabrice wrote:
> Is this comment still relevant? If yes, I think it needs to be rewritten.

Done.

https://codereview.chromium.org/1080073004/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:795:
frame_tree_node->SetNavigationRequest(navigation_request.Pass());
On 2015/04/15 12:44:50, Fabrice wrote:
> With the TODO line above, this is slightly confusing.
> Maybe re-add a comment to make it clear it is not related?

Done.

[Fabrice (no longer in Chrome), 2015-04-16 14:01:27]

One final nit :)
lgtm

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:156: // Reset the current
navigation request. |is_commit| is true if the reset is
nit: s/Reset/Resets/

[nasko, 2015-04-16 16:02:34]

LGTM with a couple of nits.

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:186: // Upon commit the current
navigation request will be reset. There should be
nit: s/navigation request/NavigationRequest/

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:215: // be reset and the navigation
will be owned by a RenderFrameHost.
nit: s/navigation/NavigationRequest/. A navigation on its own isn't an object
that anyone owns.

[clamy, 2015-04-16 16:14:05]

Thanks!

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:186: // Upon commit the current
navigation request will be reset. There should be
On 2015/04/16 16:02:33, nasko wrote:
> nit: s/navigation request/NavigationRequest/

Done.

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:156: // Reset the current
navigation request. |is_commit| is true if the reset is
On 2015/04/16 14:01:27, Fabrice wrote:
> nit: s/Reset/Resets/

Done.

https://codereview.chromium.org/1080073004/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:215: // be reset and the navigation
will be owned by a RenderFrameHost.
On 2015/04/16 16:02:34, nasko wrote:
> nit: s/navigation/NavigationRequest/. A navigation on its own isn't an object
> that anyone owns.

I've rephrased it. We cannot use NavigationRequest here, since the
NavigationRequest is destroyed at commit time, and so it is never owned by a
RFH.

[clamy, 2015-04-16 16:22:32]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-16 16:22:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
fdegans@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1080073004/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-04-16 16:23:06]

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

[commit-bot: I haz the power, 2015-04-16 16:39:24]

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

[commit-bot: I haz the power, 2015-04-16 16:39:26]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[clamy, 2015-04-16 18:30:30]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-16 18:30:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
fdegans@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1080073004/#ps80001
(title: "Fixed compilation error")

[commit-bot: I haz the power, 2015-04-16 18:32:32]

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

[commit-bot: I haz the power, 2015-04-16 19:29:24]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-16 19:30:20]

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

[johnme, 2015-04-17 15:56:58]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1092973002/ by johnme@chromium.org.

The reason for reverting is: Tentative revert to try to fix
fast/dom/Window/new-window-opener.html on Mac, which has been crashing
consistently since
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9/builds/....