PlzNavigate: inform the renderer that a navigation is a POST

content/common/navigation_params.h

 // Copyright 2014 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.
 
 #ifndef CONTENT_COMMON_NAVIGATION_PARAMS_H_
 #define CONTENT_COMMON_NAVIGATION_PARAMS_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 41 matching lines @@
                          const Referrer& referrer,
                          ui::PageTransition transition,
                          FrameMsg_Navigate_Type::Value navigation_type,
                          bool allow_download,
                          bool should_replace_current_entry,
                          base::TimeTicks ui_timestamp,
                          FrameMsg_UILoadMetricsReportType::Value report_type,
                          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.
   // PlzNavigate: May be modified when the navigation is ready to commit.
   GURL url;
 
   // The URL to send in the "Referer" header field. Can be empty if there is
   // no referrer.
   Referrer referrer;
 
@@ skipping 33 matching lines @@
   // Whether or not to request a LoFi version of the document or let the browser
   // decide.
   LoFiState lofi_state;
 
   // The navigationStart time exposed through the Navigation Timing API to JS.
   // If this is for a browser-initiated navigation, this can override the
   // navigation_start value in Blink.
   // 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 ----------------------------------------------------
 //
 // 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,
+  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;
 
   // net::URLRequest load flags (net::LOAD_NORMAL) by default).
   int load_flags;
 
   // True if the request was user initiated.
   bool has_user_gesture;
 
   // True if the ServiceWorker should be skipped.
@@ skipping 13 matching lines @@
 
 // Used by FrameMsg_Navigate. Holds the parameters needed by the renderer to
 // start a browser-initiated navigation besides those in CommonNavigationParams.
 // The difference with the RequestNavigationParams 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,
 #if defined(OS_ANDROID)
       bool has_user_gesture,
 #endif
       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;
 
 #if defined(OS_ANDROID)
   bool has_user_gesture;
 #endif
@@ skipping 125 matching lines @@
   ~NavigationParams();
 
   CommonNavigationParams common_params;
   StartNavigationParams start_params;
   RequestNavigationParams request_params;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_NAVIGATION_PARAMS_H_