PlzNavigate: store POST data in the FrameNavigationEntry

content/common/resource_request_body.cc

 // 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.
 
 #include "content/common/resource_request_body.h"
 
+#include "base/strings/utf_string_conversions.h"
+#include "content/common/page_state_serialization.h"
+
+using blink::WebHTTPBody;
+using blink::WebString;
+
 namespace content {
 
 ResourceRequestBody::ResourceRequestBody()
     : identifier_(0) {
 }
 
+void ResourceRequestBody::AppendExplodedHTTPBodyElement(
+    const ExplodedHttpBodyElement& element) {
+  switch (element.type) {
+    case WebHTTPBody::Element::TypeData:
+      if (!element.data.empty()) {
+        // Blink sometimes gives empty data to append. These aren't
+        // necessary so they are just optimized out here.
+        AppendBytes(element.data.data(), static_cast<int>(element.data.size()));

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

It is not immediately clear why size_t -> int cast won't lead to trouble.

Is there something here or elsewhere that guarantees that the size will fit into
an int?  If yes, could you please add a DCHECK + a comment explaining this?

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

This whole method is a copy of what we do to convert a WebHTTPBody to a
ResourceRequestBody. It appears all the underlying code (ie ResourceRequestBody
and DataElement) expect an int there, hence the cast. In case it ends up being
negative, nothing will be added to the body.

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

It is pretty sad that we have to repeat almost the same code, just because
ExplodedHttpBodyElement and WebHTTPBody::Element are different structs (they
have almost the same fields - they both have blink::WebHTTPBody::Element::Type
type field and the other fields just replace blink-types with base or std types
- i.e. WebString with std::string).

That said, I guess your CL hasn't introduced these separate structs, so there
isn't really anything to be done in the current CL...

[Charlie Reis, 2016/05/16 21:16:42]

Please add a comment above this method indicating where it's copied from.  Even
with all this context, I had a hard time finding it and thought you might have
been copying from AppendHTTPBodyElement in history_serialization.cc instead of
GetRequestBodyForWebURLRequest in web_url_request_util.cc.  It's useful info in
case we find a way to refactor them to share code in the future.

[clamy, 2016/05/19 13:11:57]

Done.

+      }
+      break;
+    case WebHTTPBody::Element::TypeFile:
+      if (element.file_length == -1) {
+        AppendFileRange(
+            base::FilePath::FromUTF16Unsafe(element.file_path.string()), 0,
+            std::numeric_limits<uint64_t>::max(), base::Time());

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

I see that UploadFileElementReader::OnGetFileInfoCompleted should deal with
max_uint64_t fine, but I wonder 1) if other consumers of ResourceRequestBody are
aware of this convention and 2) if this convention should be documented in a
comment somewhere (maybe in declarations of ResourceRequestBody::AppendFileRange
and ResourceRequestBody::AppendFileSystemFileRange;  maybe next to fields or
accessors of storage::DataElement?).

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

Possibly? As explained above, I'm just copy pasting the code for
GetRequestBodyForWebURLRequest from web_url_request_util.cc (it used to be in
weburlloaderimpl before being moved). So since we instantiate it that way there,
I  expect consumers of ResourceRequestBody to be aware of it.

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

Acknowledged.

+      } else {
+        AppendFileRange(
+            base::FilePath::FromUTF16Unsafe(element.file_path.string()),
+            static_cast<uint64_t>(element.file_start),
+            static_cast<uint64_t>(element.file_length),
+            base::Time::FromDoubleT(element.file_modification_time));

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

Just wanted to double-check - is it okay to use base::Time() above, but need to
use base::Time::FromDoubleT(element.file_modification_time) here?  i.e. the
expected file_modification_time is only applicable to file ranges, but never
applicable to whole files?  If so, then it would be great if this was explained
in a comment somewhere in WebHTTPBody. (although it seems that Blink's
historical style is to have as little comments as possible... :-( ).

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

Again I imagine it's ok since we've been doing it. Yes WebKit style guide on the
comment part is questionnable, though this is based on code coming from content/
so it could have come with a little more comments...

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

Acknowledged.

[Charlie Reis, 2016/05/16 21:16:41]

In general, copy/pasting code isn't an excuse for ignoring issues with it (in
terms of bugs or missing comments).  :)  It's even more important in those
cases, since a bug fix in the original location likely won't get applied here as
well.

Given how these various conversion functions are scattered around the code
without references to each other, I think it's worth understanding whether there
are issues that need to be fixed and leaving comments where possible.

[Łukasz Anforowicz, 2016/05/18 00:22:27]

I wonder if we could avoid copy&pasted conversion code (here and also in
render_frame_impl.cc) by getting rid of ExplodedHttpBodyElement as a separate
type (and making it a type alias for ResourceRequestBody::Element instead). 
I've tried putting together a quick (and very dirty for now - some things still
don't compile) CL that does this at https://crrev.com/1987053002.

[clamy, 2016/05/19 13:11:57]

So I did a bit of code archeology, and the time parameter there was introduced
in https://codereview.chromium.org/594036/ in 2010. Based on the comments in the
code review, it seems it was meant as a validator when they introduced file
ranges, and the time value for whole files was set to base::Time() there.

I'd like to avoid making modifications to the HTTP post data code used outside
of PlzNavigate, so if we want to modify this, I'd prefer to do it in a separate
CL (and I'd like to keep this part matching the code in
web_url_request_utils.cc).

[Charlie Reis, 2016/05/19 17:43:48]

Agreed.  Since we aren't certain there's a bug here, let's leave any further
investigation until later.  Hopefully Lukasz will be able to eliminate
ExplodedHttpBodyElement in his CL.

+      }
+      break;
+    case WebHTTPBody::Element::TypeFileSystemURL: {
+      GURL file_system_url = element.filesystem_url;
+      DCHECK(file_system_url.SchemeIsFileSystem());

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

I guess a DCHECK is sufficient, because UploadFileSystemFileElementReader::Init
will sanitize the url?

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

I imagine. I don't know if we want to crash the renderer over this (and honestly
if it has to be, I'd rather we use a bad message when receiving the message).

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

Acknowledged.

+      AppendFileSystemFileRange(
+          file_system_url, static_cast<uint64_t>(element.file_start),
+          static_cast<uint64_t>(element.file_length),
+          base::Time::FromDoubleT(element.file_modification_time));
+      break;
+    }
+    case WebHTTPBody::Element::TypeBlob:
+      AppendBlob(element.blob_uuid);
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
 void ResourceRequestBody::AppendBytes(const char* bytes, int bytes_len) {
   if (bytes_len > 0) {
     elements_.push_back(Element());
     elements_.back().SetToBytes(bytes, bytes_len);
   }
 }
 
 void ResourceRequestBody::AppendFileRange(
     const base::FilePath& file_path,
     uint64_t offset,
@@ skipping 12 matching lines @@
 void ResourceRequestBody::AppendFileSystemFileRange(
     const GURL& url,
     uint64_t offset,
     uint64_t length,
     const base::Time& expected_modification_time) {
   elements_.push_back(Element());
   elements_.back().SetToFileSystemUrlRange(url, offset, length,
                                            expected_modification_time);
 }
 
+scoped_refptr<ResourceRequestBody> ResourceRequestBody::MakeCopy() {
+  scoped_refptr<ResourceRequestBody> copy = new ResourceRequestBody();
+  copy->set_identifier(identifier_);
+  for (auto element : elements_)
+    copy->elements_mutable()->push_back(element);
+  return copy;
+}
+
 ResourceRequestBody::~ResourceRequestBody() {
 }
 
 }  // namespace content