Using ResourceRequestBody as the type of HTTP body outside of //content.

content/public/browser/navigation_entry.h

Index: content/public/browser/navigation_entry.h
diff --git a/content/public/browser/navigation_entry.h b/content/public/browser/navigation_entry.h
index a68e66151ba604d5b19e64a974a0d1748a674f30..4ac51e35e055145b712c7e7e5ad0cbd6d42c1173 100644
--- a/content/public/browser/navigation_entry.h
+++ b/content/public/browser/navigation_entry.h
@@ -17,6 +17,7 @@
 #include "content/common/content_export.h"
 #include "content/public/common/page_type.h"
 #include "content/public/common/referrer.h"
+#include "content/public/common/resource_request_body.h"
 #include "ui/base/page_transition_types.h"
 
 class GURL;
@@ -139,7 +140,7 @@ class NavigationEntry {
   // whether the page had post data.
   //
   // The actual post data is stored either in
-  // 1) browser_initiated_post_data when a new post data request is started.
+  // 1) post_data when a new post data request is started.
   // 2) content_state when a post request has started and is extracted by
   //    WebKit to actually make the request.

[Charlie Reis, 2016/06/09 22:38:16]

Is this distinction still accurate?  Sounds like we might put the POST data into
|post_data| for OpenURL cases, in the midst of an ongoing navigation.

Also, I think "content_state" was the name for PageState at the time this was
added (r149232 in 2012).  Presumably the POST data is still in the PageState
after commit?

[Łukasz Anforowicz, 2016/06/10 19:20:50]

Hmmm... you're right - NavigationEntry::Set[BrowserInitiated]PostData method is
only called with non-null data from NavigationControllerImpl::LoadURLWithParams.
 And after the CL under review, LoadURLWithParams can be called with
LOAD_TYPE_[BROWSER_INITIATED]_HTTP_POST in more cases - potentially in cases
where the POST data is already present in PageState (which I think is limited to
reloads and history navigations).

I tried to see if browser-side ever looks at ResourceRequestBody embedded inside
PageState.  This can happen, because PageState is only semi-opaque - it shines
through via ExplodedHttpBody (so I looked at references to
ExplodedHttpBody::request_body.  I see that PlzNavigate extracts the body in
NavigationRequest::CreateBrowserInitiated, but this is fine - this is only the
beginning of a request, and afterwards I think we would not look for POST data
inside PageState.  Other than PlzNavigate, the ExplodedHttpBody::request_body
field is only referenced in code dealing with page/history serialization.

So - based on the previous paragraph, uses of PageState vs
NavigationEntryImpl::[browser_initiated]_post_data_ are not overlapping.  This
makes me feel relatively confident that the CL under review is okay to land
(i.e. shouldn't unexpectedly affect behavior related to code inspecting
PageState).

[Charlie Reis, 2016/06/10 20:59:28]

Ok.  I'm slightly concerned about the opportunity for future bugs if we have
POST data in two places for in-progress navigations (e.g., reloads and history
navigations), but I think there's a path to avoid that-- I'm hoping to make
PageState less opaque in the near future, in which case we can probably unify
them.  This should be fine for now.

[Łukasz Anforowicz, 2016/06/13 18:01:11]

Acknowledged.

   virtual void SetHasPostData(bool has_post_data) = 0;
@@ -149,16 +150,15 @@ class NavigationEntry {
   virtual void SetPostID(int64_t post_id) = 0;
   virtual int64_t GetPostID() const = 0;
 
-  // Holds the raw post data of a browser initiated post request.
+  // Holds the raw post data of a post request.
   // For efficiency, this should be cleared when content_state is populated

[Charlie Reis, 2016/06/09 22:38:16]

Same question.

[Łukasz Anforowicz, 2016/06/10 19:20:50]

I think the comment above is still true.  PageState will be populated *after*
the HTTP request has already been made, so no need to retain ResourceRequestBody
afterwards (in addition to what is inside PageState).  OTOH, I don't know if
this is also true for PlzNavigate (+ whether we actually clear this at the right
moments).

[Charlie Reis, 2016/06/10 20:59:28]

Not sure on PlzNavigate either, but this seems reasonable to me.

[Łukasz Anforowicz, 2016/06/13 18:01:11]

Acknowledged.

   // since the data is duplicated.
   // Note, this field:
   // 1) is not persisted in session restore.
   // 2) is shallow copied with the static copy Create method above.
   // 3) may be nullptr so check before use.
-  virtual void SetBrowserInitiatedPostData(
-      const base::RefCountedMemory* data) = 0;
-  virtual const base::RefCountedMemory* GetBrowserInitiatedPostData() const = 0;
+  virtual void SetPostData(const scoped_refptr<ResourceRequestBody>& data) = 0;
+  virtual scoped_refptr<ResourceRequestBody> GetPostData() const = 0;
 
   // The favicon data and tracking information. See content::FaviconStatus.
   virtual const FaviconStatus& GetFavicon() const = 0;