PlzNavigate: move ownership of the NavigationRequest to the FrameTreeNode

This CL moves the ownership of NavigationRequests from a map in NavigatorImpl
to the FrameTreeNode. This ensures that NavigationRequests are unique per
FrameTreeNode and are also properly deleted on destruction of the
FrameTreeNode. It also makes it easier to have PlzNavigate adapt to the
refactoring of DidStart/StopLoading happening in
https://codereview.chromium.org/1080143003/.

BUG=439423

Review URL: https://codereview.chromium.org/1080073004

Cr-Commit-Position: refs/heads/master@{#325498}

content/browser/frame_host/frame_tree_node.cc

@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/stl_util.h"
 #include "content/browser/frame_host/frame_tree.h"
+#include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ -71,10 +72,6 @@ FrameTreeNode::~FrameTreeNode() {
   frame_tree_->FrameRemoved(this);
 
   g_frame_tree_node_id_map.Get().erase(frame_tree_node_id_);
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableBrowserSideNavigation)) {
-    navigator_->CancelNavigation(this);
-  }
 }
 
 bool FrameTreeNode::IsMainFrame() const {
@@ -174,4 +171,27 @@ bool FrameTreeNode::CommitPendingSandboxFlags() {
   return did_change_flags;
 }
 
+void FrameTreeNode::SetNavigationRequest(
+    scoped_ptr<NavigationRequest> navigation_request) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  ResetNavigationRequest(false);
+  // TODO(clamy): perform the StartLoading logic here.
+  navigation_request_ = navigation_request.Pass();
+}
+
+void FrameTreeNode::ResetNavigationRequest(bool is_commit) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // Upon commit the current NavigationRequest will be reset. There should be no
+  // cleanup performed since the navigation is still ongoing. If the reset
+  // corresponds to a cancelation, the RenderFrameHostManager should clean up
+  // any speculative RenderFrameHost it created for the navigation.
+  if (navigation_request_ && !is_commit) {
+    // TODO(clamy): perform the StopLoading logic.
+    render_manager_.CleanUpNavigation();
+  }
+  navigation_request_.reset();
+}
+
 }  // namespace content

content/browser/frame_host/frame_tree_node.h

