[Fetch API] Send request body as a FormData.

[Fetch API] Send request body as a FormData.

Previously fetch() sent request's body data as a blob. That made the data
invisible from the devtools. Now it sends the body data as a form data to
fix the issue.

BUG=457484
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200725

[yhirano, 2015-08-17 06:49:20]

yhirano@chromium.org changed reviewers:
+ caseq@chromium.org, hiroshige@chromium.org

[yhirano, 2015-08-17 06:53:28]

On 2015/08/17 06:49:20, yhirano wrote:

This CL depends on https://codereview.chromium.org/1265413002/.

[hiroshige, 2015-08-17 08:46:02]

lgtm for modules/fetch part with a nit.

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Request.h
File Source/modules/fetch/Request.h (left):

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.h:71: static Request*
createRequestWithRequestOrString(ScriptState*, Request*, const String&, const
RequestInit&, ExceptionState&);
nit optional: I think removing |const| here is to allow
RequestInit::body.release(), but passing non-const reference looks like it is
variable for output.
Using PassOwnPtr<RequestInit> might be clearer.
But it's optional because this is an internal function...

another nit optional: we can remove declaration of 
createRequestWithRequestOrString() here and move
createCopyOfFetchRequestDataForFetch() and createRequestWithRequestOrString()
into an anonymous namespace in Request.cpp.

[caseq, 2015-08-17 17:24:38]

devtools tests lgtm

https://codereview.chromium.org/1301523002/diff/1/LayoutTests/http/tests/insp...
File LayoutTests/http/tests/inspector/network/network-fetch-post-payload.html
(right):

https://codereview.chromium.org/1301523002/diff/1/LayoutTests/http/tests/insp...
LayoutTests/http/tests/inspector/network/network-fetch-post-payload.html:11:
InspectorTest.makeFetch("resources/resource.php?foo", {method: 'POST', body:
payload}, step2);
double quotes please!

[yhirano, 2015-08-18 09:12:39]

https://codereview.chromium.org/1301523002/diff/1/LayoutTests/http/tests/insp...
File LayoutTests/http/tests/inspector/network/network-fetch-post-payload.html
(right):

https://codereview.chromium.org/1301523002/diff/1/LayoutTests/http/tests/insp...
LayoutTests/http/tests/inspector/network/network-fetch-post-payload.html:11:
InspectorTest.makeFetch("resources/resource.php?foo", {method: 'POST', body:
payload}, step2);
On 2015/08/17 17:24:38, caseq wrote:
> double quotes please!

Done.

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Request.h
File Source/modules/fetch/Request.h (left):

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.h:71: static Request*
createRequestWithRequestOrString(ScriptState*, Request*, const String&, const
RequestInit&, ExceptionState&);
On 2015/08/17 08:46:02, hiroshige (ooo zombie) wrote:
> nit optional: I think removing |const| here is to allow
> RequestInit::body.release(), but passing non-const reference looks like it is
> variable for output.
> Using PassOwnPtr<RequestInit> might be clearer.
> But it's optional because this is an internal function...
> 
RequestInit is marked as STACK_ALLOCATED due to an oilpan reason and I would
like to keep it.

> another nit optional: we can remove declaration of 
> createRequestWithRequestOrString() here and move
> createCopyOfFetchRequestDataForFetch() and createRequestWithRequestOrString()
> into an anonymous namespace in Request.cpp.
> 

The function uses many private methods / members, so I would keep the
declaration.

[hiroshige, 2015-08-18 09:36:53]

lgtm.

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Request.h
File Source/modules/fetch/Request.h (left):

https://codereview.chromium.org/1301523002/diff/1/Source/modules/fetch/Reques...
Source/modules/fetch/Request.h:71: static Request*
createRequestWithRequestOrString(ScriptState*, Request*, const String&, const
RequestInit&, ExceptionState&);
On 2015/08/18 09:12:39, yhirano wrote:
> On 2015/08/17 08:46:02, hiroshige (ooo zombie) wrote:
> > nit optional: I think removing |const| here is to allow
> > RequestInit::body.release(), but passing non-const reference looks like it
is
> > variable for output.
> > Using PassOwnPtr<RequestInit> might be clearer.
> > But it's optional because this is an internal function...
> > 
> RequestInit is marked as STACK_ALLOCATED due to an oilpan reason and I would
> like to keep it.
> 
> > another nit optional: we can remove declaration of 
> > createRequestWithRequestOrString() here and move
> > createCopyOfFetchRequestDataForFetch() and
createRequestWithRequestOrString()
> > into an anonymous namespace in Request.cpp.
> > 
> 
> The function uses many private methods / members, so I would keep the
> declaration.

Acknowledged.

[yhirano, 2015-08-18 10:18:54]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-08-18 10:18:55]

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

[commit-bot: I haz the power, 2015-08-18 10:19:06]

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

[commit-bot: I haz the power, 2015-08-18 11:30:50]

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

[commit-bot: I haz the power, 2015-08-18 11:30:51]

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

[yhirano, 2015-08-18 11:55:16]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2015-08-18 11:55:27]

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

[commit-bot: I haz the power, 2015-08-18 12:35:09]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200725

[brucedawson, 2015-08-19 16:45:28]

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

[brucedawson, 2015-08-19 16:45:28]

https://codereview.chromium.org/1301523002/diff/20001/Source/modules/fetch/Re...
File Source/modules/fetch/RequestInit.cpp (right):

https://codereview.chromium.org/1301523002/diff/20001/Source/modules/fetch/Re...
Source/modules/fetch/RequestInit.cpp:41: RefPtr<FormData> formData =
FormData::create();
This variable declaration appears to have no value. It is shadowed by another
declaration of the same name and type on line 52 such that this variable is
never actually referenced.

It's not a 'bug', but it could be confusing to human readers.

This was found as a new /analyze variable shadowing warning.

[yhirano, 2015-08-20 02:16:49]

https://codereview.chromium.org/1301523002/diff/20001/Source/modules/fetch/Re...
File Source/modules/fetch/RequestInit.cpp (right):

https://codereview.chromium.org/1301523002/diff/20001/Source/modules/fetch/Re...
Source/modules/fetch/RequestInit.cpp:41: RefPtr<FormData> formData =
FormData::create();
On 2015/08/19 16:45:28, brucedawson wrote:
> This variable declaration appears to have no value. It is shadowed by another
> declaration of the same name and type on line 52 such that this variable is
> never actually referenced.
> 
> It's not a 'bug', but it could be confusing to human readers.
> 
> This was found as a new /analyze variable shadowing warning.

Thank you! I will fix it.