PlzNavigate: store POST data in the FrameNavigationEntry

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 d45cdaae73337a466a57500d384aa5f935f24aee..ff50ec4b6c599420ebe8c7026f07544738edb7fe 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -150,6 +150,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/http/http_util.h"
+#include "storage/common/data_element.h"
 #include "third_party/WebKit/public/platform/URLConversion.h"
 #include "third_party/WebKit/public/platform/WebCachePolicy.h"
 #include "third_party/WebKit/public/platform/WebData.h"
@@ -537,6 +538,43 @@ WebURLRequest CreateURLRequestForNavigation(
   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) {

[Łukasz Anforowicz, 2016/05/12 02:53:59]

:-)  I've started creating my own version of this code (OTOH, I see that your
version is better - my version for example I didn't copy the identifier :-/). 
https://codereview.chromium.org/1956383003/patch/20001/30017

[clamy, 2016/05/12 08:53:13]

This is based on the code from content/renderer/history_serialization.cc.

[Łukasz Anforowicz, 2016/05/12 19:44:12]

I see.  Fascinating that we have 3 separate structs for essentially the same
thing - ExplodedHttpBodyElement, WebHTTPBody::Element and
ResourceRequestBody::Element.  If those structs truly need to stay the same then
I guess there is no way around having to have separate convertion code... :-(

+  WebHTTPBody http_body;
+  http_body.initialize();
+  http_body.setIdentifier(body->identifier());
+  for (auto element : *(body->elements())) {

[Łukasz Anforowicz, 2016/05/12 02:53:59]

nit: Could you replace auto with: const ResourceRequestBody::Element& ?

[clamy, 2016/05/12 08:53:13]

Done.

+    switch (element.type()) {
+      case storage::DataElement::TYPE_BYTES:
+        http_body.appendData(WebData(
+            reinterpret_cast<const char*>(element.bytes()), element.length()));

[Łukasz Anforowicz, 2016/05/12 02:54:00]

Why is reinterpret_cast needed here?  reinterpret_casts are evil, right?  FWIW,
similar code compiled fine without the cast at
https://codereview.chromium.org/1956383003/patch/20001/30017?

[clamy, 2016/05/12 08:53:13]

Done.

+        break;
+      case storage::DataElement::TYPE_FILE:
+        http_body.appendFileRange(
+            element.path().AsUTF16Unsafe(), element.offset(), element.length(),

[Łukasz Anforowicz, 2016/05/12 02:54:00]

Should we translate back std::numeric_limits<uint64_t>::max() into -1, as done
in the other translation direction at
https://codereview.chromium.org/1907443006/patch/180001/190014 ?  Or will Blink
understand / gracefully deal with this?  I guess this might already work today
because in 2s-complement there is (-1) == static_cast<long
long>(std::numeric_limits<uint64_t>::max()), but it feels better to be explicit.

[clamy, 2016/05/12 08:53:13]

Done.

+            element.expected_modification_time().ToDoubleT());
+        break;
+      case storage::DataElement::TYPE_FILE_FILESYSTEM:
+        http_body.appendFileSystemURLRange(
+            element.filesystem_url(), element.offset(), element.length(),
+            element.expected_modification_time().ToDoubleT());
+        break;
+      case storage::DataElement::TYPE_BLOB:
+        http_body.appendBlob(WebString::fromUTF8(element.blob_uuid()));
+        break;
+      default:

[Łukasz Anforowicz, 2016/05/12 02:53:59]

nit: I wonder if we should explicitly spell out types that we don't expect to
encounter - spelling them out will show future maintainers that we've thought
about these types (rather than having future maintainers wonder if some types
were added after this switch statement).  FWIW, this is what is done in
UploadDataStreamBuilder::Build -
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...

[clamy, 2016/05/12 08:53:13]

Done.

+        NOTREACHED();
+        break;
+    }
+  }
+  request->setHTTPBody(http_body);
+}
+
 // Sanitizes the navigation_start timestamp for browser-initiated navigations,
 // where the browser possibly has a better notion of start time than the
 // renderer. In the case of cross-process navigations, this carries over the
@@ -1501,7 +1539,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>());
+                   std::unique_ptr<StreamOverrideParameters>(), nullptr);
 }
 
 void RenderFrameImpl::BindServiceRegistry(
@@ -4638,7 +4676,8 @@ void RenderFrameImpl::OnCommitNavigation(
     const ResourceResponseHead& response,
     const GURL& stream_url,
     const CommonNavigationParams& common_params,
-    const RequestNavigationParams& request_params) {
+    const RequestNavigationParams& request_params,
+    scoped_refptr<ResourceRequestBody> post_data) {
   CHECK(IsBrowserSideNavigationEnabled());
   // This will override the url requested by the WebURLLoader, as well as
   // provide it with the response to the request.
@@ -4648,7 +4687,7 @@ void RenderFrameImpl::OnCommitNavigation(
   stream_override->response = response;
 
   NavigateInternal(common_params, StartNavigationParams(), request_params,
-                   std::move(stream_override));
+                   std::move(stream_override), post_data);
 }
 
 // PlzNavigate
@@ -5244,7 +5283,8 @@ void RenderFrameImpl::NavigateInternal(
     const CommonNavigationParams& common_params,
     const StartNavigationParams& start_params,
     const RequestNavigationParams& request_params,
-    std::unique_ptr<StreamOverrideParameters> stream_params) {
+    std::unique_ptr<StreamOverrideParameters> stream_params,
+    scoped_refptr<ResourceRequestBody> post_data) {
   bool browser_side_navigation = IsBrowserSideNavigationEnabled();
 
   // Lower bound for browser initiated navigation start time.
@@ -5305,6 +5345,9 @@ void RenderFrameImpl::NavigateInternal(
       CreateURLRequestForNavigation(common_params, std::move(stream_params),
                                     frame_->isViewSourceModeEnabled());
 
+  if (IsBrowserSideNavigationEnabled() && post_data)
+    AddHTTPBodyToRequest(&request, 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;