@@ -21,6 +21,7 @@
 namespace content {
 
 class FrameTree;
+class NavigationRequest;
 class Navigator;
 class RenderFrameHostImpl;
 
@@ -142,6 +143,20 @@ class CONTENT_EXPORT FrameTreeNode {
   // Returns this node's loading progress.
   double loading_progress() const { return loading_progress_; }
 
+  NavigationRequest* navigation_request() { return navigation_request_.get(); }
+
+  // PlzNavigate
+  // Takes ownership of |navigation_request| and makes it the current
+  // NavigationRequest of this frame. This corresponds to the start of a new
+  // navigation. If there was an ongoing navigation request before calling this
+  // function, it is canceled. |navigation_request| should not be null.
+  void SetNavigationRequest(scoped_ptr<NavigationRequest> navigation_request);
+
+  // PlzNavigate
+  // Resets the current navigation request. |is_commit| is true if the reset is
+  // due to the commit of the navigation.
+  void ResetNavigationRequest(bool is_commit);
+
  private:
   void set_parent(FrameTreeNode* parent) { parent_ = parent; }
 
@@ -195,6 +210,11 @@ class CONTENT_EXPORT FrameTreeNode {
   // Used to track this node's loading progress (from 0 to 1).
   double loading_progress_;
 
+  // PlzNavigate
+  // Owns an ongoing NavigationRequest until it is ready to commit. It will then
+  // be reset and a RenderFrameHost will be responsible for the navigation.
+  scoped_ptr<NavigationRequest> navigation_request_;
+
   DISALLOW_COPY_AND_ASSIGN(FrameTreeNode);
 };
 

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -220,10 +220,8 @@ class NavigationControllerTest
   bool HasNavigationRequest() {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kEnableBrowserSideNavigation)) {
-      FrameTreeNode* root = contents()->GetFrameTree()->root();
-      NavigationRequest* navigation_request = static_cast<NavigatorImpl*>(
-          root->navigator())->GetNavigationRequestForNodeForTesting(root);
-      return navigation_request != nullptr;
+      return contents()->GetFrameTree()->root()->navigation_request() !=
+             nullptr;
     }
     return process()->sink().GetFirstMessageMatching(FrameMsg_Navigate::ID)
         != nullptr;
@@ -232,9 +230,8 @@ class NavigationControllerTest
   const GURL GetLastNavigationURL() {
     if (base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kEnableBrowserSideNavigation)) {
-      FrameTreeNode* root = contents()->GetFrameTree()->root();
-      NavigationRequest* navigation_request = static_cast<NavigatorImpl*>(
-          root->navigator())->GetNavigationRequestForNodeForTesting(root);
+      NavigationRequest* navigation_request =
+          contents()->GetFrameTree()->root()->navigation_request();
       CHECK(navigation_request);
       return navigation_request->common_params().url;
     }

content/browser/frame_host/navigator_impl.cc

@@ -367,8 +367,7 @@ void NavigatorImpl::DidNavigate(
   // need to care about it anymore.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
-    navigation_request_map_.erase(
-        render_frame_host->frame_tree_node()->frame_tree_node_id());
+    render_frame_host->frame_tree_node()->ResetNavigationRequest(true);
   }
 
   FrameHostMsg_DidCommitProvisionalLoad_Params params(input_params);
@@ -615,8 +614,7 @@ void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
 
-  NavigationRequest* navigation_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
   // The NavigationRequest may have been canceled while the renderer was
   // executing the BeforeUnload event.
@@ -644,7 +642,7 @@ void NavigatorImpl::OnBeginNavigation(
   DCHECK(frame_tree_node);
 
   NavigationRequest* ongoing_navigation_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+      frame_tree_node->navigation_request();
 
   // The renderer-initiated navigation request is ignored iff a) there is an
   // ongoing request b) which is browser or user-initiated and c) the renderer
@@ -657,17 +655,13 @@ void NavigatorImpl::OnBeginNavigation(
   }
 
   // In all other cases the current navigation, if any, is canceled and a new
-  // NavigationRequest is created and stored in the map.
-  if (ongoing_navigation_request)
-    CancelNavigation(frame_tree_node);
-
+  // NavigationRequest is created for the node.
   scoped_ptr<NavigationRequest> navigation_request =
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body,
           controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount());
-  navigation_request_map_.set(
-      frame_tree_node->frame_tree_node_id(), navigation_request.Pass());
+  frame_tree_node->SetNavigationRequest(navigation_request.Pass());
 
   if (frame_tree_node->IsMainFrame())
     navigation_data_.reset();
@@ -682,8 +676,7 @@ void NavigatorImpl::CommitNavigation(FrameTreeNode* frame_tree_node,
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
-  NavigationRequest* navigation_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
   DCHECK(navigation_request);
   DCHECK(response ||
          !NavigationRequest::ShouldMakeNetworkRequest(
@@ -729,8 +722,7 @@ void NavigatorImpl::FailedNavigation(FrameTreeNode* frame_tree_node,
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
-  NavigationRequest* navigation_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
   DCHECK(navigation_request);
 
   // Select an appropriate renderer to show the error page.
@@ -749,26 +741,16 @@ void NavigatorImpl::FailedNavigation(FrameTreeNode* frame_tree_node,
 void NavigatorImpl::CancelNavigation(FrameTreeNode* frame_tree_node) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  navigation_request_map_.erase(frame_tree_node->frame_tree_node_id());
+  frame_tree_node->ResetNavigationRequest(false);
   if (frame_tree_node->IsMainFrame())
     navigation_data_.reset();
-  // TODO(carlosk): move this cleanup into the NavigationRequest destructor once
-  // we properly cancel ongoing navigations.
-  frame_tree_node->render_manager()->CleanUpNavigation();
-}
-
-// PlzNavigate
-NavigationRequest* NavigatorImpl::GetNavigationRequestForNodeForTesting(
-    FrameTreeNode* frame_tree_node) {
-  return navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
 }
 
 bool NavigatorImpl::IsWaitingForBeforeUnloadACK(
     FrameTreeNode* frame_tree_node) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
-  NavigationRequest* request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  NavigationRequest* request = frame_tree_node->navigation_request();
   if (!request)
     return false;
   return request->state() == NavigationRequest::WAITING_FOR_RENDERER_RESPONSE;
@@ -821,36 +803,23 @@ void NavigatorImpl::RequestNavigation(
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
-  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::CreateBrowserInitiated(frame_tree_node, entry,
                                                 navigation_type,
                                                 navigation_start, controller_);
-  // TODO(clamy): Check if navigations are blocked and if so store the
-  // parameters.
-
-  // If there is an ongoing request, cancel and replace it.
-  NavigationRequest* ongoing_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
-  if (ongoing_request)
-    CancelNavigation(frame_tree_node);
-
-  navigation_request_map_.set(frame_tree_node_id, navigation_request.Pass());
+  frame_tree_node->SetNavigationRequest(navigation_request.Pass());
 
   // Have the current renderer execute its beforeUnload event if needed. If it
   // 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->navigation_request()->SetWaitingForRendererResponse();
   frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);
 }
 
 void NavigatorImpl::BeginNavigation(FrameTreeNode* frame_tree_node) {
-  NavigationRequest* navigation_request =
-      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
   // A browser-initiated navigation could have been cancelled while it was
   // waiting for the BeforeUnload event to execute.

content/browser/frame_host/navigator_impl.h

@@ -88,11 +88,6 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
   void CancelNavigation(FrameTreeNode* frame_tree_node) override;
   bool IsWaitingForBeforeUnloadACK(FrameTreeNode* frame_tree_node) override;
 
-  // PlzNavigate
-  // Returns the navigation request for a given node. Used in tests.
-  NavigationRequest* GetNavigationRequestForNodeForTesting(
-      FrameTreeNode* frame_tree_node);
-
  private:
   // Holds data used to track browser side navigation metrics.
   struct NavigationMetricsData;
@@ -142,11 +137,6 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
 
   scoped_ptr<NavigatorImpl::NavigationMetricsData> navigation_data_;
 
-  // PlzNavigate: used to track the various ongoing NavigationRequests in the
-  // different FrameTreeNodes, based on the frame_tree_node_id.
-  typedef base::ScopedPtrHashMap<int64, NavigationRequest> NavigationRequestMap;
-  NavigationRequestMap navigation_request_map_;
-
   DISALLOW_COPY_AND_ASSIGN(NavigatorImpl);
 };
 

content/test/test_render_frame_host.cc

@@ -238,9 +238,7 @@ void TestRenderFrameHost::PrepareForCommitWithServerRedirect(
   }
 
   // PlzNavigate
-  NavigationRequest* request =
-      static_cast<NavigatorImpl*>(frame_tree_node_->navigator())
-          ->GetNavigationRequestForNodeForTesting(frame_tree_node_);
+  NavigationRequest* request = frame_tree_node_->navigation_request();
   CHECK(request);
 
   // Simulate a beforeUnload ACK from the renderer if the browser is waiting for