PlzNavigate: Create the speculative renderer earlier.

PlzNavigate: Create the speculative renderer earlier.

Create the speculative renderer earlier on in the navigation flow, before the
response from the beforeunload event is received, when browser-side navigation
is enabled. Existing tests were adapted to accept the earlier creation of the
renderer.

For more details:
https://docs.google.com/a/chromium.org/document/d/1Oxnu7C396FB33nZLFWzR4AOvYvhqEzsfBih3yyBKwo0

BUG=376094, 475027
Committed: https://crrev.com/c49005eba8fe85e1972d4722a521c2fc5b5e1c63
Cr-Commit-Position: refs/heads/master@{#334584}

[carlosk, 2015-06-08 17:53:13]

Patchset #1 (id:1) has been deleted

[carlosk, 2015-06-08 18:09:53]

Patchset #1 (id:20001) has been deleted

[carlosk, 2015-06-08 18:32:42]

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

[carlosk, 2015-06-08 18:32:43]

clamy@, fdegans@: PTAL.

This fixes:
 SecurityExploitBrowserTest.AttemptDuplicateRenderViewHost
 SecurityExploitBrowserTest.AttemptDuplicateRenderWidgetHost

https://codereview.chromium.org/1163303003/diff/40001/content/browser/securit...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/1163303003/diff/40001/content/browser/securit...
content/browser/security_exploit_browsertest.cc:86: pending_rvh =
wc->GetRenderManagerForTesting()->pending_render_view_host();
I didn't create an equivalent RFHM::speculative_render_view_host method there's
a TODO in this one stating it should be removed in the future.

[clamy, 2015-06-09 10:02:01]

Thanks! I think this patch can be made simpler by having the call to the RFHM
either:
1 - inside FrameTreeNode::SetNavigationRequest.
2 - inside the constructor of the NavigationRequest.

This two points in code correspond to a new navigation (browser-initiated or
render-initiated), which makes the reasoning simpler since there will only be a
single call to RFHM. Then RFHM::BeginNavigation should also be renamed
RFHM::DidCreateNavigationRequest to match what it now corresponds to.

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

https://codereview.chromium.org/1163303003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:822: if
(NavigationRequest::ShouldMakeNetworkRequest(
I find the new ordering of comments not clearer. I think having the paragraph
summary was simpler. This will be even more the case if you move the call to
RFHM inside FrameTreeNode.

[carlosk, 2015-06-09 15:33:51]

On 2015/06/09 10:02:01, clamy wrote:
> Thanks! I think this patch can be made simpler by having the call to the RFHM
> either:
> 1 - inside FrameTreeNode::SetNavigationRequest.
> 2 - inside the constructor of the NavigationRequest.
> 
> This two points in code correspond to a new navigation (browser-initiated or
> render-initiated), which makes the reasoning simpler since there will only be
a
> single call to RFHM. Then RFHM::BeginNavigation should also be renamed
> RFHM::DidCreateNavigationRequest to match what it now corresponds to.

Thanks.

I chose option 1 and moved RFHM::BeginNavigation into FTN::SetNavigationRequest.
Finally it was not needed to guard that call with
NavigationRequest::ShouldMakeNetworkRequest as it used to be the case.

On the other hand as this change changes the order of things -- now first the
next RFH is dealt with and only after the beforeunload dispatch happens -- the
check for the current RFH being live is effected. So the preconditions for
sending the beforeunload IPC were extracted into a new method that is called
before FTN::SetNavigationRequest to be used later.

https://chromiumcodereview.appspot.com/1163303003/diff/40001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/40001/content/browser/...
content/browser/frame_host/navigator_impl.cc:822: if
(NavigationRequest::ShouldMakeNetworkRequest(
On 2015/06/09 10:02:01, clamy wrote:
> I find the new ordering of comments not clearer. I think having the paragraph
> summary was simpler. This will be even more the case if you move the call to
> RFHM inside FrameTreeNode.

Reverted to the previous comment with slight adaptations.

[clamy, 2015-06-09 16:01:30]

Thanks! It looks better now. A few comments.

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

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:223:
render_manager()->BeginNavigation(*navigation_request_);
Please rename the function in RFHM DidSetNavigationRequest (or
DidCreateNavigationRequest) so that there is no confusion with
OnBeginNavigation.

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:808:
GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
Please add a comment that the value needs to be computed here, since a RFH could
be created when setting the navigation request in the FTN.

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1685: // We don't have a
live renderer or are on a subframe, so just skip running
If changing the comment, rephrase so that it does not use we.

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1727: bool
RenderFrameHostImpl::ShouldDispatchBeforeUnload() {
I think you may copy the TODO(creis): Support beforeunload on subframes. here.

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:350: // beforeunload IPC.
This will not be the case if the current renderer it not
s/beforeunload/BeforeUnload
s/renderer it/renderer is

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:351: // live or this frame
is not the root.
nit: I would rephrase the whole sentence as: This is the case if the current
renderer is live and this frame is the main frame. (to avoid the negations).

https://codereview.chromium.org/1163303003/diff/60001/content/browser/securit...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/1163303003/diff/60001/content/browser/securit...
content/browser/security_exploit_browsertest.cc:78: RenderViewHostImpl*
pending_rvh = nullptr;
I think it would be better to rewrite this part using RFHs instead of RVHs,
since the facts we use RVH here is not important (you can get the Process from
the RFH as well).

[clamy, 2015-06-09 16:07:44]

One additional comment I just thought about.

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1163303003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1687: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
Now that I think about it, we should never end up there for a navigation in
PlzNavigate. Can you add a DCHECK(!base... || !for_navigation) here and remove
the if/else condition.

[carlosk, 2015-06-10 11:53:09]

Thanks.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
File content/browser/frame_host/frame_tree_node.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/frame_tree_node.cc:223:
render_manager()->BeginNavigation(*navigation_request_);
On 2015/06/09 16:01:29, clamy wrote:
> Please rename the function in RFHM DidSetNavigationRequest (or
> DidCreateNavigationRequest) so that there is no confusion with
> OnBeginNavigation.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/navigator_impl.cc:808:
GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
On 2015/06/09 16:01:29, clamy wrote:
> Please add a comment that the value needs to be computed here, since a RFH
could
> be created when setting the navigation request in the FTN.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1685: // We don't have a
live renderer or are on a subframe, so just skip running
On 2015/06/09 16:01:30, clamy wrote:
> If changing the comment, rephrase so that it does not use we.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1687: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/06/09 16:07:44, clamy wrote:
> Now that I think about it, we should never end up there for a navigation in
> PlzNavigate. Can you add a DCHECK(!base... || !for_navigation) here and remove
> the if/else condition.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1727: bool
RenderFrameHostImpl::ShouldDispatchBeforeUnload() {
On 2015/06/09 16:01:30, clamy wrote:
> I think you may copy the TODO(creis): Support beforeunload on subframes. here.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:350: // beforeunload IPC.
This will not be the case if the current renderer it not
On 2015/06/09 16:01:30, clamy wrote:
> s/beforeunload/BeforeUnload
> s/renderer it/renderer is

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:351: // live or this frame
is not the root.
On 2015/06/09 16:01:30, clamy wrote:
> nit: I would rephrase the whole sentence as: This is the case if the current
> renderer is live and this frame is the main frame. (to avoid the negations).

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
File content/browser/security_exploit_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/60001/content/browser/...
content/browser/security_exploit_browsertest.cc:78: RenderViewHostImpl*
pending_rvh = nullptr;
On 2015/06/09 16:01:30, clamy wrote:
> I think it would be better to rewrite this part using RFHs instead of RVHs,
> since the facts we use RVH here is not important (you can get the Process from
> the RFH as well).

Done.

[clamy, 2015-06-10 12:30:28]

Thanks! A few more comments.

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

https://codereview.chromium.org/1163303003/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:821: //
NavigationRequest::BeginNavigation should get called. If the navigation is
I think this comment needs a bit of rephrasing to match the current code: 
-Remove the (eg...) from above.
-After should get called: This is the case if the renderer is not live, or if
the navigation is synchronous... 
-No need to mention that it can be sent directly to the current renderer, but
the part in parenthesis is important.

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

https://codereview.chromium.org/1163303003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1685:
switches::kEnableBrowserSideNavigation));
The check should be:
!base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableBrowserSideNavigation) || !for_navigation.

https://codereview.chromium.org/1163303003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1721: // TODO(creis):
Support beforeunload on subframes.  For now just pretend that
Remove the sentence starting with "For now..."

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

https://codereview.chromium.org/1163303003/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:387: // Notifies the RFHM
that a new navigation request has been set at the frame
s/navigation request/NavigationRequest
s/at/in

https://codereview.chromium.org/1163303003/diff/100001/content/browser/securi...
File content/browser/security_exploit_browsertest.cc (right):

https://codereview.chromium.org/1163303003/diff/100001/content/browser/securi...
content/browser/security_exploit_browsertest.cc:76: // speculative/pending
RenderViewHost. Ensure it exists and is in a different
s/RenderViewHost/RenderFrameHost

[carlosk, 2015-06-10 13:15:06]

Thanks again.

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
content/browser/frame_host/navigator_impl.cc:821: //
NavigationRequest::BeginNavigation should get called. If the navigation is
On 2015/06/10 12:30:27, clamy wrote:
> I think this comment needs a bit of rephrasing to match the current code: 
> -Remove the (eg...) from above.
> -After should get called: This is the case if the renderer is not live, or if
> the navigation is synchronous... 
> -No need to mention that it can be sent directly to the current renderer, but
> the part in parenthesis is important.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1685:
switches::kEnableBrowserSideNavigation));
On 2015/06/10 12:30:27, clamy wrote:
> The check should be:
> !base::CommandLine::ForCurrentProcess()->HasSwitch(
> switches::kEnableBrowserSideNavigation) || !for_navigation.

Done. I missed that part from the previous comment.

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1721: // TODO(creis):
Support beforeunload on subframes.  For now just pretend that
On 2015/06/10 12:30:27, clamy wrote:
> Remove the sentence starting with "For now..."

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
content/browser/frame_host/render_frame_host_manager.h:387: // Notifies the RFHM
that a new navigation request has been set at the frame
On 2015/06/10 12:30:27, clamy wrote:
> s/navigation request/NavigationRequest
> s/at/in

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
File content/browser/security_exploit_browsertest.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/100001/content/browser...
content/browser/security_exploit_browsertest.cc:76: // speculative/pending
RenderViewHost. Ensure it exists and is in a different
On 2015/06/10 12:30:27, clamy wrote:
> s/RenderViewHost/RenderFrameHost

Done.

[clamy, 2015-06-10 13:22:03]

Thanks! Lgtm with one nit.

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

https://codereview.chromium.org/1163303003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:809: // This value must be set here
because SetNavigationRequest might change the
nit: empty line above comment. In fact, it would be better to move the
should_dispatch_beforeunload initialization above the navigation_type
initialization (and leave an empty line between the two of them).

[carlosk, 2015-06-10 14:22:52]

carlosk@chromium.org changed reviewers:
+ nasko@chromium.org

[carlosk, 2015-06-10 14:22:53]

Thanks.

nasko@: PTAL.

https://chromiumcodereview.appspot.com/1163303003/diff/120001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/120001/content/browser...
content/browser/frame_host/navigator_impl.cc:809: // This value must be set here
because SetNavigationRequest might change the
On 2015/06/10 13:22:03, clamy wrote:
> nit: empty line above comment. In fact, it would be better to move the
> should_dispatch_beforeunload initialization above the navigation_type
> initialization (and leave an empty line between the two of them).

Done.

[nasko, 2015-06-12 17:22:22]

Looks mostly good, just a few minor things.

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:643:
frame_tree_node->SetNavigationRequest(navigation_request.Pass());
If you've eliminated the local scoped_ptr in RequestNavigation and this method
is already being modified, might as well standardize on one pattern and use it.

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:822: // the case if the renderer is
not live, this is a subframe navigation, or if
The details of when running the beforeUnload handler is required or not
shouldn't be documented here, but rather at ShouldDispatchBeforeUnload.

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:883:
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
You have an EXPECT_EQ in other places with this pattern, why not here?

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1682: // There's no live
renderer or this is a subframe, so just skip running
ShouldDispatchBeforeUnload should be clear enough without need to comment on the
decision behind it. It invites stale comments if the implementation changes.

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1683: // BeforeUnload.
nit: Keep the capitalization intact (aka no capitalization).

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1686: !for_navigation);
!A || !B is the same as !(A && B), but is a bit more readable.

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

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:704: void
RenderFrameHostManager::DidSetNavigationRequest(
nit: I don't have a great alternative for the name, but the "Set" part of it
seems a bit strange. Maybe DidCreateNavigationRequest, such that the logic
becomes "When a new nav request is created, we should create a speculative RFH".

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

https://codereview.chromium.org/1163303003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:388: // set in the frame
node so that it can speculatively create a new
nit: Let's use actual class names, so it is more concrete. s/frame
node/FrameTreeNode/.

[carlosk, 2015-06-15 19:11:55]

Thanks.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/navigator_impl.cc:643:
frame_tree_node->SetNavigationRequest(navigation_request.Pass());
On 2015/06/12 17:22:22, nasko (slow) wrote:
> If you've eliminated the local scoped_ptr in RequestNavigation and this method
> is already being modified, might as well standardize on one pattern and use
it.

Done.

My initial thought was that as it is used it only once here (there it was used
many times) I'd keep the diff simpler by not making this change. Still trying to
find the correct balance.

I was tempted to merge the now raw navigation_request pointer with the
ongoing_navigation_request above but felt like this split make things clearer.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/navigator_impl.cc:822: // the case if the renderer is
not live, this is a subframe navigation, or if
On 2015/06/12 17:22:22, nasko (slow) wrote:
> The details of when running the beforeUnload handler is required or not
> shouldn't be documented here, but rather at ShouldDispatchBeforeUnload.

I'm assuming the same logic applies to the details concerning
ShouldMakeNetworkRequest.

So I updated this comment to not go into either of them and moved information
out to the declaration of ShouldMakeNetworkRequest (ShouldDispatchBeforeUnload
already had the concerned details).

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/navigator_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/navigator_impl_unittest.cc:883:
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
On 2015/06/12 17:22:22, nasko (slow) wrote:
> You have an EXPECT_EQ in other places with this pattern, why not here?

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1682: // There's no live
renderer or this is a subframe, so just skip running
On 2015/06/12 17:22:22, nasko (slow) wrote:
> ShouldDispatchBeforeUnload should be clear enough without need to comment on
the
> decision behind it. It invites stale comments if the implementation changes.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1683: // BeforeUnload.
On 2015/06/12 17:22:22, nasko (slow) wrote:
> nit: Keep the capitalization intact (aka no capitalization).

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_impl.cc:1686: !for_navigation);
On 2015/06/12 17:22:22, nasko (slow) wrote:
> !A || !B is the same as !(A && B), but is a bit more readable.

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_manager.cc:704: void
RenderFrameHostManager::DidSetNavigationRequest(
On 2015/06/12 17:22:22, nasko (slow) wrote:
> nit: I don't have a great alternative for the name, but the "Set" part of it
> seems a bit strange. Maybe DidCreateNavigationRequest, such that the logic
> becomes "When a new nav request is created, we should create a speculative
RFH".

I renamed the method to DidCreateNavigationRequest (it was one of the
suggestions by clamy@ as well).

But now should I also move the call to it from
FrameTreeNode::SetNavigationRequest to NavigationRequest's constructor  given
the new name? Even though it seems more consistent I feel it a bit odd to have a
notification triggered just by instantiating the request.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_manager.h:388: // set in the frame
node so that it can speculatively create a new
On 2015/06/12 17:22:22, nasko (slow) wrote:
> nit: Let's use actual class names, so it is more concrete. s/frame
> node/FrameTreeNode/.

Done.

[nasko, 2015-06-15 22:12:45]

LGTM with a couple of nits.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_manager.cc:704: void
RenderFrameHostManager::DidSetNavigationRequest(
On 2015/06/15 19:11:54, carlosk wrote:
> On 2015/06/12 17:22:22, nasko (slow) wrote:
> > nit: I don't have a great alternative for the name, but the "Set" part of it
> > seems a bit strange. Maybe DidCreateNavigationRequest, such that the logic
> > becomes "When a new nav request is created, we should create a speculative
> RFH".
> 
> I renamed the method to DidCreateNavigationRequest (it was one of the
> suggestions by clamy@ as well).
> 
> But now should I also move the call to it from
> FrameTreeNode::SetNavigationRequest to NavigationRequest's constructor  given
> the new name? Even though it seems more consistent I feel it a bit odd to have
a
> notification triggered just by instantiating the request.

I think moving the SetNavigationRequest call to the constructor makes sense.
Alternatively (or in addition), rename FT::SetNavigationRequest to
FT::CreatedNavigationRequest to be more clear on when to call this method.

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
content/browser/frame_host/navigation_request.h:57: // sent to the network
stack. It will not be sent for data URLs or Javascript
nit: JavaScript

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
content/browser/frame_host/navigator_impl.cc:820: // Have the current renderer
execute its beforeUnload event if needed. If it
Let's fix the usage of 'beforeunload' on the first line to be inline with the
rest.

[carlosk, 2015-06-16 10:14:46]

Thanks again.

See below for why I finally didn't move that call.

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/140001/content/browser...
content/browser/frame_host/render_frame_host_manager.cc:704: void
RenderFrameHostManager::DidSetNavigationRequest(
On 2015/06/15 22:12:45, nasko (slow) wrote:
> On 2015/06/15 19:11:54, carlosk wrote:
> > On 2015/06/12 17:22:22, nasko (slow) wrote:
> > > nit: I don't have a great alternative for the name, but the "Set" part of
it
> > > seems a bit strange. Maybe DidCreateNavigationRequest, such that the logic
> > > becomes "When a new nav request is created, we should create a speculative
> > RFH".
> > 
> > I renamed the method to DidCreateNavigationRequest (it was one of the
> > suggestions by clamy@ as well).
> > 
> > But now should I also move the call to it from
> > FrameTreeNode::SetNavigationRequest to NavigationRequest's constructor 
given
> > the new name? Even though it seems more consistent I feel it a bit odd to
have
> a
> > notification triggered just by instantiating the request.
> 
> I think moving the SetNavigationRequest call to the constructor makes sense.
> Alternatively (or in addition), rename FT::SetNavigationRequest to
> FT::CreatedNavigationRequest to be more clear on when to call this method.

Moving the SetNavigationRequest call to the constructor makes so that it is
called *before* the request is set on the FrameTreeNode. This caused more tests
failures because there's a lot of things that can happen from when the call
happens, like the navigation committing or delegate notifications being invoked.

So I decided to keep it where it is but rename the method as you suggested
making the naming scheme more consistent.

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
content/browser/frame_host/navigation_request.h:57: // sent to the network
stack. It will not be sent for data URLs or Javascript
On 2015/06/15 22:12:45, nasko (slow) wrote:
> nit: JavaScript

Done.

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/1163303003/diff/180001/content/browser...
content/browser/frame_host/navigator_impl.cc:820: // Have the current renderer
execute its beforeUnload event if needed. If it
On 2015/06/15 22:12:45, nasko (slow) wrote:
> Let's fix the usage of 'beforeunload' on the first line to be inline with the
> rest.

Done.

[carlosk, 2015-06-16 10:14:56]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2015-06-16 10:14:57]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
nasko@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1163303003/#ps190011 (title: "Method
renames and comment updates.")

[commit-bot: I haz the power, 2015-06-16 10:15:38]

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

[commit-bot: I haz the power, 2015-06-16 11:25:16]

Committed patchset #9 (id:190011)

[commit-bot: I haz the power, 2015-06-16 11:26:14]

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