Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

content/public/common/page_state.cc

 // Copyright (c) 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/public/common/page_state.h"
 
 #include <stddef.h>
 
 #include "base/files/file_path.h"
 #include "base/strings/nullable_string16.h"
@@ skipping 67 matching lines @@
 PageState PageState::CreateForTesting(
     const GURL& url,
     bool body_contains_password_data,
     const char* optional_body_data,
     const base::FilePath* optional_body_file_path) {
   ExplodedPageState state;
 
   state.top.url_string = ToNullableString16(url.possibly_invalid_spec());
 
   if (optional_body_data || optional_body_file_path) {
-    state.top.http_body.is_null = false;
     if (optional_body_data) {
-      ExplodedHttpBodyElement element;
       std::string body_data(optional_body_data);
-      element.SetToBytes(body_data.data(), body_data.size());
-      state.top.http_body.elements.push_back(element);
+      state.top.http_body.request_body = new ResourceRequestBody();
+      state.top.http_body.request_body->AppendBytes(body_data.data(),
+                                                    body_data.size());
     }
     if (optional_body_file_path) {

[Łukasz Anforowicz, 2016/05/26 16:26:52]

This branch is taken by some tests.  I assume that these tests only care about
|state.referenced_files|.

[Charlie Reis, 2016/05/27 21:45:53]

Oh, interesting.  I see that this old code never appended the element to
|state|, which suggests that they didn't bother to check it.

However, I suspect that will be a headache as I try to fix
https://crbug.com/613260.  In that bug (which I'll soon post an update to),
we're failing to track the referenced files per frame, so we end up killing
OOPIFs when they have files in the PageState.  I'm hoping to eliminate
state.referenced_files (which fundamentally can't track files per process in an
OOPIF world) in favor of looking at the actual files in the ResourceRequestBody.

Can you go ahead and add a file element here?  (If it causes problems, I can
revisit it later.)

[Łukasz Anforowicz, 2016/05/31 16:38:04]

Done - I should have written it this way from the start - this more faithfully
mimics what the production code would have done.  I guess I was surprised by the
no-op code here and this is why I've just removed it initially...
I don't foresee problems for this CL, because currently no test looks at the
files inside the request body being created here.  OTOH, this means that the
code I've added doesn't have test coverage and can be already incorrect and/or
rot in the future... :-/

Still - any verification I could add right now would be tautological (i.e. I
would just verify that the test matches what I wrote), so probably best to just
let it be.

-      ExplodedHttpBodyElement element;
-      element.SetToFilePath(*optional_body_file_path);
       state.referenced_files.push_back(base::NullableString16(
           optional_body_file_path->AsUTF16Unsafe(), false));
     }
     state.top.http_body.contains_passwords =
         body_contains_password_data;
   }
 
   return ToPageState(state);
 }
 
@@ skipping 56 matching lines @@
 }
 
 PageState::PageState(const std::string& data)
     : data_(data) {
   // TODO(darin): Enable this DCHECK once tests have been fixed up to not pass
   // bogus encoded data to CreateFromEncodedData.
   //DCHECK(IsValid());
 }
 
 }  // namespace content