PlzNavigate: implement RequestNavigation in the no live renderer case

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 6aca3d54347570876a7d072b6c1a53e381ab1740..d19231bde41c2c7b898a6ddf5b14ddb528a2bb88 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -62,11 +62,30 @@ FrameMsg_Navigate_Type::Value GetNavigationType(
   return FrameMsg_Navigate_Type::NORMAL;
 }
 
-void MakeNavigateParams(const NavigationEntryImpl& entry,
-                        const NavigationControllerImpl& controller,
-                        NavigationController::ReloadType reload_type,
-                        base::TimeTicks navigation_start,
-                        FrameMsg_Navigate_Params* params) {
+RenderFrameHostManager* GetRenderManager(RenderFrameHostImpl* rfh) {
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
+    return rfh->frame_tree_node()->render_manager();
+
+  return rfh->frame_tree_node()->frame_tree()->root()->render_manager();
+}
+
+}  // namespace
+
+
+NavigatorImpl::NavigatorImpl(

[nasko, 2014/07/17 12:05:08]

Any reason for the move of the constructor around?

[clamy, 2014/07/17 13:07:53]

Actually I'm moving the function MakeNavigateParams out of the anonymous
namespace and below the constructor, but git registered this as the constructor
moving up.

+    NavigationControllerImpl* navigation_controller,
+    NavigatorDelegate* delegate)
+    : controller_(navigation_controller),
+      delegate_(delegate) {
+}
+
+// Static.

[nasko, 2014/07/17 12:05:08]

nit: lowercase S

[clamy, 2014/07/17 13:07:53]

Done.

+void NavigatorImpl::MakeNavigateParams(
+    const NavigationEntryImpl& entry,
+    const NavigationControllerImpl& controller,
+    NavigationController::ReloadType reload_type,
+    base::TimeTicks navigation_start,
+    FrameMsg_Navigate_Params* params) {
   params->page_id = entry.GetPageID();
   params->should_clear_history_list = entry.should_clear_history_list();
   params->should_replace_current_entry = entry.should_replace_entry();
@@ -126,23 +145,6 @@ void MakeNavigateParams(const NavigationEntryImpl& entry,
   params->browser_navigation_start = navigation_start;
 }
 
-RenderFrameHostManager* GetRenderManager(RenderFrameHostImpl* rfh) {
-  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
-    return rfh->frame_tree_node()->render_manager();
-
-  return rfh->frame_tree_node()->frame_tree()->root()->render_manager();
-}
-
-}  // namespace
-
-
-NavigatorImpl::NavigatorImpl(
-    NavigationControllerImpl* navigation_controller,
-    NavigatorDelegate* delegate)
-    : controller_(navigation_controller),
-      delegate_(delegate) {
-}
-
 NavigationController* NavigatorImpl::GetController() {
   return controller_;
 }
@@ -334,8 +336,29 @@ bool NavigatorImpl::NavigateToEntry(
   // capture the time needed for the RenderFrameHost initialization.
   base::TimeTicks navigation_start = base::TimeTicks::Now();
 
+  // WebContents uses this to fill LoadNotificationDetails when the load
+  // completes, so that PerformanceMonitor that listens to the notification can
+  // record the load time. PerformanceMonitor is no longer maintained.
+  // TODO(ppi): make this go away.
+  current_load_start_ = base::TimeTicks::Now();
+
+  // Create the navigation parameters that may be used directly by the
+  // RenderFrameHostManager in browser initiated navigation (part of project
+  // PlzNavigate navigation refactoring).

[nasko, 2014/07/17 12:05:08]

nit: "navigation" is extraneous here.

[clamy, 2014/07/17 13:07:53]

Done.

+  FrameMsg_Navigate_Params navigate_params;
+  MakeNavigateParams(
+      entry, *controller_, reload_type, navigation_start, &navigate_params);
+
   RenderFrameHostManager* manager =
       render_frame_host->frame_tree_node()->render_manager();
+
+  // As part of the PlzNavigate project, the RenderFrameHosts are no longer
+  // asked to navigate. Instead the RenderFrameHostManager handles the
+  // navigation requests for that frame node.

[nasko, 2014/07/17 12:05:08]

There is quite a bit of code that follows the return here, including
notifications for various events and security checks. Why are we skipping them?

[clamy, 2014/07/17 13:07:53]

They need a destination render frame host, which we don't possess at that time.
I thought it would be better to check them when we have the final destination
renderer (on commit).

+#if defined(USE_PLZ_NAVIGATE)

[nasko, 2014/07/17 12:05:08]

Did we agree on define vs command line flag?

[clamy, 2014/07/17 13:07:53]

No, I was asking about that in the previous patch set actually. Do you have any
preference on the question?

[nasko, 2014/07/21 13:02:05]

Let's use a command line flag. This way we don't have to re-run gyp/recompile to
run in the two modes.

+  return manager->RequestNavigation(entry, navigaet_params);
+#endif
+
   RenderFrameHostImpl* dest_render_frame_host = manager->Navigate(entry);
   if (!dest_render_frame_host)
     return false;  // Unable to create the desired RenderFrameHost.
@@ -361,16 +384,7 @@ bool NavigatorImpl::NavigateToEntry(
   if (delegate_)
     delegate_->AboutToNavigateRenderFrame(dest_render_frame_host);
 
-  // WebContents uses this to fill LoadNotificationDetails when the load
-  // completes, so that PerformanceMonitor that listens to the notification can
-  // record the load time. PerformanceMonitor is no longer maintained.
-  // TODO(ppi): make this go away.
-  current_load_start_ = base::TimeTicks::Now();
-
   // Navigate in the desired RenderFrameHost.
-  FrameMsg_Navigate_Params navigate_params;
-  MakeNavigateParams(entry, *controller_, reload_type, navigation_start,
-      &navigate_params);
   dest_render_frame_host->Navigate(navigate_params);
 
   // Make sure no code called via RFH::Navigate clears the pending entry.