Make NavigationParams clearer

content/common/navigation_params.h

Index: content/common/navigation_params.h
diff --git a/content/common/navigation_params.h b/content/common/navigation_params.h
index 9b3fccf4ab78e61b656a68d5e077f0c0a1611989..69ab928b192f7f7a324212f7652bbe41ef659a51 100644
--- a/content/common/navigation_params.h
+++ b/content/common/navigation_params.h
@@ -21,12 +21,13 @@ class RefCountedMemory;
 }
 
 namespace content {
-class NavigationEntry;
 
 // The following structures hold parameters used during a navigation. In
 // particular they are used by FrameMsg_Navigate, FrameMsg_CommitNavigation and
 // FrameHostMsg_BeginNavigation.
 
+// Provided by the browser or the renderer -------------------------------------
+
 // Used by all navigation IPCs.
 struct CONTENT_EXPORT CommonNavigationParams {
   CommonNavigationParams();
@@ -76,31 +77,57 @@ struct CONTENT_EXPORT CommonNavigationParams {
   GURL history_url_for_data_url;
 };
 
-// PlzNavigate: parameters needed to start a navigation on the IO thread.
-struct CONTENT_EXPORT BeginNavigationParams {
-  // TODO(clamy): See if it is possible to reuse this in
-  // ResourceMsg_Request_Params.
-  BeginNavigationParams();
-  BeginNavigationParams(std::string method,
-                        std::string headers,
-                        int load_flags,
-                        bool has_user_gesture);
+// Provided by the browser -----------------------------------------------------
+//
+// These structs are sent by the browser to the renderer to start (commit with

[Charlie Reis, 2015/03/19 20:45:52]

I don't know what "start (commit with PlzNavigate enabled)" means.  Can you
rephrase it?

[clamy, 2015/03/20 10:42:43]

Is it clearer now?

+// PlzNavigate enabled) a navigation. Parameters used both in the current
+// architecture and PlzNavigate should be put in CommitNavigationParams.
+// Parameters only used by the current architecture should go in
+// StartNavigationParams.
+
+// Used by FrameMsg_Navigate, holds the parameters needed by the renderer to

[Charlie Reis, 2015/03/19 20:45:51]

nit: New sentence for "Holds..."

[clamy, 2015/03/20 10:42:43]

Done.

+// start a browser-initiated navigation besides those in CommonNavigationParams.
+// The difference with the CommitNavigationParams below is that they are only
+// used in the current architecture of navigation, and will not be used by
+// PlzNavigate.
+// PlzNavigate: These are not used.
+struct CONTENT_EXPORT StartNavigationParams {
+  StartNavigationParams();
+  StartNavigationParams(
+      bool is_post,
+      const std::string& extra_headers,
+      const std::vector<unsigned char>& browser_initiated_post_data,
+      bool should_replace_current_entry,
+      int transferred_request_child_id,
+      int transferred_request_request_id);
+  ~StartNavigationParams();
 
-  // The request method: GET, POST, etc.
-  std::string method;
+  // Whether the navigation is a POST request (as opposed to a GET).
+  bool is_post;
 
-  // Additional HTTP request headers.
-  std::string headers;
+  // Extra headers (separated by \n) to send during the request.
+  std::string extra_headers;
 
-  // net::URLRequest load flags (net::LOAD_NORMAL) by default).
-  int load_flags;
+  // If is_post is true, holds the post_data information from browser. Empty
+  // otherwise.
+  std::vector<unsigned char> browser_initiated_post_data;
 
-  // True if the request was user initiated.
-  bool has_user_gesture;
+  // Informs the RenderView the pending navigation should replace the current
+  // history entry when it commits. This is used for cross-process redirects so
+  // the transferred navigation can recover the navigation state.
+  bool should_replace_current_entry;
+
+  // The following two members identify a previous request that has been
+  // created before this navigation is being transferred to a new render view.
+  // This serves the purpose of recycling the old request.
+  // Unless this refers to a transferred navigation, these values are -1 and -1.
+  int transferred_request_child_id;
+  int transferred_request_request_id;
 };
 
-// Used by FrameMsg_Navigate.
-// PlzNavigate: sent to the renderer when the navigation is ready to commit.
+// Used by FrameMsg_Navigate, holds the parameters needed by the renderer to

[Charlie Reis, 2015/03/19 20:45:51]

nit: New sentence for "Holds..."

[clamy, 2015/03/20 10:42:43]

Done.

+// start a browser-initiated navigation besides those in CommonNavigationParams.

[Charlie Reis, 2015/03/19 20:45:51]

This doesn't sound right.  Why are "commit" parameters needed to "start" a
navigation?

I've had trouble in the past with using the word "commit" in PlzNavigate for a
stage before the actual commit happens.  This is a much bigger problem now that
we're using these parameters in the current architecture for the start of a
navigation.  That happens well before anything is ready to commit, and in some
cases the commit won't ever happen (e.g., downloads, 204s).

I would really like to find a different name for this struct if we're going to
use it for starting a navigation.  Maybe something like RequestNavigationParams?
 (I'm open to suggestions, as long as they indicate that this is to make the
renderer process start a navigation attempt and not commit one.)

(Even in the PlzNavigate case, this struct is about getting the new renderer
process to issue a fake request for the stream that is ready to commit, right? 
The commit itself happens after that, asynchronously.  And if I understand
correctly, we hope to eliminate that fake request eventually, which might change
what's in here.)

[clamy, 2015/03/20 10:42:43]

Done. They were initially called CommitNavigationParams, since there used to be
some RequestNavigationParams when we still had a RequestNavigation IPC. Now that
it is gone, they can be called RequestNavigationParams.

+// PlzNavigate: sent to the renderer to commit a navigation.
 struct CONTENT_EXPORT CommitNavigationParams {
   CommitNavigationParams();
   CommitNavigationParams(bool is_overriding_user_agent,
@@ -108,7 +135,13 @@ struct CONTENT_EXPORT CommitNavigationParams {
                          const std::vector<GURL>& redirects,
                          bool can_load_local_resources,
                          const std::string& frame_to_navigate,
-                         base::Time request_time);
+                         base::Time request_time,
+                         const PageState& page_state,
+                         int32 page_id,
+                         int pending_history_list_offset,
+                         int current_history_list_offset,
+                         int current_history_list_length,
+                         bool should_clear_history_list);
   ~CommitNavigationParams();
 
   // Whether or not the user agent override string should be used.
@@ -132,19 +165,6 @@ struct CONTENT_EXPORT CommitNavigationParams {
   // infrastructure to set up DocumentState associated with the RenderView.
   // TODO(ppi): make it go away.
   base::Time request_time;
-};
-
-// Used by FrameMsg_Navigate.
-// PlzNavigate: sent to the renderer when the navigation is ready to commit.
-struct CONTENT_EXPORT HistoryNavigationParams {
-  HistoryNavigationParams();
-  HistoryNavigationParams(const PageState& page_state,
-                          int32 page_id,
-                          int pending_history_list_offset,
-                          int current_history_list_offset,
-                          int current_history_list_length,
-                          bool should_clear_history_list);
-  ~HistoryNavigationParams();
 
   // Opaque history state (received by ViewHostMsg_UpdateState).
   PageState page_state;
@@ -170,54 +190,50 @@ struct CONTENT_EXPORT HistoryNavigationParams {
   bool should_clear_history_list;
 };
 
-// Parameters needed at the start of a navigation. Used by FrameMsg_Navigate.
-// PlzNavigate: these parameters are not used in navigation.
-struct CONTENT_EXPORT StartNavigationParams {
-  StartNavigationParams();
-  StartNavigationParams(
-      bool is_post,
-      const std::string& extra_headers,
-      const std::vector<unsigned char>& browser_initiated_post_data,
-      bool should_replace_current_entry,
-      int transferred_request_child_id,
-      int transferred_request_request_id);
-  ~StartNavigationParams();
-
-  // Whether the navigation is a POST request (as opposed to a GET).
-  bool is_post;
-
-  // Extra headers (separated by \n) to send during the request.
-  std::string extra_headers;
-
-  // If is_post is true, holds the post_data information from browser. Empty
-  // otherwise.
-  std::vector<unsigned char> browser_initiated_post_data;
-
-  // Informs the RenderView the pending navigation should replace the current
-  // history entry when it commits. This is used for cross-process redirects so
-  // the transferred navigation can recover the navigation state.
-  bool should_replace_current_entry;
-
-  // The following two members identify a previous request that has been
-  // created before this navigation is being transferred to a new render view.
-  // This serves the purpose of recycling the old request.
-  // Unless this refers to a transferred navigation, these values are -1 and -1.
-  int transferred_request_child_id;
-  int transferred_request_request_id;
-};
-
+// Helper struct keeping track in one place of all the parameters the browser
+// need to provide to the renderer.

[Charlie Reis, 2015/03/19 20:45:51]

nit: needs

[clamy, 2015/03/20 10:42:43]

Done.

 struct NavigationParams {
   NavigationParams(const CommonNavigationParams& common_params,
                    const StartNavigationParams& start_params,
-                   const CommitNavigationParams& commit_params,
-                   const HistoryNavigationParams& history_params);
+                   const CommitNavigationParams& commit_params);
   ~NavigationParams();
 
   CommonNavigationParams common_params;
   StartNavigationParams start_params;
   CommitNavigationParams commit_params;
-  HistoryNavigationParams history_params;
 };
+
+// Provided by the renderer ----------------------------------------------------

[Charlie Reis, 2015/03/19 20:45:51]

What would you think about putting this before the "Provided by the browser"
section?  This happens earlier in the timeline, so I think it makes sense to
come before those.

[clamy, 2015/03/20 10:42:43]

Done.

+//
+// This struct holds parameters sent by the renderer to the browser. It is only
+// used in PlzNavigate (since in the current architecture, the renderer does not
+// inform the browser of navigations until they commit).
+
+// This struct is not used outside of the PlzNavigate project.
+// PlzNavigate: parameters needed to start a navigation on the IO thread,
+// following a renderer-initiated navigation request.
+struct CONTENT_EXPORT BeginNavigationParams {
+  // TODO(clamy): See if it is possible to reuse this in
+  // ResourceMsg_Request_Params.
+  BeginNavigationParams();
+  BeginNavigationParams(std::string method,
+                        std::string headers,
+                        int load_flags,
+                        bool has_user_gesture);
+
+  // The request method: GET, POST, etc.
+  std::string method;
+
+  // Additional HTTP request headers.
+  std::string headers;
+
+  // net::URLRequest load flags (net::LOAD_NORMAL) by default).
+  int load_flags;
+
+  // True if the request was user initiated.
+  bool has_user_gesture;
+};
+
 }  // namespace content
 
 #endif  // CONTENT_COMMON_NAVIGATION_PARAMS_H_