PlzNavigate: Move the navigation logic to NavigatorImpl

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index f48f266347443ea23cdb2682047d7d2329f14639..8935acc6017982cb2f1137e57e0213fa2273bfc8 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -18,8 +18,6 @@
 #include "content/browser/frame_host/navigation_before_commit_info.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
-#include "content/browser/frame_host/navigation_request.h"
-#include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_factory.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
@@ -30,7 +28,7 @@
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/browser/webui/web_ui_impl.h"
-#include "content/common/navigation_params.h"
+#include "content/common/frame_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
@@ -42,54 +40,9 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
-#include "net/base/load_flags.h"
 
 namespace content {
 
-// PlzNavigate
-// Returns the net load flags to use based on the navigation type.
-// TODO(clamy): unify the code with what is happening on the renderer side.
-int LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
-  int load_flags = net::LOAD_NORMAL;
-  switch (navigation_type) {
-    case FrameMsg_Navigate_Type::RELOAD:
-    case FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL:
-      load_flags |= net::LOAD_VALIDATE_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE:
-      load_flags |= net::LOAD_BYPASS_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RESTORE:
-      load_flags |= net::LOAD_PREFERRING_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RESTORE_WITH_POST:
-      load_flags |= net::LOAD_ONLY_FROM_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::NORMAL:
-    default:
-      break;
-  }
-  return load_flags;
-}
-
-// PlzNavigate
-// Generates a default FrameHostMsg_BeginNavigation_Params to be used when there
-// is no live renderer.
-FrameHostMsg_BeginNavigation_Params MakeDefaultBeginNavigation(
-    const RequestNavigationParams& request_params,
-    FrameMsg_Navigate_Type::Value navigation_type) {
-  FrameHostMsg_BeginNavigation_Params begin_navigation_params;
-  begin_navigation_params.method = request_params.is_post ? "POST" : "GET";
-  begin_navigation_params.load_flags =
-      LoadFlagFromNavigationType(navigation_type);
-
-  // TODO(clamy): Post data from the browser should be put in the request body.
-  // Headers should be filled in as well.
-
-  begin_navigation_params.has_user_gesture = false;
-  return begin_navigation_params;
-}
-
 bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   node->render_manager()->pending_delete_hosts_.clear();
   return true;
