PlzNavigate: Remove the RequestNavigation IPC

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 ebd10e9739d85a3dbb38636447002fd1893e0df6..8e092e614898061b7b9054dab5bde674af7f93e7 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -19,6 +19,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/frame_messages.h"
 #include "content/common/navigation_params.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
@@ -37,8 +38,6 @@
 #include "content/public/common/resource_response.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
-#include "net/base/load_flags.h"
-#include "net/http/http_request_headers.h"
 
 namespace content {
 
@@ -70,65 +69,6 @@ FrameMsg_Navigate_Type::Value GetNavigationType(
   return FrameMsg_Navigate_Type::NORMAL;
 }
 
-// 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);
-
-  // Copy existing headers and add necessary headers that may not be present
-  // in the RequestNavigationParams.
-  net::HttpRequestHeaders headers;
-  headers.AddHeadersFromString(request_params.extra_headers);
-  headers.SetHeaderIfMissing(net::HttpRequestHeaders::kUserAgent,
-                             GetContentClient()->GetUserAgent());
-  headers.SetHeaderIfMissing("Accept", "*/*");
-  begin_navigation_params.headers = headers.ToString();
-
-  // Fill POST data from the browser in the request body.
-  if (request_params.is_post) {
-    begin_navigation_params.request_body = new ResourceRequestBody();
-    begin_navigation_params.request_body->AppendBytes(
-        reinterpret_cast<const char *>(
-            &request_params.browser_initiated_post_data.front()),
-        request_params.browser_initiated_post_data.size());
-  }
-
-  begin_navigation_params.has_user_gesture = false;
-  return begin_navigation_params;
-}
-
 RenderFrameHostManager* GetRenderManager(RenderFrameHostImpl* rfh) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSitePerProcess))
@@ -746,10 +686,8 @@ void NavigatorImpl::RequestTransferURL(
 }
 
 // PlzNavigate
-void NavigatorImpl::OnBeginNavigation(
-    FrameTreeNode* frame_tree_node,
-    const FrameHostMsg_BeginNavigation_Params& params,
-    const CommonNavigationParams& common_params) {
+void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
+                                      bool proceed) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
@@ -757,43 +695,41 @@ void NavigatorImpl::OnBeginNavigation(
   NavigationRequest* navigation_request =
       navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
 
-  if (!navigation_request) {
-    // This is a renderer initiated navigation, so generate a new
-    // NavigationRequest and store it in the map.
-    // TODO(clamy): Check if some PageState should be provided here.
-    // TODO(clamy): See how we should handle override of the user agent when the
-    // navigation may start in a renderer and commit in another one.
-    // TODO(clamy): See if the navigation start time should be measured in the
-    // renderer and sent to the browser instead of being measured here.
-    scoped_ptr<NavigationRequest> scoped_request(new NavigationRequest(
-        frame_tree_node, common_params,
-        CommitNavigationParams(PageState(), false, base::TimeTicks::Now()),
-        nullptr));
-    navigation_request = scoped_request.get();
-    navigation_request_map_.set(
-        frame_tree_node->frame_tree_node_id(), scoped_request.Pass());
-  }
-  DCHECK(navigation_request);
-
-  // Update the referrer with the one received from the renderer.
-  navigation_request->common_params().referrer = common_params.referrer;
+  // The NavigationRequest may have been canceled while the renderer was
+  // executing the BeforeUnload event.
+  if (!navigation_request)
+    return;
 
-  scoped_ptr<NavigationRequestInfo> info(new NavigationRequestInfo(params));
+  DCHECK(navigation_request->state() ==
+      NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);

[davidben, 2015/02/03 02:23:21]

Nit: DCHECK_EQ

[carlosk, 2015/02/03 16:06:02]

Isn't it possible that the original request was cancelled AND another request
was created in the meantime? 

[clamy, 2015/02/03 16:17:09]

Yes but in that case we do not send another BeforeUnload IPC. So we should not
be receiving an ACK when we are in another state.

[clamy, 2015/02/03 16:17:09]

Done.

 
-  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();
+  if (proceed)
+    BeginNavigation(frame_tree_node);
+  else
+    CancelNavigation(frame_tree_node);
+}
 
-  // First start the request on the IO thread.
-  navigation_request->BeginNavigation(info.Pass(), params.request_body);
+// PlzNavigate
+void NavigatorImpl::OnBeginNavigation(
+    FrameTreeNode* frame_tree_node,
+    const CommonNavigationParams& common_params,
+    const BeginNavigationParams& begin_params,
+    scoped_refptr<ResourceRequestBody> body) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  DCHECK(frame_tree_node);
 
-  // Then notify the RenderFrameHostManager so it can speculatively create a
-  // RenderFrameHost (and potentially a new renderer process) in parallel.
-  frame_tree_node->render_manager()->BeginNavigation(*navigation_request);
+  // This is a renderer initiated navigation, so generate a new

[nasko, 2015/02/02 21:17:43]

nit: renderer-initiated

[clamy, 2015/02/03 16:17:09]

Done.

