[Fetch API] Put body members directly on Response/Request

[Fetch API] Put body members directly on Response/Request

http://fetch.spec.whatwg.org/

https://github.com/whatwg/fetch/commit/a898f9a2941350aa625aa79b24673628ac2b2a8e
Request.body and Response.body is removed from the spec.
Body interface is introduced.

interface Body {
  readonly attribute boolean bodyUsed;
  Promise<ArrayBuffer> arrayBuffer();
  Promise<Blob> blob();
  Promise<FormData> formData();
  Promise<JSON> json();
  Promise<ScalarValueString> text();
};
Request implements Body;
Response implements Body;

BUG=411743, 410197, 410196
TEST=http/tests/serviceworker/ http/tests/local/serviceworker/

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

[horo, 2014-09-09 03:23:54]

Patchset #1 (id:1) has been deleted

[horo, 2014-09-09 03:24:03]

Patchset #1 (id:20001) has been deleted

[horo, 2014-09-09 03:24:09]

Patchset #1 (id:40001) has been deleted

[horo, 2014-09-09 03:24:19]

Patchset #1 (id:60001) has been deleted

[horo, 2014-09-09 03:24:25]

Patchset #1 (id:80001) has been deleted

[horo, 2014-09-09 03:24:31]

Patchset #1 (id:100001) has been deleted

[horo, 2014-09-09 03:24:37]

Patchset #1 (id:120001) has been deleted

[horo, 2014-09-09 03:24:42]

Patchset #1 (id:140001) has been deleted

[horo, 2014-09-09 03:26:35]

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

[horo, 2014-09-09 03:26:36]

yhirano@
Could you please review this?

[yhirano, 2014-09-09 09:31:49]

https://codereview.chromium.org/555443002/diff/130026/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js
(right):

https://codereview.chromium.org/555443002/diff/130026/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:84:
// Send the result to fetch-access-control.html.
This handler "catch"es an error, i.e. port.postMessage({fetchResult: 'resolved',
...}) will be sent. Is that what you want to do?

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:44: // If the size of |blobHandle| is set
correctly, creates Blob from it.
Please wrap the comment in 80 columns.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:49: // If the size is not set, read as
ArrayBuffer and create a new blob to get the size.
ditto

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:51: // When we will stop using Blob as a
base system of Body to support stream, this problem should be solved.
ditto

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:44: bool bodyUsed();
+const

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:53: Body(ExecutionContext*);
You don't have to hide the constructor because we can't instantiate a class
having a pure virtual method.

[jsbell, 2014-09-09 22:04:24]

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

[jsbell, 2014-09-09 22:04:25]

Just some drive-by observations...

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:53: Body(ExecutionContext*);
single arg constructor should have 'explicit' qualifier

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Response.h:46: Response(ExecutionContext*);
single argument constructor should have `explicit` qualifier

[horo, 2014-09-10 06:18:44]

https://codereview.chromium.org/555443002/diff/130026/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js
(right):

https://codereview.chromium.org/555443002/diff/130026/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:84:
// Send the result to fetch-access-control.html.
On 2014/09/09 09:31:48, yhirano wrote:
> This handler "catch"es an error, i.e. port.postMessage({fetchResult:
'resolved',
> ...}) will be sent. Is that what you want to do?

