Skip to content

Commit

Permalink
Deduplicating code performing WebHTTPBody::Element conversions.
Browse files Browse the repository at this point in the history
Before this CL, there were 2 separate structs in //content layer holding
essentially the same information about parts of http body:
ResourceRequestBody::Element and ExplodedHttpBodyElement.

To deal with 2 separate structs, recent CLs had to introduce duplicated,
copy & pasted code that performed conversions between WebHTTPBody::Element
from the Blink layer and the 2 structs in the //content layer.
Examples:

1. https://crrev.com/1956383003:
   1.a. AddHTTPBodyToRequest in //content/renderer/render_frame_impl.cc
        duplicated code from //content/renderer/history_serialization.cc

2. https://crrev.com/1907443006:
   2.a. Almost exactly the same as above, AddHTTPBodyToRequest in
        //content/renderer/render_frame_impl.cc duplicated code from
        //content/renderer/history_serialization.cc
   2.b. ResourceRequestBody::AppendExplodedHTTPBodyElement in
        //content/common/resource_request_body.cc was duplicating

The current CL removes the duplicated code by making
ExplodedHttpBodyElement a type alias for ResourceRequestBody::Element
and removing code that dealt with the old ExplodedHttpBodyElement
struct.

BUG=582211

Review-Url: https://codereview.chromium.org/1987053002
Cr-Commit-Position: refs/heads/master@{#395461}
  • Loading branch information
anforowicz authored and Commit bot committed May 23, 2016
1 parent 9d6cc87 commit 2d0c9ae
Show file tree
Hide file tree
Showing 11 changed files with 197 additions and 224 deletions.
92 changes: 44 additions & 48 deletions content/common/page_state_serialization.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <limits>

#include "base/pickle.h"
#include "base/strings/nullable_string16.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
Expand All @@ -30,8 +31,7 @@ float g_device_scale_factor_for_testing = 0.0;
void AppendDataToHttpBody(ExplodedHttpBody* http_body, const char* data,
int data_length) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeData;
element.data.assign(data, data_length);
element.SetToBytes(data, data_length);
http_body->elements.push_back(element);
}

Expand All @@ -41,11 +41,10 @@ void AppendFileRangeToHttpBody(ExplodedHttpBody* http_body,
int file_length,
double file_modification_time) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeFile;
element.file_path = file_path;
element.file_start = file_start;
element.file_length = file_length;
element.file_modification_time = file_modification_time;
element.SetToFilePathRange(
base::FilePath::FromUTF16Unsafe(file_path.string()),
static_cast<uint64_t>(file_start), static_cast<uint64_t>(file_length),
base::Time::FromDoubleT(file_modification_time));
http_body->elements.push_back(element);
}

Expand All @@ -55,19 +54,17 @@ void AppendURLRangeToHttpBody(ExplodedHttpBody* http_body,
int file_length,
double file_modification_time) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeFileSystemURL;
element.filesystem_url = url;
element.file_start = file_start;
element.file_length = file_length;
element.file_modification_time = file_modification_time;
element.SetToFileSystemUrlRange(
url, static_cast<uint64_t>(file_start),
static_cast<uint64_t>(file_length),
base::Time::FromDoubleT(file_modification_time));
http_body->elements.push_back(element);
}

void AppendBlobToHttpBody(ExplodedHttpBody* http_body,
const std::string& uuid) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeBlob;
element.blob_uuid = uuid;
element.SetToBlob(uuid);
http_body->elements.push_back(element);
}

Expand All @@ -77,8 +74,9 @@ void AppendReferencedFilesFromHttpBody(
const std::vector<ExplodedHttpBodyElement>& elements,
std::vector<base::NullableString16>* referenced_files) {
for (size_t i = 0; i < elements.size(); ++i) {
if (elements[i].type == blink::WebHTTPBody::Element::TypeFile)
referenced_files->push_back(elements[i].file_path);
if (elements[i].type() == ExplodedHttpBodyElement::TYPE_FILE)
referenced_files->push_back(
base::NullableString16(elements[i].path().AsUTF16Unsafe(), false));
}
}

