PlzNavigate: Create the speculative renderer earlier.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 683 matching lines @@
     }
   }
   return false;
 }
 
 void RenderFrameHostManager::ResetProxyHosts() {
   STLDeleteValues(&proxy_hosts_);
 }
 
 // PlzNavigate
-void RenderFrameHostManager::BeginNavigation(const NavigationRequest& request) {
+void RenderFrameHostManager::DidSetNavigationRequest(

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

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".

[carlosk, 2015/06/15 19:11:54]

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.

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

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.

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

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.

+    const NavigationRequest& request) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   // Clean up any state in case there's an ongoing navigation.
   // TODO(carlosk): remove this cleanup here once we properly cancel ongoing
   // navigations.
   CleanUpNavigation();
 
   RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request);
   DCHECK(dest_rfh);
 }
@@ skipping 1461 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content