Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index ed2b611bed047fcbbfaef807bb92a21c478f094b..4b5390e68014fb812380fc270f770995237f1d57 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -547,7 +547,7 @@ WebURLRequest CreateURLRequestForNavigation(
 // to the WebURLRequest used to commit the navigation. This ensures that the
 // POST data will be in the PageState sent to the browser on commit.
 void AddHTTPBodyToRequest(WebURLRequest* request,
-                          scoped_refptr<ResourceRequestBody> body) {
+                          const scoped_refptr<ResourceRequestBody>& body) {
   WebHTTPBody http_body;
   http_body.initialize();
   http_body.setIdentifier(body->identifier());
@@ -639,7 +639,8 @@ CommonNavigationParams MakeCommonNavigationParams(
       request->url(), referrer, extra_data->transition_type(),
       FrameMsg_Navigate_Type::NORMAL, true, should_replace_current_entry,
       ui_timestamp, report_type, GURL(), GURL(), extra_data->lofi_state(),
-      base::TimeTicks::Now(), request->httpMethod().latin1());
+      base::TimeTicks::Now(), request->httpMethod().latin1(),
+      GetRequestBodyForWebURLRequest(*request));
 }
 
 media::Context3D GetSharedMainThreadContext3D(
@@ -1588,7 +1589,7 @@ void RenderFrameImpl::OnNavigate(
   TRACE_EVENT2("navigation", "RenderFrameImpl::OnNavigate", "id", routing_id_,
                "url", common_params.url.possibly_invalid_spec());
   NavigateInternal(common_params, start_params, request_params,
-                   std::unique_ptr<StreamOverrideParameters>(), nullptr);
+                   std::unique_ptr<StreamOverrideParameters>());
 }
 
 void RenderFrameImpl::BindServiceRegistry(
@@ -4726,8 +4727,7 @@ void RenderFrameImpl::OnCommitNavigation(
     const ResourceResponseHead& response,
     const GURL& stream_url,
     const CommonNavigationParams& common_params,
-    const RequestNavigationParams& request_params,
-    scoped_refptr<ResourceRequestBody> post_data) {
+    const RequestNavigationParams& request_params) {
   CHECK(IsBrowserSideNavigationEnabled());
   // This will override the url requested by the WebURLLoader, as well as
   // provide it with the response to the request.
@@ -4737,7 +4737,7 @@ void RenderFrameImpl::OnCommitNavigation(
   stream_override->response = response;
 
   NavigateInternal(common_params, StartNavigationParams(), request_params,
-                   std::move(stream_override), post_data);
+                   std::move(stream_override));
 }
 
 // PlzNavigate
@@ -5333,8 +5333,7 @@ void RenderFrameImpl::NavigateInternal(
     const CommonNavigationParams& common_params,
     const StartNavigationParams& start_params,
     const RequestNavigationParams& request_params,
-    std::unique_ptr<StreamOverrideParameters> stream_params,
-    scoped_refptr<ResourceRequestBody> post_data) {
+    std::unique_ptr<StreamOverrideParameters> stream_params) {
   bool browser_side_navigation = IsBrowserSideNavigationEnabled();
 
   // Lower bound for browser initiated navigation start time.
@@ -5395,8 +5394,8 @@ void RenderFrameImpl::NavigateInternal(
       CreateURLRequestForNavigation(common_params, std::move(stream_params),
                                     frame_->isViewSourceModeEnabled());
 
-  if (IsBrowserSideNavigationEnabled() && post_data)
-    AddHTTPBodyToRequest(&request, post_data);
+  if (IsBrowserSideNavigationEnabled() && common_params.post_data)
+    AddHTTPBodyToRequest(&request, common_params.post_data);
 
   // Used to determine whether this frame is actually loading a request as part
   // of a history navigation.
@@ -5487,26 +5486,17 @@ void RenderFrameImpl::NavigateInternal(
       }
     }
 
-    if (common_params.method == "POST" && !browser_side_navigation) {
-      // Set post data.
-      WebHTTPBody http_body;
-      http_body.initialize();
-      const char* data = nullptr;
-      if (start_params.browser_initiated_post_data.size()) {
-        data = reinterpret_cast<const char*>(
-            &start_params.browser_initiated_post_data.front());
-      }
-      http_body.appendData(
-          WebData(data, start_params.browser_initiated_post_data.size()));
-      request.setHTTPBody(http_body);
-    }
-
     // A session history navigation should have been accompanied by state.
     CHECK_EQ(request_params.page_id, -1);
 
     should_load_request = true;
   }
 
+  if (common_params.method == "POST" && !browser_side_navigation &&
+      common_params.post_data) {
+    AddHTTPBodyToRequest(&request, common_params.post_data);

[clamy, 2016/05/23 16:59:28]

What happens in the reload/history navigation case, if the WebUrlRequest
generated from the history object already has post data?

[Łukasz Anforowicz, 2016/05/23 18:38:47]

You're right - we should only add common_params.post_data to the WebUrlRequest
if we are creating WebUrlRequest from scratch / not when reusing one in case of
a reload or a history navigation.  This incorrect code move was based on my
misunderstanding of the question in the other CR [1] - I've incorrectly thought
the question was a suggestion to add the post body in all the cases (i.e. also
in refresh and history case).  Thanks for pointing out that WebUrlRequest for a
reload or history case would already have post body taken care of.

I've moved the code back under the "else" branch.  Does this look good?  Or is
there something we need to do in case of a reload or a history navigation?

[1]
https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...

[Charlie Reis, 2016/05/23 19:07:26]

Thanks, looks right to me.

[clamy, 2016/05/24 11:46:35]

Thanks lgtm too. My comment in the other CL was really a question about what
should be done in the history/reload case, not a suggestion :).

+  }
+
   if (should_load_request) {
     // Sanitize navigation start now that we know the load_type.
     pending_navigation_params_->common_params.navigation_start =
@@ -5757,8 +5747,7 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
                             GetLoadFlagsForWebURLRequest(*request),
                             request->hasUserGesture(),
                             request->skipServiceWorker(),
-                            GetRequestContextTypeForWebURLRequest(*request)),
-      GetRequestBodyForWebURLRequest(*request)));
+                            GetRequestContextTypeForWebURLRequest(*request))));
 }
 
 void RenderFrameImpl::LoadDataURL(