Expand Down Expand Up @@ -407,24 +405,35 @@ void WriteHttpBody(const ExplodedHttpBody& http_body, SerializeObject* obj) {
WriteAndValidateVectorSize(http_body.elements, obj);
for (size_t i = 0; i < http_body.elements.size(); ++i) {
const ExplodedHttpBodyElement& element = http_body.elements[i];
WriteInteger(element.type, obj);
if (element.type == blink::WebHTTPBody::Element::TypeData) {
WriteData(element.data.data(), static_cast<int>(element.data.size()),
obj);
} else if (element.type == blink::WebHTTPBody::Element::TypeFile) {
WriteString(element.file_path, obj);
WriteInteger64(element.file_start, obj);
WriteInteger64(element.file_length, obj);
WriteReal(element.file_modification_time, obj);
} else if (element.type ==
blink::WebHTTPBody::Element::TypeFileSystemURL) {
WriteGURL(element.filesystem_url, obj);
WriteInteger64(element.file_start, obj);
WriteInteger64(element.file_length, obj);
WriteReal(element.file_modification_time, obj);
} else {
DCHECK(element.type == blink::WebHTTPBody::Element::TypeBlob);
WriteStdString(element.blob_uuid, obj);
switch (element.type()) {
case ExplodedHttpBodyElement::TYPE_BYTES:
WriteInteger(blink::WebHTTPBody::Element::TypeData, obj);
WriteData(element.bytes(), static_cast<int>(element.length()), obj);
break;
case ExplodedHttpBodyElement::TYPE_FILE:
WriteInteger(blink::WebHTTPBody::Element::TypeFile, obj);
WriteString(
base::NullableString16(element.path().AsUTF16Unsafe(), false), obj);
WriteInteger64(static_cast<int64_t>(element.offset()), obj);
WriteInteger64(static_cast<int64_t>(element.length()), obj);
WriteReal(element.expected_modification_time().ToDoubleT(), obj);
break;
case ExplodedHttpBodyElement::TYPE_FILE_FILESYSTEM:
WriteInteger(blink::WebHTTPBody::Element::TypeFileSystemURL, obj);
WriteGURL(element.filesystem_url(), obj);
WriteInteger64(static_cast<int64_t>(element.offset()), obj);
WriteInteger64(static_cast<int64_t>(element.length()), obj);
WriteReal(element.expected_modification_time().ToDoubleT(), obj);
break;
case ExplodedHttpBodyElement::TYPE_BLOB:
WriteInteger(blink::WebHTTPBody::Element::TypeBlob, obj);
WriteStdString(element.blob_uuid(), obj);
break;
case ExplodedHttpBodyElement::TYPE_BYTES_DESCRIPTION:
case ExplodedHttpBodyElement::TYPE_DISK_CACHE_ENTRY:
default:
NOTREACHED();
continue;
}
}
WriteInteger64(http_body.identifier, obj);
Expand Down Expand Up @@ -667,19 +676,6 @@ void ReadPageState(SerializeObject* obj, ExplodedPageState* state) {

} // namespace

ExplodedHttpBodyElement::ExplodedHttpBodyElement()
: type(blink::WebHTTPBody::Element::TypeData),
file_start(0),
file_length(-1),
file_modification_time(std::numeric_limits<double>::quiet_NaN()) {
}

ExplodedHttpBodyElement::ExplodedHttpBodyElement(
const ExplodedHttpBodyElement& other) = default;

ExplodedHttpBodyElement::~ExplodedHttpBodyElement() {
}

ExplodedHttpBody::ExplodedHttpBody()
: identifier(0),
contains_passwords(false),
Expand Down Expand Up @@ -758,7 +754,7 @@ bool GeneratePostData(const ExplodedHttpBody& exploded,

http_body->set_identifier(exploded.identifier);
for (auto element : exploded.elements)
http_body->AppendExplodedHTTPBodyElement(element);
http_body->elements_mutable()->push_back(element);

return true;
}
Expand Down
18 changes: 2 additions & 16 deletions content/common/page_state_serialization.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/strings/nullable_string16.h"
#include "build/build_config.h"
#include "content/common/content_export.h"
#include "content/common/resource_request_body.h"
#include "third_party/WebKit/public/platform/WebHTTPBody.h"
#include "third_party/WebKit/public/platform/WebHistoryScrollRestorationType.h"
#include "third_party/WebKit/public/platform/WebReferrerPolicy.h"
Expand All @@ -21,22 +22,7 @@

namespace content {

class ResourceRequestBody;

struct CONTENT_EXPORT ExplodedHttpBodyElement {
blink::WebHTTPBody::Element::Type type;
std::string data;
base::NullableString16 file_path;
GURL filesystem_url;
int64_t file_start;
int64_t file_length;
double file_modification_time;
std::string blob_uuid;

ExplodedHttpBodyElement();
ExplodedHttpBodyElement(const ExplodedHttpBodyElement& other);
~ExplodedHttpBodyElement();
};
using ExplodedHttpBodyElement = ResourceRequestBody::Element;

struct CONTENT_EXPORT ExplodedHttpBody {
base::NullableString16 http_content_type;
Expand Down
46 changes: 22 additions & 24 deletions content/common/page_state_serialization_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,18 @@ void ExpectEquality(const std::vector<T>& a, const std::vector<T>& b) {
template <>
void ExpectEquality(const ExplodedHttpBodyElement& a,
const ExplodedHttpBodyElement& b) {
EXPECT_EQ(a.type, b.type);
EXPECT_EQ(a.data, b.data);
EXPECT_EQ(a.file_path, b.file_path);
EXPECT_EQ(a.filesystem_url, b.filesystem_url);
EXPECT_EQ(a.file_start, b.file_start);
EXPECT_EQ(a.file_length, b.file_length);
if (!(std::isnan(a.file_modification_time) &&
std::isnan(b.file_modification_time))) {
EXPECT_DOUBLE_EQ(a.file_modification_time, b.file_modification_time);
EXPECT_EQ(a.type(), b.type());
if (a.type() == ExplodedHttpBodyElement::TYPE_BYTES &&
b.type() == ExplodedHttpBodyElement::TYPE_BYTES) {
EXPECT_EQ(std::string(a.bytes(), a.length()),
std::string(b.bytes(), b.length()));
}
EXPECT_EQ(a.blob_uuid, b.blob_uuid);
EXPECT_EQ(a.path(), b.path());
EXPECT_EQ(a.filesystem_url(), b.filesystem_url());
EXPECT_EQ(a.offset(), b.offset());
EXPECT_EQ(a.length(), b.length());
EXPECT_EQ(a.expected_modification_time(), b.expected_modification_time());
EXPECT_EQ(a.blob_uuid(), b.blob_uuid());
}

template <>
Expand Down Expand Up @@ -120,19 +121,17 @@ class PageStateSerializationTest : public testing::Test {
http_body->http_content_type = NS16("text/foo");

ExplodedHttpBodyElement e1;
e1.type = blink::WebHTTPBody::Element::TypeData;
e1.data = "foo";
std::string test_body("foo");
e1.SetToBytes(test_body.data(), test_body.size());
http_body->elements.push_back(e1);

ExplodedHttpBodyElement e2;
e2.type = blink::WebHTTPBody::Element::TypeFile;
e2.file_path = NS16("file.txt");
e2.file_start = 100;
e2.file_length = 1024;
e2.file_modification_time = 9999.0;
e2.SetToFilePathRange(base::FilePath::FromUTF8Unsafe("file.txt"), 100, 1024,
base::Time::FromDoubleT(9999.0));
http_body->elements.push_back(e2);

referenced_files->push_back(e2.file_path);
referenced_files->push_back(
base::NullableString16(e2.path().AsUTF16Unsafe(), false));
}

void PopulateFrameStateForBackwardsCompatTest(
Expand Down Expand Up @@ -167,18 +166,17 @@ class PageStateSerializationTest : public testing::Test {
frame_state->http_body.is_null = false;

ExplodedHttpBodyElement e1;
e1.type = blink::WebHTTPBody::Element::TypeData;
e1.data = "first data block";
std::string test_body("first data block");
e1.SetToBytes(test_body.data(), test_body.size());
frame_state->http_body.elements.push_back(e1);

ExplodedHttpBodyElement e2;
e2.type = blink::WebHTTPBody::Element::TypeFile;
e2.file_path = NS16("file.txt");
e2.SetToFilePath(base::FilePath::FromUTF8Unsafe("file.txt"));
frame_state->http_body.elements.push_back(e2);

ExplodedHttpBodyElement e3;
e3.type = blink::WebHTTPBody::Element::TypeData;
e3.data = "data the second";
std::string test_body2("data the second");
e3.SetToBytes(test_body2.data(), test_body2.size());
frame_state->http_body.elements.push_back(e3);

ExplodedFrameState child_state;
Expand Down
44 changes: 0 additions & 44 deletions content/common/resource_request_body.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,50 +16,6 @@ ResourceRequestBody::ResourceRequestBody()
: identifier_(0) {
}

void ResourceRequestBody::AppendExplodedHTTPBodyElement(
const ExplodedHttpBodyElement& element) {
// Note: this code is based on GetRequestBodyForWebURLRequest (in
// web_url_request_util.cc). The other function transforms a
// blink::WebHTTPBody into a ResourceRequestBody. This function is used to
// transform an ExplodedHttpBody into a ResourceRequestBody.
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()));
}
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());
} 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));
}
break;
case WebHTTPBody::Element::TypeFileSystemURL: {
GURL file_system_url = element.filesystem_url;
DCHECK(file_system_url.SchemeIsFileSystem());
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());
Expand Down
4 changes: 0 additions & 4 deletions content/common/resource_request_body.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class FilePath;

namespace content {

struct ExplodedHttpBodyElement;

// A struct used to represent upload data. The data field is populated by
// WebURLLoader from the data given as WebHTTPBody.
class CONTENT_EXPORT ResourceRequestBody
Expand All @@ -34,8 +32,6 @@ class CONTENT_EXPORT ResourceRequestBody

ResourceRequestBody();

void AppendExplodedHTTPBodyElement(const ExplodedHttpBodyElement& element);

void AppendBytes(const char* bytes, int bytes_len);
void AppendFileRange(const base::FilePath& file_path,
uint64_t offset,
Expand Down
2 changes: 2 additions & 0 deletions content/content_renderer.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,8 @@
'renderer/history_entry.h',
'renderer/history_serialization.cc',
'renderer/history_serialization.h',
'renderer/http_body_conversions.cc',
'renderer/http_body_conversions.h',
'renderer/idle_user_detector.cc',
'renderer/idle_user_detector.h',
'renderer/image_downloader/image_downloader_impl.cc',
Expand Down
13 changes: 6 additions & 7 deletions content/public/common/page_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <stddef.h>

#include "base/files/file_path.h"
#include "base/strings/nullable_string16.h"
#include "base/strings/utf_string_conversions.h"
#include "content/common/page_state_serialization.h"

Expand Down Expand Up @@ -87,17 +88,15 @@ PageState PageState::CreateForTesting(
state.top.http_body.is_null = false;
if (optional_body_data) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeData;
element.data = optional_body_data;
std::string body_data(optional_body_data);
element.SetToBytes(body_data.data(), body_data.size());
state.top.http_body.elements.push_back(element);
}
if (optional_body_file_path) {
ExplodedHttpBodyElement element;
element.type = blink::WebHTTPBody::Element::TypeFile;
element.file_path =
ToNullableString16(optional_body_file_path->AsUTF8Unsafe());
state.top.http_body.elements.push_back(element);
state.referenced_files.push_back(element.file_path);
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;
Expand Down
Loading

0 comments on commit 2d0c9ae

Please sign in to comment.