Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

It turns out that https://crrev.com/1987053002 has still left some
remaining / unaddressed code deduplication opportunities - they are
taken care of by the current CL.

The CL replaces the following old fields of ExplodedHttpBody:
- std::vector<ExplodedHttpBodyElement> elements
- int64_t identifier
- bool is_null
with a single field:
- scoped_ptr<ResourceRequestBody> request_body
  - the old is_null field is effectively replaced by scoped_ptr
  - the old identifier is covered by ResourceRequestBody::identifier_
Note that ExplodedHttpBody still contains extra data on top of
ResourceRequestBody, like |bool contains_password| and
|base::NullableString16| http_content_type (and because of this the
whole ExplodedHttpBody struct cannot at this point be replaced with
ResourceRequestBody).

This CL deletes
    void AppendHttpBodyElement(const ResourceRequestBodyElement&, WebHTTPBody)
function from //content/renderer/http_body_conversions.h and tweaks
callsites to instead use
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebHTTPBody(
        const blink::WebHTTPBody& httpBody)
which has been extracted from the already existing:
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebURLRequest(
        const blink::WebURLRequest& request)
(both in content/child/web_url_request_util.h).

This CL moves
    void ConvertToHttpBodyElement(const WebHTTPBody::Element& input,
                                  ResourceRequestBody::Element* output)
function from //content/renderer/http_body_conversions.h into
content/child/web_url_request_util.h and makes this function more generic
(i.e. covering the whole ResourceRequestBody rather than working with
individual elements):
    blink::WebHTTPBody GetWebHTTPBodyForRequestBody
        const scoped_refptr<ResourceRequestBody>& input);

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9a9ece4a455f8d8849727ceac3e4365d61d11974
Cr-Commit-Position: refs/heads/master@{#396882}

[Łukasz Anforowicz, 2016-05-25 20:55:09]

Description was changed from

==========
Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

It turns out that https://crrev.com/1987053002 has still left some
remaining / unaddressed code deduplication opportunities - they are
taken care of by the current CL.

The CL replaces the following old fields of ExplodedHttpBody:
- std::vector<ExplodedHttpBodyElement> elements
- int64_t identifier
- bool is_null
with a single field:
- scoped_ptr<ResourceRequestBody> request_body
  - the old is_null field is effectively replaced by scoped_ptr
  - the old identifier is covered by ResourceRequestBody::identifier_
Note that ExplodedHttpBody still contains extra data on top of
ResourceRequestBody, like |bool contains_password| and
|base::NullableString16| http_content_type (and because of this the
whole ExplodedHttpBody struct cannot at this point be replaced with
ResourceRequestBody).

This CL deletes
    void AppendHttpBodyElement(const ResourceRequestBodyElement&, WebHTTPBody)
function from //content/renderer/http_body_conversions.h and tweaks
callsites to instead use
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebHTTPBody(
        const blink::WebHTTPBody& httpBody)
which has been extracted from the already existing:
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebURLRequest(
        const blink::WebURLRequest& request)
(both in content/child/web_url_request_util.h).

This CL moves
    void ConvertToHttpBodyElement(const WebHTTPBody::Element& input,
                                  ResourceRequestBody::Element* output)
function from //content/renderer/http_body_conversions.h into
content/child/web_url_request_util.h and makes this function more generic
(i.e. covering the whole ResourceRequestBody rather than working with
individual elements):
    blink::WebHTTPBody GetWebHTTPBodyForRequestBody
        const scoped_refptr<ResourceRequestBody>& input);

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

to

==========
Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

It turns out that https://crrev.com/1987053002 has still left some
remaining / unaddressed code deduplication opportunities - they are
taken care of by the current CL.

The CL replaces the following old fields of ExplodedHttpBody:
- std::vector<ExplodedHttpBodyElement> elements
- int64_t identifier
- bool is_null
with a single field:
- scoped_ptr<ResourceRequestBody> request_body
  - the old is_null field is effectively replaced by scoped_ptr
  - the old identifier is covered by ResourceRequestBody::identifier_
Note that ExplodedHttpBody still contains extra data on top of
ResourceRequestBody, like |bool contains_password| and
|base::NullableString16| http_content_type (and because of this the
whole ExplodedHttpBody struct cannot at this point be replaced with
ResourceRequestBody).

This CL deletes
    void AppendHttpBodyElement(const ResourceRequestBodyElement&, WebHTTPBody)
function from //content/renderer/http_body_conversions.h and tweaks
callsites to instead use
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebHTTPBody(
        const blink::WebHTTPBody& httpBody)
which has been extracted from the already existing:
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebURLRequest(
        const blink::WebURLRequest& request)
(both in content/child/web_url_request_util.h).

This CL moves
    void ConvertToHttpBodyElement(const WebHTTPBody::Element& input,
                                  ResourceRequestBody::Element* output)
function from //content/renderer/http_body_conversions.h into
content/child/web_url_request_util.h and makes this function more generic
(i.e. covering the whole ResourceRequestBody rather than working with
individual elements):
    blink::WebHTTPBody GetWebHTTPBodyForRequestBody
        const scoped_refptr<ResourceRequestBody>& input);

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

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

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

[Łukasz Anforowicz, 2016-05-26 16:26:53]

