PlzNavigate: implement CommitNavigation on the browser side

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/debug/trace_event.h"
@@ skipping 12 matching lines @@
 #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"
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #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/view_messages.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
 #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 {
 
-namespace {
+// PlzNavigate
+// Returns the net load flags to use based on the navigation type.
+// TODO(clamy): unify the code with what is happening on the browser side.

[nasko, 2014/09/24 22:42:14]

This is the browser side. Do you mean to unify with renderer side?

[clamy, 2014/09/26 17:22:32]

Done.

+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
-// Simulates a renderer response to a navigation request when there is no live
-// renderer.
-FrameHostMsg_BeginNavigation_Params BeginNavigationFromNavigate(
-    const FrameMsg_Navigate_Params& navigate_params) {
+// 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 = navigate_params.is_post ? "POST" : "GET";
-  begin_navigation_params.url = navigate_params.url;
-  begin_navigation_params.referrer =
-      Referrer(navigate_params.referrer.url, navigate_params.referrer.policy);
-
-  // TODO(clamy): This should be modified to take into account caching policy
-  // requirements (eg for POST reloads).
-  begin_navigation_params.load_flags = net::LOAD_NORMAL;
+  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;
-  begin_navigation_params.transition_type = navigate_params.transition;
-  begin_navigation_params.should_replace_current_entry =
-      navigate_params.should_replace_current_entry;
-  begin_navigation_params.allow_download =
-      navigate_params.allow_download;
   return begin_navigation_params;
 }
 
-}  // namespace
-
 bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   node->render_manager()->pending_delete_hosts_.clear();
   return true;
 }
 
 RenderFrameHostManager::RenderFrameHostManager(
     FrameTreeNode* frame_tree_node,
     RenderFrameHostDelegate* render_frame_delegate,
     RenderViewHostDelegate* render_view_delegate,
     RenderWidgetHostDelegate* render_widget_delegate,
@@ skipping 471 matching lines @@
   if (iter != pending_delete_hosts_.end() && iter->second.get() == rfh)
     pending_delete_hosts_.erase(site_instance_id);
 }
 
 void RenderFrameHostManager::ResetProxyHosts() {
   STLDeleteValues(&proxy_hosts_);
 }
 
 // PlzNavigate
 bool RenderFrameHostManager::RequestNavigation(
-    const NavigationEntryImpl& entry,
-    const FrameMsg_Navigate_Params& navigate_params) {
+    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(BeginNavigationFromNavigate(navigate_params));
+  OnBeginNavigation(
+      MakeDefaultBeginNavigation(
+          request_params, navigation_request_->core_params().navigation_type),
+      navigation_request_->core_params());
   return true;
 }
 
 // PlzNavigate
 void RenderFrameHostManager::OnBeginNavigation(
-    const FrameHostMsg_BeginNavigation_Params& params) {
+    const FrameHostMsg_BeginNavigation_Params& params,
+    const CoreNavigationParams& core_params) {
   CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  // TODO(clamy): Check if navigations are blocked and if so, return
-  // immediately.
-  NavigationRequestInfo info(params);
+  DCHECK(navigation_request_.get());

[nasko, 2014/09/24 22:42:14]

Is this valid DCHECK? In the case of renderer initiated navigation, this is the
entrypoint, so I'd expect us to not have generated the navigation_request_ just
yet.

[clamy, 2014/09/26 17:22:32]

I added a TODO to reflect that we should generate the navigation_request_ in
that case.

+  // Update the referrer with the one received from the renderer.
+  navigation_request_->core_params().referrer = core_params.referrer;
 
-  info.first_party_for_cookies = frame_tree_node_->IsMainFrame() ?
-      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() ?
+  scoped_ptr<NavigationRequestInfo> info(new NavigationRequestInfo(params));
+
+  info->first_party_for_cookies =
+      frame_tree_node_->IsMainFrame()
+          ? navigation_request_->core_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.

[nasko, 2014/09/24 22:42:14]

What happened with the code to cancel existing navigation?

[clamy, 2014/09/26 17:22:32]

It was moved to RequestNavigation which is where we now reset the
navigation_request_. David's patch makes it disappear entirely so the lifetime
should be even easier to track.

[nasko, 2014/09/26 22:16:43]

Acknowledged.

 
-  // If there is an ongoing request it must be canceled.
-  if (navigation_request_.get())
-    navigation_request_->CancelNavigation();
-
-  navigation_request_.reset(new NavigationRequest(
-      info, frame_tree_node_->frame_tree_node_id()));
-  navigation_request_->BeginNavigation(params.request_body);
+  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.
   SiteInstance* new_instance = GetSiteInstanceForNavigation(
       info.navigation_url,
       NULL,
-      navigation_request_->info().navigation_params.transition_type,
+      navigation_request_->core_params().transition,
       false,
       false);
   DCHECK(!pending_render_frame_host_.get());
 
   // TODO(clamy): Update how pending WebUI objects are handled.
   if (current_instance != new_instance) {
     CreateRenderFrameHostForNewSiteInstance(
         current_instance, new_instance, frame_tree_node_->IsMainFrame());
     DCHECK(pending_render_frame_host_.get());
     // TODO(clamy): Wait until the navigation has committed before swapping
@@ skipping 11 matching lines @@
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         render_frame_host_->GetSiteInstance());
     if (!InitRenderView(render_frame_host_->render_view_host(),
                         opener_route_id,
                         MSG_ROUTING_NONE,
                         frame_tree_node_->IsMainFrame())) {
       return;
     }
   }
 
   frame_tree_node_->navigator()->CommitNavigation(

[nasko, 2014/09/24 22:42:14]

RFHM deals directly with RFHs and manages their lifetime/state. Why do we have
to go through the Navigator if we know which RFH we are about to commit to and
can directly call into it?

[clamy, 2014/09/26 17:22:32]

Because Navigator is making a security check to make sure that a WebUI is not
displayed in a non WebUI RFH. I initially had this security check as a method of
NavigatorImpl, but was asked for it to remain private and checked by the
navigator in a previous code review (I think by Charlie).

-      render_frame_host_.get(), info);
+      render_frame_host_.get(),
+      info.stream_url,
+      navigation_request_->core_params(),
+      navigation_request_->commit_params());
 }
 
 void RenderFrameHostManager::Observe(
     int type,
     const NotificationSource& source,
     const NotificationDetails& details) {
   switch (type) {
     case NOTIFICATION_RENDERER_PROCESS_CLOSED:
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
@@ skipping 1036 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