[ServiceWorker] Make Response class better conformance with the spec.

[ServiceWorker] Make Response class better conformance with the spec.

In current implementation Response class only supports status, statusText, and headers attributes.
This change introduce type, url attributes and change the class of headers attribute from HeaderMap to Headers and make all attributes readonly.
And also this change implements the logic which is defined in the spec.

http://fetch.spec.whatwg.org/#response-class 
http://fetch.spec.whatwg.org/#response


This cl depends on http://crrev.com/329853012 and http://crrev.com/370733002

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

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

[horo, 2014-07-07 02:15:51]

falken@
Could you please review this?

[falken, 2014-07-07 08:10:41]

looking good

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/response-worker.js (right):

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:6:
assert_equals(response.url, '', 'Response.url should not be set');
maybe "should be the empty string"?

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:30: status:
303,
4 space indent

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:48:
responses.forEach(function(response){
space before {

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:81: var
response = new Response(new Blob(['dummy'], {type : 'audio/wav'}));
no space before :

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:96: function()
{ var response = new Response(new Blob(), {status: status}); },
I suspect you don't need the "var response"

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:109: var obj =
{};
4 space indent (here, and the ones below)

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:110: obj[name]
= 'a';
can this just be var obj = { name: 'a' }? or even just not use obj altogether:
  new Response(new Blob(), {headers: {name: 'a'}});

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:124: var
response =
seems like you can save room by removing the "var response"

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:42: response->
m_blobDataHandle = m_blobDataHandle;
errant space

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:69: response->
m_blobDataHandle = m_blobDataHandle;
space

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.h (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.h:63: 
extra newline

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:67: if
(!r->m_response->headerList()->has("Content-Type") && !body->type().isEmpty())
The isEmpty() check doesn't seem to be in the quoted spec. Is there a
discrepancy here?

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:125: 
extra newline

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:132: 
extra newl

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:36: Dictionary headersDictionary;
We haven't been following it but I guess members should start with m_

[horo, 2014-07-07 10:19:44]

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/response-worker.js (right):

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:6:
assert_equals(response.url, '', 'Response.url should not be set');
On 2014/07/07 08:10:40, falken wrote:
> maybe "should be the empty string"?

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:30: status:
303,
On 2014/07/07 08:10:40, falken wrote:
> 4 space indent

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:48:
responses.forEach(function(response){
On 2014/07/07 08:10:40, falken wrote:
> space before {

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:81: var
response = new Response(new Blob(['dummy'], {type : 'audio/wav'}));
On 2014/07/07 08:10:40, falken wrote:
> no space before :

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:96: function()
{ var response = new Response(new Blob(), {status: status}); },
On 2014/07/07 08:10:40, falken wrote:
> I suspect you don't need the "var response"

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:109: var obj =
{};
On 2014/07/07 08:10:40, falken wrote:
> 4 space indent (here, and the ones below)

Done.

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:110: obj[name]
= 'a';
On 2014/07/07 08:10:40, falken wrote:
> can this just be var obj = { name: 'a' }? or even just not use obj altogether:
>   new Response(new Blob(), {headers: {name: 'a'}});
No.
{name: 'a'}
means
obj['name'] = 'a';

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:124: var
response =
On 2014/07/07 08:10:40, falken wrote:
> seems like you can save room by removing the "var response"

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:42: response->
m_blobDataHandle = m_blobDataHandle;
On 2014/07/07 08:10:40, falken wrote:
> errant space

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.cpp:69: response->
m_blobDataHandle = m_blobDataHandle;
On 2014/07/07 08:10:40, falken wrote:
> space

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.h (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.h:63: 
On 2014/07/07 08:10:40, falken wrote:
> extra newline

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:67: if
(!r->m_response->headerList()->has("Content-Type") && !body->type().isEmpty())
On 2014/07/07 08:10:40, falken wrote:
> The isEmpty() check doesn't seem to be in the quoted spec. Is there a
> discrepancy here?

Done.
Changed to !isNull()

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:125: 
On 2014/07/07 08:10:40, falken wrote:
> extra newline

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:132: 
On 2014/07/07 08:10:40, falken wrote:
> extra newl

Done.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:36: Dictionary headersDictionary;
On 2014/07/07 08:10:40, falken wrote:
> We haven't been following it but I guess members should start with m_

Done.

[falken, 2014-07-08 03:56:24]

lgtm if comments are addressed

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/response-worker.js (right):

https://codereview.chromium.org/373613004/diff/140001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:110: obj[name]
= 'a';
On 2014/07/07 10:19:44, horo wrote:
> On 2014/07/07 08:10:40, falken wrote:
> > can this just be var obj = { name: 'a' }? or even just not use obj
altogether:
> >   new Response(new Blob(), {headers: {name: 'a'}});
> No.
> {name: 'a'}
> means
> obj['name'] = 'a';

Ah, right.

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:67: if
(!r->m_response->headerList()->has("Content-Type") && !body->type().isEmpty())
On 2014/07/07 10:19:44, horo wrote:
> On 2014/07/07 08:10:40, falken wrote:
> > The isEmpty() check doesn't seem to be in the quoted spec. Is there a
> > discrepancy here?
> 
> Done.
> Changed to !isNull()

I'm still not sure this follows the spec. The spec seems to says you always
append "Content-Type" to |r|'s header list if it doesn't exist.

Scanning the spec's "extract body" algorithm, it looks like "Content-Type" is
set to the empty byte sequence by default. So it looks like we should be adding
"Content-Type": "" to |r| if body->type()->isNull(). WDYT?

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/response-worker.js (right):

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:59:
response.headers.set('X-ServiceWorker-Test2', 'response test field2')
missing semicolon

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:87:
{headers:{'Content-Type': 'text/html; charset=UTF-8'}})
missing semicolon

[horo, 2014-07-08 05:04:44]

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/373613004/diff/140001/Source/modules/servicew...
Source/modules/serviceworkers/Response.cpp:67: if
(!r->m_response->headerList()->has("Content-Type") && !body->type().isEmpty())
On 2014/07/08 03:56:24, falken wrote:
> On 2014/07/07 10:19:44, horo wrote:
> > On 2014/07/07 08:10:40, falken wrote:
> > > The isEmpty() check doesn't seem to be in the quoted spec. Is there a
> > > discrepancy here?
> > 
> > Done.
> > Changed to !isNull()
> 
> I'm still not sure this follows the spec. The spec seems to says you always
> append "Content-Type" to |r|'s header list if it doesn't exist.
> 
> Scanning the spec's "extract body" algorithm, it looks like "Content-Type" is
> set to the empty byte sequence by default. So it looks like we should be
adding
> "Content-Type": "" to |r| if body->type()->isNull(). WDYT?


Humm..
I can't say which is correct.
I filed an issue.
https://github.com/slightlyoff/ServiceWorker/issues/363
And I changed this cl to add a empty string.

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/response-worker.js (right):

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:59:
response.headers.set('X-ServiceWorker-Test2', 'response test field2')
On 2014/07/08 03:56:24, falken wrote:
> missing semicolon

Done.

https://codereview.chromium.org/373613004/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:87:
{headers:{'Content-Type': 'text/html; charset=UTF-8'}})
On 2014/07/08 03:56:24, falken wrote:
> missing semicolon

Done.

[horo, 2014-07-08 07:45:26]

haraken@
Could you please review Source/bindings/core/v8/Dictionary.*?

[haraken, 2014-07-08 08:18:21]

bindings/ LGTM.

[horo, 2014-07-08 08:25:55]

tkent@
Could you please review Response.idl as API owner?

[tkent, 2014-07-08 08:40:24]

> [ServiceWorker] Make Response class better conformance with the spec. 
>
> http://fetch.spec.whatwg.org/#response-class 
> http://fetch.spec.whatwg.org/#response 

Please write what this CL changes concretely.

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.h:20: class FetchResponseData :
public RefCounted<FetchResponseData>  {
nit: extra space before {

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:33: unsigned short m_status;
We don't add m_ to struct members.

[falken, 2014-07-08 08:51:20]

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:33: unsigned short m_status;
On 2014/07/08 08:40:23, tkent wrote:
> We don't add m_ to struct members.

I directed horo@ to add m_ after seeing jochen@'s comment on:
https://codereview.chromium.org/329853012/diff/330001/Source/modules/servicew...

Is there a definitive style decision?
(The codebase looks inconsistent about this, I see cases of both.)

[tkent, 2014-07-08 08:56:19]

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:33: unsigned short m_status;
On 2014/07/08 08:51:20, falken wrote:
> On 2014/07/08 08:40:23, tkent wrote:
> > We don't add m_ to struct members.
> 
> I directed horo@ to add m_ after seeing jochen@'s comment on:
>
https://codereview.chromium.org/329853012/diff/330001/Source/modules/servicew...
> 
> Is there a definitive style decision?
> (The codebase looks inconsistent about this, I see cases of both.)

http://www.chromium.org/blink/coding-style
The style guide says:
> Data members in C++ classes should be private. Static data members should be
prefixed by "s_". Other data members should be prefixed by "m_".

If an engineer thinks a struct is not a C++ class, he should not add "m_".
If an engineer thinks a struct is a kind of C++ class, he should make members
private. In this case, using struct makes no sense.

[horo, 2014-07-08 09:13:22]

> Please write what this CL changes concretely.
done

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchResponseData.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/FetchResponseData.h:20: class FetchResponseData :
public RefCounted<FetchResponseData>  {
On 2014/07/08 08:40:23, tkent wrote:
> nit: extra space before {

Done.

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
File Source/modules/serviceworkers/ResponseInit.h (right):

https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
Source/modules/serviceworkers/ResponseInit.h:33: unsigned short m_status;
On 2014/07/08 08:40:23, tkent wrote:
> We don't add m_ to struct members.

Done.

[jochen (gone - plz use gerrit), 2014-07-08 09:27:42]

On 2014/07/08 at 08:51:20, falken wrote:
>
https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
> File Source/modules/serviceworkers/ResponseInit.h (right):
> 
>
https://codereview.chromium.org/373613004/diff/260001/Source/modules/servicew...
> Source/modules/serviceworkers/ResponseInit.h:33: unsigned short m_status;
> On 2014/07/08 08:40:23, tkent wrote:
> > We don't add m_ to struct members.
> 
> I directed horo@ to add m_ after seeing jochen@'s comment on:
>
https://codereview.chromium.org/329853012/diff/330001/Source/modules/servicew...
> 
> Is there a definitive style decision?
> (The codebase looks inconsistent about this, I see cases of both.)

the structs I looked at in platform/network all had m_ prefixes, so I thought
that would be more consistent for request related stuff.

[tkent, 2014-07-08 12:50:37]

lgtm

[horo, 2014-07-10 00:59:58]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-07-10 01:00:04]

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

[commit-bot: I haz the power, 2014-07-10 01:57:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/16150)

[commit-bot: I haz the power, 2014-07-10 02:28:51]

Change committed as 177793

[Yuta Kitamura, 2014-07-10 05:12:39]

A revert of this CL has been created in
https://codereview.chromium.org/383653002/ by yutak@chromium.org.

The reason for reverting is: Broke content_browsertests

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/37860

ServiceWorkerVersionBrowserTest.FetchEvent_Response
ServiceWorkerBrowserTest.Reload
.

[horo, 2014-07-10 11:30:10]

Reopened.

I disabled these tests.
https://src.chromium.org/viewvc/chrome?revision=282309&view=revision

I will re-enable the tests after this patch will land.

[horo, 2014-07-10 11:30:27]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-07-10 11:30:40]

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

[commit-bot: I haz the power, 2014-07-10 11:31:19]

Change committed as 177817