Relanding: Deduplicating code performing WebHTTPBody::Element conversions.

Relanding: 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, 614299
Committed: https://crrev.com/28b27bcc839a67e8a44a18536cd587fd0746ee40
Cr-Commit-Position: refs/heads/master@{#396043}

[Łukasz Anforowicz, 2016-05-25 00:25:52]

Description was changed from

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

==========
Relanding: 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, 614299
==========

[Łukasz Anforowicz, 2016-05-25 00:26:17]

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

[Łukasz Anforowicz, 2016-05-25 00:26:18]

Charlie, could you please take a look?  This is a reland, so you probably only
want to take a look at the diff between patchset 1 (the original, reverted CL)
and patchset 2.

https://codereview.chromium.org/2006053004/diff/20001/content/renderer/http_b...
File content/renderer/http_body_conversions.cc (right):

https://codereview.chromium.org/2006053004/diff/20001/content/renderer/http_b...
content/renderer/http_body_conversions.cc:51: // TODO(lukasza): Dedupe wrt
GetRequestBodyForWebURLRequest.
More deduplication coming soon - I didn't realize that
//content/child/web_url_request_util.cc also has very similar code.

https://codereview.chromium.org/2006053004/diff/20001/storage/common/data_ele...
File storage/common/data_element.h (right):

https://codereview.chromium.org/2006053004/diff/20001/storage/common/data_ele...
storage/common/data_element.h:45: const char* bytes() const { return bytes_ ?
bytes_ : buf_.data(); }
There are places in code where users of DataElement would check that type() ==
TYPE_BYTES and then assume that they can call something like
std::string(elem.bytes(), elem.length()).  This seems like a valid assumption -
similar to how it is okay to access STL's container's begin() or data() even if
the container is empty (it is okay as long as one doesn't dereference the
result).

The old code would crash when |buf_| was empty.  The crash would only happen in
debug builds - the STL in Goobuntu would say that dereferencing element 0 is
wrong when there are zero elements.

I assume that the old code was written before C++11 where std::vector::data()
accessor was introduced.  The new accessors doesn't crash for empty containers
(even on debug builds).

The above hopefully convinced you that the change above is really the right fix,
but you might still be asking yourself why we didn't hit this problem before
this CL.  The answer is that before this CL ExplodedHttpBodyElement would hold
bytes in a std::string, rather than in storage::DataElement (as it does after
the deduplication from this CL).

[Charlie Reis, 2016-05-25 19:01:06]

Works for me.  Unfortunate that we don't have any debug build try jobs to catch
this sort of thing by default.  (We run release builds with DCHECKs enabled, and
it sounds like that doesn't catch it.)  LGTM.

[Łukasz Anforowicz, 2016-05-25 21:00:25]

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

[Łukasz Anforowicz, 2016-05-25 21:00:26]

Dana, could you please take a look at the changes in
storage/common/data_element.h?  I tried to explain the rationale for the changes
in
https://codereview.chromium.org/2006053004/diff/20001/storage/common/data_ele...

https://codereview.chromium.org/2006053004/diff/20001/content/renderer/http_b...
File content/renderer/http_body_conversions.cc (right):

https://codereview.chromium.org/2006053004/diff/20001/content/renderer/http_b...
content/renderer/http_body_conversions.cc:51: // TODO(lukasza): Dedupe wrt
GetRequestBodyForWebURLRequest.
On 2016/05/25 00:26:18, Łukasz Anforowicz wrote:
> More deduplication coming soon - I didn't realize that
> //content/child/web_url_request_util.cc also has very similar code.

Just as an FYI - I am working on this in
https://codereview.chromium.org/2012913002/

[danakj, 2016-05-25 22:13:46]

https://codereview.chromium.org/2006053004/diff/20001/storage/common/data_ele...
File storage/common/data_element.h (right):

https://codereview.chromium.org/2006053004/diff/20001/storage/common/data_ele...
storage/common/data_element.h:45: const char* bytes() const { return bytes_ ?
bytes_ : buf_.data(); }
On 2016/05/25 00:26:18, Łukasz Anforowicz wrote:
> There are places in code where users of DataElement would check that type() ==
> TYPE_BYTES and then assume that they can call something like
> std::string(elem.bytes(), elem.length()).  This seems like a valid assumption
-
> similar to how it is okay to access STL's container's begin() or data() even
if
> the container is empty (it is okay as long as one doesn't dereference the
> result).
> 
> The old code would crash when |buf_| was empty.  The crash would only happen
in
> debug builds - the STL in Goobuntu would say that dereferencing element 0 is
> wrong when there are zero elements.
> 
> I assume that the old code was written before C++11 where std::vector::data()
> accessor was introduced.  The new accessors doesn't crash for empty containers
> (even on debug builds).
> 
> The above hopefully convinced you that the change above is really the right
fix,
> but you might still be asking yourself why we didn't hit this problem before
> this CL.  The answer is that before this CL ExplodedHttpBodyElement would hold
> bytes in a std::string, rather than in storage::DataElement (as it does after
> the deduplication from this CL).

Returning null lgtm

[Łukasz Anforowicz, 2016-05-25 22:19:34]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-05-25 22:19:34]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2006053004/#ps40001
(title: "Rebasing...")

[commit-bot: I haz the power, 2016-05-25 22:20:34]

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

[commit-bot: I haz the power, 2016-05-25 23:45:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-25 23:47:13]

Description was changed from

==========
Relanding: 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, 614299
==========

to

==========
Relanding: 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, 614299

Committed: https://crrev.com/28b27bcc839a67e8a44a18536cd587fd0746ee40
Cr-Commit-Position: refs/heads/master@{#396043}
==========

[commit-bot: I haz the power, 2016-05-25 23:47:13]

Patchset 3 (id:??) landed as
https://crrev.com/28b27bcc839a67e8a44a18536cd587fd0746ee40
Cr-Commit-Position: refs/heads/master@{#396043}