[ServiceWorker] Send the body of Request in fetch() API.

[ServiceWorker] Send the body of Request in fetch() API.


TEST=LayoutTests/http/tests/serviceworker/resources/request-worker.js
BUG=402387
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181436

[horo, 2014-09-03 10:44:54]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2014-09-03 10:44:54]

yhirano@
Could you please review?

[michaeln1, 2014-09-03 20:03:44]

drive by

https://codereview.chromium.org/533123002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/533123002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:273: if (m_request->method() !=
"GET" && m_request->method() != "HEAD") {
Would it be better to test positive for "POST" and "PUT" (so unexpected inputs
don't get a post body), or is there a reason you're testing negative?

Ok, i see XMLHttpRequest::areMethodAndURLValidForSend() tests negative like this
too, it also tests for m_url.protocolIsInHTTPFamily. Should this method test for
that as well?

[horo, 2014-09-04 05:21:01]

https://codereview.chromium.org/533123002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/533123002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:273: if (m_request->method() !=
"GET" && m_request->method() != "HEAD") {
On 2014/09/03 20:03:44, michaeln1 wrote:
> Would it be better to test positive for "POST" and "PUT" (so unexpected inputs
> don't get a post body), or is there a reason you're testing negative?
> 
> Ok, i see XMLHttpRequest::areMethodAndURLValidForSend() tests negative like
this
> too, it also tests for m_url.protocolIsInHTTPFamily. Should this method test
for
> that as well?

I want to do the same as XMLHttpRequest.
performHTTPFetch() is called only when m_request->url().protocolIsInHTTPFamily()
it true.
Added ASSERT() in the top of performHTTPFetch().

[yhirano, 2014-09-04 08:00:56]

https://codereview.chromium.org/533123002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/533123002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:277: if (m_request->method() !=
"GET" && m_request->method() != "HEAD") {
I could not find any statement corresponding to this predicate in the spec. Is
there?

[horo, 2014-09-04 11:55:15]

https://codereview.chromium.org/533123002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/533123002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchManager.cpp:277: if (m_request->method() !=
"GET" && m_request->method() != "HEAD") {
On 2014/09/04 08:00:55, yhirano wrote:
> I could not find any statement corresponding to this predicate in the spec. Is
> there?


RFC7231 says...
> http://tools.ietf.org/html/rfc7231#section-4.3.1

> 4.3.1.  GET
> ...
> A payload within a GET request message has no defined semantics;
> sending a payload body on a GET request might cause some existing
> implementations to reject the request.

> 4.3.2.  HEAD
> ...
> A payload within a HEAD request message has no defined semantics;
> sending a payload body on a HEAD request might cause some existing
>  implementations to reject the request.

So I think we can ignore the body in GET and HEAD request.

If we send body in GET/HEAD request, DCHECK fails in
WebURLLoaderImpl::Context::Start()
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...

[yhirano, 2014-09-05 04:11:40]

lgtm

[horo, 2014-09-05 04:18:04]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-05 04:18:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/horo@chromium.org/533123002/60001

[commit-bot: I haz the power, 2014-09-05 05:26:32]

Committed patchset #4 (id:60001) as 181436