Charlie, can you take a look please?

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
File content/public/common/page_state.cc (left):

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
content/public/common/page_state.cc:95: if (optional_body_file_path) {
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]

Wow, this is even nicer.  Much appreciated!

LGTM with one request below.

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
File content/public/common/page_state.cc (left):

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
content/public/common/page_state.cc:95: if (optional_body_file_path) {
On 2016/05/26 16:26:52, Łukasz Anforowicz wrote:
> This branch is taken by some tests.  I assume that these tests only care about
> |state.referenced_files|.

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]

Thanks for reviewing.

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
File content/public/common/page_state.cc (left):

https://codereview.chromium.org/2012913002/diff/40001/content/public/common/p...
content/public/common/page_state.cc:95: if (optional_body_file_path) {
On 2016/05/27 21:45:53, Charlie Reis wrote:
> On 2016/05/26 16:26:52, Łukasz Anforowicz wrote:
> > This branch is taken by some tests.  I assume that these tests only care
about
> > |state.referenced_files|.
> 
> 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?

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

> (If it causes problems, I can
> revisit it later.)

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.

[Łukasz Anforowicz, 2016-05-31 18:03:02]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-05-31 18:03:02]

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/2012913002/#ps80001
(title: "Calling ResourceRequestBody::AppendFileRange with
optional_body_file_path.")

[commit-bot: I haz the power, 2016-05-31 18:03:37]

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

[commit-bot: I haz the power, 2016-05-31 19:01:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-31 19:04:34]

Description was changed from

==========
Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

It turns out that https://crrev.com/1987053002 has still left some
remaining / unaddressed code deduplication opportunities - they are
taken care of by the current CL.

The CL replaces the following old fields of ExplodedHttpBody:
- std::vector<ExplodedHttpBodyElement> elements
- int64_t identifier
- bool is_null
with a single field:
- scoped_ptr<ResourceRequestBody> request_body
  - the old is_null field is effectively replaced by scoped_ptr
  - the old identifier is covered by ResourceRequestBody::identifier_
Note that ExplodedHttpBody still contains extra data on top of
ResourceRequestBody, like |bool contains_password| and
|base::NullableString16| http_content_type (and because of this the
whole ExplodedHttpBody struct cannot at this point be replaced with
ResourceRequestBody).

This CL deletes
    void AppendHttpBodyElement(const ResourceRequestBodyElement&, WebHTTPBody)
function from //content/renderer/http_body_conversions.h and tweaks
callsites to instead use
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebHTTPBody(
        const blink::WebHTTPBody& httpBody)
which has been extracted from the already existing:
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebURLRequest(
        const blink::WebURLRequest& request)
(both in content/child/web_url_request_util.h).

This CL moves
    void ConvertToHttpBodyElement(const WebHTTPBody::Element& input,
                                  ResourceRequestBody::Element* output)
function from //content/renderer/http_body_conversions.h into
content/child/web_url_request_util.h and makes this function more generic
(i.e. covering the whole ResourceRequestBody rather than working with
individual elements):
    blink::WebHTTPBody GetWebHTTPBodyForRequestBody
        const scoped_refptr<ResourceRequestBody>& input);

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Deduping conversions between ResourceRequestBody/WebHTTPBody/ExplodedHttpBody.

It turns out that https://crrev.com/1987053002 has still left some
remaining / unaddressed code deduplication opportunities - they are
taken care of by the current CL.

The CL replaces the following old fields of ExplodedHttpBody:
- std::vector<ExplodedHttpBodyElement> elements
- int64_t identifier
- bool is_null
with a single field:
- scoped_ptr<ResourceRequestBody> request_body
  - the old is_null field is effectively replaced by scoped_ptr
  - the old identifier is covered by ResourceRequestBody::identifier_
Note that ExplodedHttpBody still contains extra data on top of
ResourceRequestBody, like |bool contains_password| and
|base::NullableString16| http_content_type (and because of this the
whole ExplodedHttpBody struct cannot at this point be replaced with
ResourceRequestBody).

This CL deletes
    void AppendHttpBodyElement(const ResourceRequestBodyElement&, WebHTTPBody)
function from //content/renderer/http_body_conversions.h and tweaks
callsites to instead use
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebHTTPBody(
        const blink::WebHTTPBody& httpBody)
which has been extracted from the already existing:
    scoped_refptr<ResourceRequestBody> GetRequestBodyForWebURLRequest(
        const blink::WebURLRequest& request)
(both in content/child/web_url_request_util.h).

This CL moves
    void ConvertToHttpBodyElement(const WebHTTPBody::Element& input,
                                  ResourceRequestBody::Element* output)
function from //content/renderer/http_body_conversions.h into
content/child/web_url_request_util.h and makes this function more generic
(i.e. covering the whole ResourceRequestBody rather than working with
individual elements):
    blink::WebHTTPBody GetWebHTTPBodyForRequestBody
        const scoped_refptr<ResourceRequestBody>& input);

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9a9ece4a455f8d8849727ceac3e4365d61d11974
Cr-Commit-Position: refs/heads/master@{#396882}
==========

[commit-bot: I haz the power, 2016-05-31 19:04:35]

Patchset 5 (id:??) landed as
https://crrev.com/9a9ece4a455f8d8849727ceac3e4365d61d11974
Cr-Commit-Position: refs/heads/master@{#396882}