@@ -121,16 +74,6 @@ RenderFrameHostManager::~RenderFrameHostManager() {
 
   // Delete any swapped out RenderFrameHosts.
   STLDeleteValues(&proxy_hosts_);
-
-  // PlzNavigate
-  // There is an active navigation request for this RFHM so it needs to be
-  // canceled.
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableBrowserSideNavigation)) {
-    if (navigation_request_.get())
-      navigation_request_->CancelNavigation();
-  }
-
 }
 
 void RenderFrameHostManager::Init(BrowserContext* browser_context,
@@ -453,14 +396,6 @@ void RenderFrameHostManager::ResumeResponseDeferredAtStart() {
 
 void RenderFrameHostManager::DidNavigateFrame(
     RenderFrameHostImpl* render_frame_host) {
-  // PlzNavigate
-  // The navigation request has been committed so the browser process doesn't
-  // need to care about it anymore.
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableBrowserSideNavigation)) {
-    navigation_request_.reset();
-  }
-
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
@@ -580,85 +515,19 @@ void RenderFrameHostManager::ResetProxyHosts() {
 }
 
 // PlzNavigate
-bool RenderFrameHostManager::RequestNavigation(
-    scoped_ptr<NavigationRequest> navigation_request,
-    const RequestNavigationParams& request_params) {
-  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableBrowserSideNavigation));
-
-  // TODO(clamy): Check if navigations are blocked and if so store the
-  // parameters.
-
-  // If there is an ongoing request it must be canceled.
-  if (navigation_request_.get())
-    navigation_request_->CancelNavigation();
-
-  navigation_request_ = navigation_request.Pass();
-
-  if (render_frame_host_->IsRenderFrameLive()) {
-    // TODO(clamy): send a RequestNavigation IPC.
-    return true;
-  }
-
-  // The navigation request is sent directly to the IO thread.
-  OnBeginNavigation(
-      MakeDefaultBeginNavigation(
-          request_params, navigation_request_->common_params().navigation_type),
-      navigation_request_->common_params());
-  return true;
-}
-
-// PlzNavigate
-void RenderFrameHostManager::OnBeginNavigation(
-    const FrameHostMsg_BeginNavigation_Params& params,
-    const CommonNavigationParams& common_params) {
+RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(

[nasko, 2014/10/01 15:59:50]

I just realized that this method acts as though --site-per-process is passed in,
aka it will navigate each frame to a new process if SiteInstance changes. I'm
not quite sure if this is the right thing to do yet, though definitely needed
long term.

Also, this should be the equivalent to UpdateStateForNavigate in the PlzNavigate
world, right? Is it possible to just use UpdateStateForNavigate?

[clamy, 2014/10/01 17:10:43]

Well it's pretty close, but UpdateStateForNavigate has a few extra things that
we don't want to use here. For example it handles transfers, or tells the old
renderer to execute its BeforeUnload event in cross process navigation. What I
could do is extract the parts of UpdateStateForNavigate that we are interested
in in helper functions, or have part of UpdateStateForNavigate not execute if we
have the browser-side navigations flag on, as well as remove its dependency on
the NavigationEntry.

As to the first concern, I think it will only be a problem once we start
handling renderer initiated navigations. As of now we only handle browser
initiated navigations, which come from WebContentsImpl::NavigateToPendingEntry.
AFAIU the code in WebContentsImpl makes sure that Navigator::NavigateToEntry is
only called on a top-level frame if --site-per-process is not passed in, right?

[nasko, 2014/10/01 18:20:19]

Either helper functions or flag checks will be fine. Whichever is more readable
and easier to follow. Looking over the code, the block that deals with
transfers/beforeunload is the only thing that we won't need if I'm not missing
something else.
 
Correct. Maybe a todo somewhere that we depend on this and things will change
once we start handling renderer initiated navigations?

[clamy, 2014/10/01 22:30:47]

Done.

+      const GURL& url,
+      ui::PageTransition transition) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  // TODO(clamy): In case of a renderer initiated navigation create a new
-  // NavigationRequest.
-  DCHECK(navigation_request_.get());
-  // Update the referrer with the one received from the renderer.
-  navigation_request_->common_params().referrer = common_params.referrer;
-
-  scoped_ptr<NavigationRequestInfo> info(new NavigationRequestInfo(params));
-
-  info->first_party_for_cookies =
-      frame_tree_node_->IsMainFrame()
-          ? navigation_request_->common_params().url
-          : frame_tree_node_->frame_tree()->root()->current_url();
-  info->is_main_frame = frame_tree_node_->IsMainFrame();
-  info->parent_is_main_frame = !frame_tree_node_->parent() ?
-      false : frame_tree_node_->parent()->IsMainFrame();
-
-  // TODO(clamy): Check if the current RFH should be initialized (in case it has
-  // crashed) not to display a sad tab while navigating.
-  // TODO(clamy): Spawn a speculative renderer process if we do not have one to
-  // use for the navigation.
-
-  navigation_request_->BeginNavigation(info.Pass(), params.request_body);
-}
-
-// PlzNavigate
-void RenderFrameHostManager::CommitNavigation(
-    const NavigationBeforeCommitInfo& info) {
-  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableBrowserSideNavigation));
-  DCHECK(navigation_request_.get());
-  // Ignores navigation commits if the request ID doesn't match the current
-  // active request.
-  if (navigation_request_->navigation_request_id() !=
-          info.navigation_request_id) {
-    return;
-  }
 
   // Pick the right RenderFrameHost to commit the navigation.
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
-  // TODO(clamy): Replace the default values by the right ones. This may require
-  // some storing in RequestNavigation.
+  // TODO(clamy): Replace the default values by the right ones.
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
-      info.navigation_url,
+      url,
       NULL,
-      navigation_request_->common_params().transition,
+      transition,
       false,
       false);
   DCHECK(!pending_render_frame_host_.get());
@@ -686,15 +555,10 @@ void RenderFrameHostManager::CommitNavigation(
                         opener_route_id,
                         MSG_ROUTING_NONE,
                         frame_tree_node_->IsMainFrame())) {
-      return;
+      return NULL;
     }
   }
-
-  frame_tree_node_->navigator()->CommitNavigation(
-      render_frame_host_.get(),
-      info.stream_url,
-      navigation_request_->common_params(),
-      navigation_request_->commit_params());
+  return render_frame_host_.get();
 }
 
 void RenderFrameHostManager::Observe(