Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 529 matching lines @@
           common_params.report_type));
   return request;
 }
 
 // Converts the HTTP body data stored in ResourceRequestBody format to a
 // WebHTTPBody, which is then added to the WebURLRequest.
 // PlzNavigate: used to add the POST data sent by the renderer at commit time
 // 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());
   for (const ResourceRequestBody::Element& element : *(body->elements())) {
     long long length = -1;
     switch (element.type()) {
       case storage::DataElement::TYPE_BYTES:
         http_body.appendData(WebData(element.bytes(), element.length()));
         break;
       case storage::DataElement::TYPE_FILE:
@@ skipping 71 matching lines @@
       static_cast<FrameMsg_UILoadMetricsReportType::Value>(
           request->inputPerfMetricReportPolicy());
 
   const RequestExtraData* extra_data =
       static_cast<RequestExtraData*>(request->getExtraData());
   DCHECK(extra_data);
   return CommonNavigationParams(
       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(
     scoped_refptr<ContextProviderCommandBuffer> provider) {
   if (!provider)
     return media::Context3D();
   return media::Context3D(provider->ContextGL(), provider->GrContext());
 }
 
 bool IsReload(FrameMsg_Navigate_Type::Value navigation_type) {
@@ skipping 928 matching lines @@
   }
 
   RenderThreadImpl* render_thread_impl = RenderThreadImpl::current();
   // Can be NULL in tests.
   if (render_thread_impl)
     render_thread_impl->GetRendererScheduler()->OnNavigationStarted();
   DCHECK(!IsBrowserSideNavigationEnabled());
   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(
     shell::mojom::InterfaceProviderRequest services,
     shell::mojom::InterfaceProviderPtr exposed_services) {
   service_registry_.Bind(std::move(services));
   service_registry_.BindRemoteServiceProvider(std::move(exposed_services));
 }
 
 ManifestManager* RenderFrameImpl::manifest_manager() {
@@ skipping 3117 matching lines @@
 void RenderFrameImpl::FocusedNodeChangedForAccessibility(const WebNode& node) {
   if (renderer_accessibility())
     renderer_accessibility()->AccessibilityFocusedNodeChanged(node);
 }
 
 // PlzNavigate
 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.
   std::unique_ptr<StreamOverrideParameters> stream_override(
       new StreamOverrideParameters());
   stream_override->stream_url = stream_url;
   stream_override->response = response;
 
   NavigateInternal(common_params, StartNavigationParams(), request_params,
-                   std::move(stream_override), post_data);
+                   std::move(stream_override));
 }
 
 // PlzNavigate
 void RenderFrameImpl::OnFailedNavigation(
     const CommonNavigationParams& common_params,
     const RequestNavigationParams& request_params,
     bool has_stale_copy_in_cache,
     int error_code) {
   DCHECK(IsBrowserSideNavigationEnabled());
   bool is_reload = IsReload(common_params.navigation_type);
@@ skipping 575 matching lines @@
     params.frame_unique_name = frame_->uniqueName().utf8();
   }
 
   Send(new FrameHostMsg_OpenURL(routing_id_, params));
 }
 
 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.
   base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
   bool is_reload = IsReload(common_params.navigation_type);
   bool is_history_navigation = request_params.page_state.IsValid();
   WebCachePolicy cache_policy = WebCachePolicy::UseProtocolCachePolicy;
   RenderFrameImpl::PrepareRenderViewForNavigation(
       common_params.url, request_params);
 
@@ skipping 40 matching lines @@
           ? blink::WebFrameLoadType::ReplaceCurrentItem
           : blink::WebFrameLoadType::Standard;
   blink::WebHistoryLoadType history_load_type =
       blink::WebHistoryDifferentDocumentLoad;
   bool should_load_request = false;
   WebHistoryItem item_for_history_navigation;
   WebURLRequest request =
       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.
   bool has_history_navigation_in_frame = false;
 
 #if defined(OS_ANDROID)
   request.setHasUserGesture(start_params.has_user_gesture);
 #endif
 
   // PlzNavigate: Make sure that Blink's loader will not try to use browser side
@@ skipping 70 matching lines @@
     if (!start_params.extra_headers.empty() && !browser_side_navigation) {
       for (net::HttpUtil::HeadersIterator i(start_params.extra_headers.begin(),
                                             start_params.extra_headers.end(),
                                             "\n");
            i.GetNext();) {
         request.addHTTPHeaderField(WebString::fromUTF8(i.name()),
                                    WebString::fromUTF8(i.values()));
       }
     }
 
-    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 =
         SanitizeNavigationTiming(load_type, common_params.navigation_start,
                                  renderer_navigation_start);
 
     // PlzNavigate: check if the navigation being committed originated as a
     // client redirect.
     bool is_client_redirect =
         browser_side_navigation
@@ skipping 230 matching lines @@
          GetRequestContextFrameTypeForWebURLRequest(*request) ==
              REQUEST_CONTEXT_FRAME_TYPE_NESTED);
 
   Send(new FrameHostMsg_BeginNavigation(
       routing_id_,
       MakeCommonNavigationParams(request, should_replace_current_entry),
       BeginNavigationParams(GetWebURLRequestHeaders(*request),
                             GetLoadFlagsForWebURLRequest(*request),
                             request->hasUserGesture(),
                             request->skipServiceWorker(),
-                            GetRequestContextTypeForWebURLRequest(*request)),
-      GetRequestBodyForWebURLRequest(*request)));
+                            GetRequestContextTypeForWebURLRequest(*request))));
 }
 
 void RenderFrameImpl::LoadDataURL(
     const CommonNavigationParams& params,
     const RequestNavigationParams& request_params,
     WebLocalFrame* frame,
     blink::WebFrameLoadType load_type,
     blink::WebHistoryItem item_for_history_navigation,
     blink::WebHistoryLoadType history_load_type,
     bool is_client_redirect) {
@@ skipping 432 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content