Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

Committed: https://crrev.com/d04640fa1384d4610828f7e40be9f60ae30ff033
Cr-Commit-Position: refs/heads/master@{#395915}

[Łukasz Anforowicz, 2016-05-20 22:15:32]

Description was changed from

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and
made more generic, so that it can be used to carry any POST data
(nost just "browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

to

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

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

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

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

Charlie, could you please take a look?

This refactoring has been suggested in
https://codereview.chromium.org/1956383003/diff/160001/content/common/navigat...

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

Description was changed from

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

to

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

[clamy, 2016-05-23 16:59:27]

clamy@chromium.org changed reviewers:
+ clamy@chromium.org

[clamy, 2016-05-23 16:59:29]

Thanks! It looks nicer with the unification. A few questions below.

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:89: request_body =
entry.ConstructBodyFromBrowserInitiatedPostData();
Won't this happen in ConstructCommonNavigationParams if the request_body passed
is null?

https://codereview.chromium.org/1999943002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:2255: if
(ResourceRequestBody* body = info.common_params.post_data.get()) {
nit: I think in the codebase we tend to instantiate a variable first and then
check if it's null.

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5497: AddHTTPBodyToRequest(&request,
common_params.post_data);
What happens in the reload/history navigation case, if the WebUrlRequest
generated from the history object already has post data?

[Charlie Reis, 2016-05-23 18:14:10]

Nice!  Looks good, but I'm curious about Camille's question in NavigateInternal.

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:576: if (GetHasPostData()) {
This check wasn't included before, but it seems reasonable to me.  Hopefully no
one was forgetting to set the method before when using browser-initiated POST
data.

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:89: request_body =
entry.ConstructBodyFromBrowserInitiatedPostData();
On 2016/05/23 16:59:28, clamy wrote:
> Won't this happen in ConstructCommonNavigationParams if the request_body
passed
> is null?

Yeah, seems unnecessary.

[Łukasz Anforowicz, 2016-05-23 18:38:47]

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:576: if (GetHasPostData()) {
On 2016/05/23 18:14:10, Charlie Reis (slow to reply) wrote:
> This check wasn't included before, but it seems reasonable to me.  Hopefully
no
> one was forgetting to set the method before when using browser-initiated POST
> data.

Acknowledged.

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_request.cc:89: request_body =
entry.ConstructBodyFromBrowserInitiatedPostData();
On 2016/05/23 16:59:28, clamy wrote:
> Won't this happen in ConstructCommonNavigationParams if the request_body
passed
> is null?

Good point.  I've removed these 2 lines.

https://codereview.chromium.org/1999943002/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/browser/loader/reso...
content/browser/loader/resource_dispatcher_host_impl.cc:2255: if
(ResourceRequestBody* body = info.common_params.post_data.get()) {
On 2016/05/23 16:59:28, clamy wrote:
> nit: I think in the codebase we tend to instantiate a variable first and then
> check if it's null.

Done.

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5497: AddHTTPBodyToRequest(&request,
common_params.post_data);
On 2016/05/23 16:59:28, clamy wrote:
> What happens in the reload/history navigation case, if the WebUrlRequest
> generated from the history object already has post data?

You're right - we should only add common_params.post_data to the WebUrlRequest
if we are creating WebUrlRequest from scratch / not when reusing one in case of
a reload or a history navigation.  This incorrect code move was based on my
misunderstanding of the question in the other CR [1] - I've incorrectly thought
the question was a suggestion to add the post body in all the cases (i.e. also
in refresh and history case).  Thanks for pointing out that WebUrlRequest for a
reload or history case would already have post body taken care of.

I've moved the code back under the "else" branch.  Does this look good?  Or is
there something we need to do in case of a reload or a history navigation?

[1]
https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...

[Charlie Reis, 2016-05-23 19:07:26]

LGTM

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5497: AddHTTPBodyToRequest(&request,
common_params.post_data);
On 2016/05/23 18:38:47, Łukasz Anforowicz wrote:
> On 2016/05/23 16:59:28, clamy wrote:
> > What happens in the reload/history navigation case, if the WebUrlRequest
> > generated from the history object already has post data?
> 
> You're right - we should only add common_params.post_data to the WebUrlRequest
> if we are creating WebUrlRequest from scratch / not when reusing one in case
of
> a reload or a history navigation.  This incorrect code move was based on my
> misunderstanding of the question in the other CR [1] - I've incorrectly
thought
> the question was a suggestion to add the post body in all the cases (i.e. also
> in refresh and history case).  Thanks for pointing out that WebUrlRequest for
a
> reload or history case would already have post body taken care of.
> 
> I've moved the code back under the "else" branch.  Does this look good?  Or is
> there something we need to do in case of a reload or a history navigation?
> 
> [1]
>
https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...

Thanks, looks right to me.

[clamy, 2016-05-24 11:46:35]

Thanks! Lgtm.

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1999943002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5497: AddHTTPBodyToRequest(&request,
common_params.post_data);
On 2016/05/23 19:07:26, Charlie Reis (slow to reply) wrote:
> On 2016/05/23 18:38:47, Łukasz Anforowicz wrote:
> > On 2016/05/23 16:59:28, clamy wrote:
> > > What happens in the reload/history navigation case, if the WebUrlRequest
> > > generated from the history object already has post data?
> > 
> > You're right - we should only add common_params.post_data to the
WebUrlRequest
> > if we are creating WebUrlRequest from scratch / not when reusing one in case
> of
> > a reload or a history navigation.  This incorrect code move was based on my
> > misunderstanding of the question in the other CR [1] - I've incorrectly
> thought
> > the question was a suggestion to add the post body in all the cases (i.e.
also
> > in refresh and history case).  Thanks for pointing out that WebUrlRequest
for
> a
> > reload or history case would already have post body taken care of.
> > 
> > I've moved the code back under the "else" branch.  Does this look good?  Or
is
> > there something we need to do in case of a reload or a history navigation?
> > 
> > [1]
> >
>
https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
> 
> Thanks, looks right to me.

Thanks lgtm too. My comment in the other CL was really a question about what
should be done in the history/reload case, not a suggestion :).

[Łukasz Anforowicz, 2016-05-24 13:54:08]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-05-24 13:54:24]

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

[commit-bot: I haz the power, 2016-05-24 14:00:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 14:00:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Łukasz Anforowicz, 2016-05-24 15:41:44]

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

[Łukasz Anforowicz, 2016-05-24 15:41:45]

+kenrb@ for security review of //content/common/frame_messages.h

Summary of changes in frame_messages.h:

- before the CL: post body present in 3 places (either as ResourceRequestBody or
as a vector of bytes)

- after the CL: post_body removed from top-level payload of IPC messages (it has
been moved into CommonNavigationParams struct [from StartNavigationParams
struct])

[kenrb, 2016-05-25 14:51:52]

ipc lgtm

[Łukasz Anforowicz, 2016-05-25 15:03:45]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-05-25 15:03:57]

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

[commit-bot: I haz the power, 2016-05-25 17:08:55]

Description was changed from

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

to

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

[commit-bot: I haz the power, 2016-05-25 17:08:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-25 17:11:16]

Description was changed from

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

to

==========
Moving HTTP POST body from StartNavigationParams to CommonNavigationParams.

StartNavigationParams used to hold
    std::vector<unsigned char> browser_initiated_post_data
field.  This CL moves the field to CommonNavigationParams and makes it
more generic, so that it can be used to carry any POST data (nost just
"browser initiated" post data):
    scoped_refptr<ResourceRequestBody> post_data;

Making the type more generic (i.e. replacing a vector of bytes with
ResourceRequestBody) is needed to prepare for forwarding POST data back to the
renderer during process transfers.  This fix will come in crrev.com/1956383003.

Moving the type from StartNavigationParams to CommonNavigationParams helps
unify many scenarios across default and PlzNavigate modes and consequently
allows avoiding passing of the body as a separate field and/or param in
quite a few places.

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

Committed: https://crrev.com/d04640fa1384d4610828f7e40be9f60ae30ff033
Cr-Commit-Position: refs/heads/master@{#395915}
==========

[commit-bot: I haz the power, 2016-05-25 17:11:17]

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