PlzNavigate: Move the navigation logic to NavigatorImpl

content/browser/frame_host/navigator_impl.h

Index: content/browser/frame_host/navigator_impl.h
diff --git a/content/browser/frame_host/navigator_impl.h b/content/browser/frame_host/navigator_impl.h
index 7d58bbdc41028cba44fb33d7eb8f7616b10bf467..02747a6dece5f35baa7f3d6c4a2f340905b3b85e 100644
--- a/content/browser/frame_host/navigator_impl.h
+++ b/content/browser/frame_host/navigator_impl.h
@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_FRAME_HOST_NAVIGATOR_IMPL_H_
 #define CONTENT_BROWSER_FRAME_HOST_NAVIGATOR_IMPL_H_
 
+#include "base/memory/linked_ptr.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ -67,14 +68,17 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
       const GlobalRequestID& transferred_global_request_id,
       bool should_replace_current_entry,
       bool user_gesture) OVERRIDE;
+  virtual void OnBeginNavigation(
+      FrameTreeNode* frame_tree_node,
+      const FrameHostMsg_BeginNavigation_Params& params,
+      const CommonNavigationParams& common_params) OVERRIDE;
   virtual void CommitNavigation(
-      RenderFrameHostImpl* render_frame_host,
-      const GURL& stream_url,
-      const CommonNavigationParams& common_params,
-      const CommitNavigationParams& commit_params) OVERRIDE;
+      FrameTreeNode* frame_tree_node,
+      const NavigationBeforeCommitInfo& info) OVERRIDE;
+  virtual void CancelNavigation(FrameTreeNode* frame_tree_node) OVERRIDE;
 
  private:
-  virtual ~NavigatorImpl() {}
+  virtual ~NavigatorImpl();
 
   // Navigates to the given entry, which must be the pending entry.  Private
   // because all callers should use NavigateToPendingEntry.
@@ -89,6 +93,14 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
     RenderFrameHostImpl* render_frame_host,
     const GURL& url);
 
+  // PlzNavigate: sends a RequestNavigation IPC to the renderer to ask it to
+  // navigate. If no live renderer is present, then the navigation request will
+  // be sent directly to the ResourceDispatcherHost.
+  bool RequestNavigation(FrameTreeNode* frame_tree_node,
+                         const NavigationEntryImpl& entry,
+                         NavigationController::ReloadType reload_type,
+                         base::TimeTicks navigation_start);
+
   // The NavigationController that will keep track of session history for all
   // RenderFrameHost objects using this NavigatorImpl.
   // TODO(nasko): Move ownership of the NavigationController from
@@ -99,6 +111,12 @@ class CONTENT_EXPORT NavigatorImpl : public Navigator {
   // events. Can be NULL in tests.
   NavigatorDelegate* delegate_;
 
+  // PlzNavigate: used to track the various ongoing NavigationRequests in the
+  // different FrameTreeNode, based on the frame_tree_node_id.

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

nit: nodes should be plural, based on the sentence structure.

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

Done.

+  typedef base::hash_map<int64, linked_ptr<NavigationRequest>>

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

Why linked_ptr? Why not scoped_ptr?

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

I was under the impression that you should not put a scoped_ptr inside a
hash_map due to the copies performed inside it.

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

Why not use ScopedPtrHashMap then? Scoped pointers are simpler to understand and
if we can use those, it will be nicer. Up to you.

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

Because I did not know we had a ScopedPtrHashMap :). I am now using it.

+      NavigationRequestMap;
+  NavigationRequestMap navigation_request_map_;

[carlosk, 2014/10/01 18:31:10]

More of a design discussion: this is in fact replicating the FrameTreeNode tree
to keep navigation state for each node into a single NavigatorImpl instance.

I imagine NavigatorImpl instances are currently 1-to-1 with FrameTree because of
historical reasons, from before RenderFrameHost existed. But now that that's not
the case anymore and navigations exist/have state on a per-node basis are we
considering making them 1-to-1 with FrameTreeNode instances instead? 

I am also guessing that "navigation history" exists in the context of the
tab/web-contents and not for each frame,  am I right? If that's correct I'd
think NavigationControllerImpl would still be kept that way but would
"translate" history based navigations into calls to the proper NavigatorImpl
instances.

Is that something that is being considered? 

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

I don't think there is such a plan. And we are not exactly replicating the frame
tree, since we just keep a hashmap of current navigation requests.

[nasko, 2014/10/02 00:09:39]

There is no plan to have Navigator be 1-1 with FrameTreeNode. Navigator
represents a context for top-level navigations, so there should only be one for
web pages. The reason it was decoupled from WebContents is that for <webview>,
it represents a top-level navigation context as well, so we can have two
top-level navigations contexts in the same frame tree.

Session history will be refactored as part of Site Isolation to move to browser
process and there will be linear session history entry per frame as well as tree
like session history for the WebContents, which is what ultimately shows up as
the history users see. All the glory details are in the session history design
doc:
https://docs.google.com/a/chromium.org/document/d/1Z-24Xmb5A00eMSfaBB5MQLTrp9...

+
   DISALLOW_COPY_AND_ASSIGN(NavigatorImpl);
 };