+  // NavigationRequest and store it in the map.
+  // TODO(clamy): Renderer-initiated navigations should not always cancel the
+  // current one.
+  scoped_ptr<NavigationRequest> navigation_request =
+      NavigationRequest::CreateRendererInitiated(
+          frame_tree_node, common_params, begin_params, body);
+  navigation_request_map_.set(

[carlosk, 2015/02/03 16:06:02]

Don't we need to verify first if another request is already ongoing?

[clamy, 2015/02/03 16:17:09]

That's the TODO above this block of code. The patch is large enough that I did
not want to go in fully implementing the navigation cancelation and associated
tests.

+      frame_tree_node->frame_tree_node_id(), navigation_request.Pass());
+
+  BeginNavigation(frame_tree_node);
 }
 
 // PlzNavigate
@@ -905,35 +841,42 @@ bool NavigatorImpl::RequestNavigation(
   int64 frame_tree_node_id = frame_tree_node->frame_tree_node_id();
   FrameMsg_Navigate_Type::Value navigation_type =
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
-  scoped_ptr<NavigationRequest> navigation_request = NavigationRequest::Create(
-      frame_tree_node, entry, navigation_type, navigation_start);
-  RequestNavigationParams request_params(entry.GetHasPostData(),
-                                         entry.extra_headers(),
-                                         entry.GetBrowserInitiatedPostData());
+  scoped_ptr<NavigationRequest> navigation_request =
+      NavigationRequest::CreateBrowserInitiated(
+          frame_tree_node, entry, navigation_type, navigation_start);
   // TODO(clamy): Check if navigations are blocked and if so store the
   // parameters.
 
   // If there is an ongoing request, replace it.
   navigation_request_map_.set(frame_tree_node_id, navigation_request.Pass());
 
-  if (frame_tree_node->current_frame_host()->IsRenderFrameLive()) {
-    NavigationRequest* request_to_send =
-        navigation_request_map_.get(frame_tree_node_id);
-    frame_tree_node->current_frame_host()->Send(new FrameMsg_RequestNavigation(
-        frame_tree_node->current_frame_host()->GetRoutingID(),
-        request_to_send->common_params(), request_params));
-    request_to_send->SetWaitingForRendererResponse();
-    return true;
-  }
-
-  // The navigation request is sent directly to the IO thread.
-  OnBeginNavigation(
-      frame_tree_node,
-      MakeDefaultBeginNavigation(request_params, navigation_type),
-      navigation_request_map_.get(frame_tree_node_id)->common_params());
+  // Have the current renderer execute its BeforeUnload event if needed. If it

[nasko, 2015/02/02 21:17:43]

nit: beforeUnload

[clamy, 2015/02/03 16:17:09]

Done.

+  // is not needed (eg. the renderer is not live), BeginNavigation should get
+  // called.
+  NavigationRequest* request_to_send =
+      navigation_request_map_.get(frame_tree_node_id);
+  request_to_send->SetWaitingForRendererResponse();
+  frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);

[nasko, 2015/02/02 21:17:43]

Do we know this is a cross-site transition? It could be just regular
browser-initiated navigation for the same site.

[davidben, 2015/02/03 02:23:21]

Even if the navigation is same-site, I think we'll want beforeunload to be
driven by the browser.

I think the reason it's special in today's flow is that today we ask a renderer
to navigate on the browser's behalf. And if it's the same renderer, it'll run
beforeunload for us because that's what Blink does. But, in PlzNavigate, by the
time the renderer finds out about the navigation, it'll be too late for it to
beforeunload anyway.

[clamy, 2015/02/03 16:17:09]

My understanding is that this flag is used by RenderFrameHostImpl to distinguish
whether we are waiting for the BeforeUnloadACK because we are navigating
(cross-site only in the current implementation) or because we are closing the
page. Given that we are navigating here, having for for_cross_site_transition
true made more sense. Maybe it could be renamed to for_navigation, which would
make it clearer.

[nasko, 2015/02/04 00:14:19]

Indeed renaming it makes sense.

[clamy, 2015/02/05 15:33:13]

Included the renaming in this CL.

   return true;

[carlosk, 2015/02/03 16:06:02]

This return doesn't seem to be meaningful anymore.

[clamy, 2015/02/03 16:17:09]

We either always return true here, or in NavigateToEntry (since NavigateToEntry
needs a return value). Not sure which one is better.

[nasko, 2015/02/04 00:14:19]

If this method cannot fail, it shouldn't be returning a boolean. I'm fine with
doing it in a separate CL to keep this one from exploding.

[clamy, 2015/02/05 15:33:13]

It's a simple enough change, so including it in this CL.

 }
 
+void NavigatorImpl::BeginNavigation(FrameTreeNode* frame_tree_node) {
+  NavigationRequest* navigation_request =
+      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+
+  // A browser-initiated navigation could have been cancelled while it was
+  // waiting for the BeforeUnload event to execute.
+  if (!navigation_request)
+    return;
+
+  // First start the request on the IO thread.
+  navigation_request->BeginNavigation();
+
+  // Then notify the RenderFrameHostManager so it can speculatively create a
+  // RenderFrameHost (and potentially a new renderer process) in parallel.
+  frame_tree_node->render_manager()->BeginNavigation(*navigation_request);
+}
+
 void NavigatorImpl::RecordNavigationMetrics(
     const LoadCommittedDetails& details,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,