I was wrong.
Fixed.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:44: // If the size of |blobHandle| is set
correctly, creates Blob from it.
On 2014/09/09 09:31:49, yhirano wrote:
> Please wrap the comment in 80 columns.

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:49: // If the size is not set, read as
ArrayBuffer and create a new blob to get the size.
On 2014/09/09 09:31:49, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:51: // When we will stop using Blob as a
base system of Body to support stream, this problem should be solved.
On 2014/09/09 09:31:49, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Body.h (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:44: bool bodyUsed();
On 2014/09/09 09:31:49, yhirano wrote:
> +const

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:53: Body(ExecutionContext*);
On 2014/09/09 22:04:25, jsbell wrote:
> single arg constructor should have 'explicit' qualifier

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Body.h:53: Body(ExecutionContext*);
On 2014/09/09 09:31:49, yhirano wrote:
> You don't have to hide the constructor because we can't instantiate a class
> having a pure virtual method.

Done.

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/555443002/diff/130026/Source/modules/servicew...
Source/modules/serviceworkers/Response.h:46: Response(ExecutionContext*);
On 2014/09/09 22:04:25, jsbell wrote:
> single argument constructor should have `explicit` qualifier

Done.

[yhirano, 2014-09-10 11:11:29]

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js
(right):

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:82:
res.text()
How about flattening then by doing something like

fetch(...).then(function(res) {
    response = res;
    return res.text();
}).then(function(body) {
    ...
}).catch(function(e) {
    ...
})?

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:11: #include "core/dom/ExceptionCode.h"
Do we need this include?

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:15: #include
"modules/serviceworkers/ResponseInit.h"
Do we need this include?

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:16: #include "platform/NotImplemented.h"
ditto

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:17: #include
"public/platform/WebServiceWorkerResponse.h"
ditto

[horo, 2014-09-11 03:07:15]

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js
(right):

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:82:
res.text()
On 2014/09/10 11:11:29, yhirano wrote:
> How about flattening then by doing something like
> 
> fetch(...).then(function(res) {
>     response = res;
>     return res.text();
> }).then(function(body) {
>     ...
> }).catch(function(e) {
>     ...
> })?

If do so, {fetchResult: 'rejected'} will be sent when an error occurs while
calling res.text() or port.postMessage().
But it is not what I intended.
I added catch handler to send {fetchResult: 'error'}.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:11: #include "core/dom/ExceptionCode.h"
On 2014/09/10 11:11:29, yhirano wrote:
> Do we need this include?

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:15: #include
"modules/serviceworkers/ResponseInit.h"
On 2014/09/10 11:11:29, yhirano wrote:
> Do we need this include?

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:16: #include "platform/NotImplemented.h"
On 2014/09/10 11:11:29, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:17: #include
"public/platform/WebServiceWorkerResponse.h"
On 2014/09/10 11:11:29, yhirano wrote:
> ditto

Done.

[horo, 2014-09-11 03:07:15]

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js
(right):

https://codereview.chromium.org/555443002/diff/170001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:82:
res.text()
On 2014/09/10 11:11:29, yhirano wrote:
> How about flattening then by doing something like
> 
> fetch(...).then(function(res) {
>     response = res;
>     return res.text();
> }).then(function(body) {
>     ...
> }).catch(function(e) {
>     ...
> })?

If do so, {fetchResult: 'rejected'} will be sent when an error occurs while
calling res.text() or port.postMessage().
But it is not what I intended.
I added catch handler to send {fetchResult: 'error'}.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:11: #include "core/dom/ExceptionCode.h"
On 2014/09/10 11:11:29, yhirano wrote:
> Do we need this include?

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:15: #include
"modules/serviceworkers/ResponseInit.h"
On 2014/09/10 11:11:29, yhirano wrote:
> Do we need this include?

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:16: #include "platform/NotImplemented.h"
On 2014/09/10 11:11:29, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/555443002/diff/170001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:17: #include
"public/platform/WebServiceWorkerResponse.h"
On 2014/09/10 11:11:29, yhirano wrote:
> ditto

Done.

[yhirano, 2014-09-11 03:43:27]

lgtm

[horo, 2014-09-11 03:47:25]

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

[horo, 2014-09-11 03:47:26]

tkent@
Could you please review Source/web/ServiceWorkerGlobalScopeProxy.cpp?

[tkent, 2014-09-11 04:05:44]

Source/web lgtm.

IMO, the name 'Body' is too generic...

[horo, 2014-09-11 04:28:03]

On 2014/09/11 04:05:44, tkent (overloaded) wrote:
> Source/web lgtm.
> 
> IMO, the name 'Body' is too generic...

Thank you.
I think so too.
But the name is defined in the spec http://fetch.spec.whatwg.org/#body.

[horo, 2014-09-11 04:28:09]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-11 04:28:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555443002/190001

[commit-bot: I haz the power, 2014-09-11 05:36:55]

Committed patchset #3 (id:190001) as 181793