Deduplicating code performing WebHTTPBody::Element conversions.

Deduplicating code performing WebHTTPBody::Element conversions.

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
Committed: https://crrev.com/2d0c9ae694afcb5c16028222e206b64aaba0cdba
Cr-Commit-Position: refs/heads/master@{#395461}

[Łukasz Anforowicz, 2016-05-18 21:31:12]

Description was changed from

==========
Getting rid of ExplodedHTTPBody as a separate type.

BUG=
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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 some of the duplicated code (the 2.a. case above)
by making ExplodedHttpBodyElement a type alias for
ResourceRequestBody::Element and removing code that dealt with the old
ExplodedHttpBodyElement struct.

Duplication in the 2.b. case above can be removed in a follow-up CL by
removing ResourceRequestBody::AppendExplodedHTTPBodyElement and
replacing it with appending elements directly (because now
ExplodedHttpBodyElement and ResourceRequestBody::Element are the same
type).

BUG=582211
==========

[Łukasz Anforowicz, 2016-05-18 21:31:53]

Description was changed from

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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 some of the duplicated code (the 2.a. case above)
by making ExplodedHttpBodyElement a type alias for
ResourceRequestBody::Element and removing code that dealt with the old
ExplodedHttpBodyElement struct.

Duplication in the 2.b. case above can be removed in a follow-up CL by
removing ResourceRequestBody::AppendExplodedHTTPBodyElement and
replacing it with appending elements directly (because now
ExplodedHttpBodyElement and ResourceRequestBody::Element are the same
type).

BUG=582211
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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 some of the duplicated code (the 2.a. case above)
by making ExplodedHttpBodyElement a type alias for
ResourceRequestBody::Element and removing code that dealt with the old
ExplodedHttpBodyElement struct.

Duplication in the 2.b. case above can be removed in a follow-up CL by
removing ResourceRequestBody::AppendExplodedHTTPBodyElement and
replacing it with appending elements directly (because now
ExplodedHttpBodyElement and ResourceRequestBody::Element are the same
type).

BUG=582211
==========

[Łukasz Anforowicz, 2016-05-20 22:11:18]

Description was changed from

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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 some of the duplicated code (the 2.a. case above)
by making ExplodedHttpBodyElement a type alias for
ResourceRequestBody::Element and removing code that dealt with the old
ExplodedHttpBodyElement struct.

Duplication in the 2.b. case above can be removed in a follow-up CL by
removing ResourceRequestBody::AppendExplodedHTTPBodyElement and
replacing it with appending elements directly (because now
ExplodedHttpBodyElement and ResourceRequestBody::Element are the same
type).

BUG=582211
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

[Łukasz Anforowicz, 2016-05-20 22:14:06]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2016-05-20 22:14:07]

Charlie, can you take a look please?

https://codereview.chromium.org/1987053002/diff/100001/content/renderer/histo...
File content/renderer/history_serialization.cc (left):

https://codereview.chromium.org/1987053002/diff/100001/content/renderer/histo...
content/renderer/history_serialization.cc:84: }
These 2 methods have been moved to the new, separate compilation unit -
content/renderer/http_body_conversions.cc

[Łukasz Anforowicz, 2016-05-20 23:24:46]

Description was changed from

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

[Charlie Reis, 2016-05-23 19:58:30]

Very happy to see the duplicated code removed-- thanks!  LGTM with one sanity
check about the type conversions.

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
content/common/page_state_serialization.cc:45:
base::FilePath::FromUTF16Unsafe(file_path.string()),
There's a lot of UTF16Unsafe calls in this CL.  Just want to sanity check that
we're not introducing new problems here-- would we have this these before as
well?  (At a quick glance, I see we do the same thing within
PageState::GetReferencedFiles, so hopefully it isn't an issue.)

Similar for the signed to unsigned int conversions below.

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
File content/common/page_state_serialization_unittest.cc (right):

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
content/common/page_state_serialization_unittest.cc:125:
e1.SetToBytes(data.data(), data.size());
nit: Maybe pick a different name than data, to avoid data.data()?

[Łukasz Anforowicz, 2016-05-23 22:13:34]

Thanks for reviewing.

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
File content/common/page_state_serialization.cc (right):

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
content/common/page_state_serialization.cc:45:
base::FilePath::FromUTF16Unsafe(file_path.string()),
On 2016/05/23 19:58:30, Charlie Reis (slow to reply) wrote:
> There's a lot of UTF16Unsafe calls in this CL.  Just want to sanity check that
> we're not introducing new problems here-- would we have this these before as
> well?  (At a quick glance, I see we do the same thing within
> PageState::GetReferencedFiles, so hopefully it isn't an issue.)
> 
We would have these UnsafeUTF16 calls before:

- Before this CL, the conversions were present in
content/common/resource_request_body.cc (inside the recently added
ResourceRequestBody::AppendExplodedHTTPBodyElement method).

- The conversions did not have to happen in
content/renderer/history_serialization.cc (because before this CL,
ExplodedHttpBodyElement used to declare field types in a way that was more
compatible with Blink types - i.e. |base::NullableString16 file_path| rather
than |base::FilePath|).

Also - AFAICT, these UTF conversion functions are safe as long as one can
guarantee that input/output truly is UTF16 or UTF8 (and not some other
encoding).  I believe that such guarantee is present here.

> Similar for the signed to unsigned int conversions below.

Before this CL, the conversions were present in
content/common/resource_request_body.cc (inside the recently added
ResourceRequestBody::AppendExplodedHTTPBodyElement method).  OTOH, the
conversions did not have to happen in content/renderer/history_serialization.cc
(because before this CL, ExplodedHttpBodyElement used to declare field types in
a way that was more compatible with Blink types - i.e. |int64_t| instead of
|uint64_t|).

At the same time - it seems worth to have a dedicated discussion for these
issues separately - I've opened https://crbug.com/614179 to for this.  If we
decide to do something here, it probably would be in a CL separate from this one
(since the current CL just preserves status quo).

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
File content/common/page_state_serialization_unittest.cc (right):

https://codereview.chromium.org/1987053002/diff/100001/content/common/page_st...
content/common/page_state_serialization_unittest.cc:125:
e1.SetToBytes(data.data(), data.size());
On 2016/05/23 19:58:30, Charlie Reis (slow to reply) wrote:
> nit: Maybe pick a different name than data, to avoid data.data()?

Done.

[Charlie Reis, 2016-05-23 22:17:01]

Thanks!  LGTM.

[Łukasz Anforowicz, 2016-05-23 22:21:31]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-05-23 22:21:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987053002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987053002/120001

[commit-bot: I haz the power, 2016-05-23 23:28:20]

Description was changed from

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

[commit-bot: I haz the power, 2016-05-23 23:28:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-23 23:34:56]

Description was changed from

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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
==========

to

==========
Deduplicating code performing WebHTTPBody::Element conversions.

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

Committed: https://crrev.com/2d0c9ae694afcb5c16028222e206b64aaba0cdba
Cr-Commit-Position: refs/heads/master@{#395461}
==========

[commit-bot: I haz the power, 2016-05-23 23:34:58]

Patchset 7 (id:??) landed as
https://crrev.com/2d0c9ae694afcb5c16028222e206b64aaba0cdba
Cr-Commit-Position: refs/heads/master@{#395461}

[kolos1, 2016-05-24 08:55:16]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2002413002/ by kolos@chromium.org.

The reason for reverting is: This CL causes failures in content_browsertests and
browser_tests
(https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...
) 

BUG=614299.