PlzNavigate: inform the renderer that a navigation is a POST

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 414319d73e384c9c27d28eea402f62596c0e3b65..1d7770c543da06b186b512e11de7e352e7d5c289 100644
--- a/content/common/navigation_params.h
+++ b/content/common/navigation_params.h
@@ -59,7 +59,8 @@ struct CONTENT_EXPORT CommonNavigationParams {
                          const GURL& base_url_for_data_url,
                          const GURL& history_url_for_data_url,
                          LoFiState lofi_state,
-                         const base::TimeTicks& navigation_start);
+                         const base::TimeTicks& navigation_start,
+                         bool is_post);

[Charlie Reis, 2016/02/17 05:55:28]

I would think that moving this to CommonNavParams implies it gets used on the
commit side as well as on the request side (as opposed to RequestNavParams,
which get used both by default and in PlzNavigate).

Is it used during commit?  Or is there another reason to move it?  Maybe it's so
that it can go browser to renderer (like RequestNavParams) and renderer to
browser (like BeginNavParams)?

[clamy, 2016/02/17 12:39:39]

It is used both at request time in the browser, and at commit time in the
renderer:
- request time: we need to know the method to do the HTTP request
- commit time: we need to send in DidCommitProvisionalLoad whether the
navigation was a POST or not

[Charlie Reis, 2016/02/17 21:52:06]

Ah, I'm missing the second part.  Where do we use it in
DidCommitProvisionalLoad?

Actually, I see a different is_post in
FrameHostMsg_DidCommitProvisionalLoad_Params, which gets assigned in
SendDidCommitProvisionalLoad.  Should that somehow be replaced by this one? 
(Then again, that's from the previous patch set.  Does the new |method|
parameter get used on commit, and how?  Does it ever get sent from renderer to
browser?)

Apologies if I'm misunderstanding the way CommonNavParams gets used.  The part
that's confusing me is that the comment above says CommonNavParams is "Used by
all navigation IPCs," but it doesn't seem to be sent in DidCommit.

[clamy, 2016/02/22 12:48:36]

Yes it seems the comment above CommonNavParams is a bit misleading. They don't
get used in DidCommitProvisionalLoad, since DidCommitProvisionalLoad already
uses a NavigationParameters struct as a base. We could probably unify that, but
it has not been done yet.

The is_post parameter in DidCommitProvisionalLoad is how the information about
the method gets used at commit time (we get it from the method of the WebRequest
that was initially set using the method param from CommonNavigationParams).

I'm not changing it right now, since I plan to make a next patch that will move
the knowledge of whether was a POST or not to the FrameNavigationEntry, and I
thought I would change the is_post parameter to method at that time.

[Charlie Reis, 2016/02/27 00:21:49]

It's fine if this isn't used at commit time, but then it seems like it would
belong in RequestNavigationParams (i.e., "Parameters used both in the current
architecture and PlzNavigate" and sent only from browser to renderer).

I'll assume that it still belongs in CommonNavParams because it's used in both
browser-to-renderer (RequestNavParams) and renderer-to-browser (BeignNavParams)
cases.  Feel free to proceed if that's correct.
I'm cautious about moving that from NavEntry to FrameNavEntry because of the
possible implications to serialized session history on disk.  There's probably a
safe way to proceed and we can discuss that separately, though; no need to hold
this up.

[clamy, 2016/02/29 10:54:02]

That's indeed the case.
OK, I probably won't get started started on this right away though, so there
will be time to see how we can safely proceed.

   ~CommonNavigationParams();
 
   // The URL to navigate to.
@@ -113,6 +114,9 @@ struct CONTENT_EXPORT CommonNavigationParams {
   // PlzNavigate: For renderer initiated navigations, this will be set on the
   // renderer side and sent with FrameHostMsg_BeginNavigation.
   base::TimeTicks navigation_start;
+
+  // Whether the navigation is a POST request (as opposed to a GET).
+  bool is_post;
 };
 
 // Provided by the renderer ----------------------------------------------------
@@ -128,16 +132,12 @@ 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,
+  BeginNavigationParams(std::string headers,
                         int load_flags,
                         bool has_user_gesture,
                         bool skip_service_worker,
                         RequestContextType request_context_type);
 
-  // The request method: GET, POST, etc.

[Charlie Reis, 2016/02/17 05:55:28]

Again, I wonder about whether the "etc" was intentional or if GET and POST are
really the only things possible.

[clamy, 2016/02/17 12:39:39]

Changed the boolean to a string.

-  std::string method;
-
   // Additional HTTP request headers.
   std::string headers;
 
@@ -171,7 +171,6 @@ struct CONTENT_EXPORT BeginNavigationParams {
 struct CONTENT_EXPORT StartNavigationParams {
   StartNavigationParams();
   StartNavigationParams(
-      bool is_post,
       const std::string& extra_headers,
       const std::vector<unsigned char>& browser_initiated_post_data,
 #if defined(OS_ANDROID)
@@ -181,9 +180,6 @@ struct CONTENT_EXPORT StartNavigationParams {
       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;