ServiceWorker: support Response.{status,statusText,headers} [blink] (2/3)

ServiceWorker: support Response.{status,statusText,headers} [blink] (2/3)

Multi-sided patch:
1/3: chromium-side, https://codereview.chromium.org/299003002/
2/3: blink-side, THIS PATCH
3/3: chromium-side, cleanup & re-enable test (n/a yet)

BUG=376733
TEST=http/tests/serviceworker/response.html
TEST=http/tests/serviceworker/headermap.html

[kinuko, 2014-05-23 13:09:01]

Depends on chromium-side patch, but this one probably needs to be reviewed
first. PTL

[jsbell, 2014-05-23 20:59:48]

overall looking good

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/headermap-expected.txt (right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/headermap-expected.txt:2: PASS HeaderMap in
ServiceWorkerGlobalScope
Should this PASS be in
virtual/serviceworker/http/tests/serviceworker/headermap-expected.txt and a FAIL
here?

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/headermap-worker.js (right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:4: var
headerMap = {
Rename this to 'expectedMap'? header vs. headerMap vs. HeaderMap gets confusing.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:15: //
'size()'
nit: 'size' - no ()

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:34: //
'set()'
Since this is [MapClass(DOMString,DOMString)], add a case which sets a number
and verifies that it gets a string back out?

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:34: //
'set()'
Do we want to toss in a case-sensitivity test?

(HTTP headers are case-insensitive e.g. in XHR, but I'm not seeing anything in
the SW or WebIDL spec that would apply that here.)

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

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:4: var
headerMap = {
Rename?

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/virtual/serv...
File
LayoutTests/virtual/serviceworker/http/tests/serviceworker/response-expected.txt
(right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/virtual/serv...
LayoutTests/virtual/serviceworker/http/tests/serviceworker/response-expected.txt:2:
PASS HeaderMap in ServiceWorkerGlobalScope
This should be 'Response in ServiceWorkerGlobalScope' shouldn't it?

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/FooCallback.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/FooCallback.idl:1: //callback interface
FooCallback {
Included this file by accident?

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

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.cpp:70: for (HashMap<String,
String>::const_iterator it = m_headers.begin(); it != m_headers.end(); ++it) {
Do we need to terminate the loop if the callback throws?

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/HeaderMap.h (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.h:39: HashMap<String, String> m_headers;
Does this maintain order? ES6's Map type has the pesky requirement that
iteration order matches insertion order; WebIDL's [MapClass] may require the
same thing.

(I wouldn't worry for now, but maybe add a FIXME? Do we need ListHashMap?)

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/HeaderMap.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.idl:16: // MapClass.
(http://crbug.com/347435)
Add a comment that this should be MapClass(DOMString, DOMString)

Also, can you add the spec URL above the attributes (line 4)? The bindings folks
have started doing that so it's easy for them to compare spec IDL vs. our IDL
files.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:34: void
setHeaders(PassRefPtr<HeaderMap>);
Where is this used?

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.idl:17: getter DOMString
getHeader(DOMString value);
Looks like the getter/setter are gone in the latest spec?

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:62: //    unsigned short m_status;
Delete these?

[falken, 2014-05-24 14:32:36]

looking good

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/headermap-worker.js (right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:16:
assert_equals(headers.size, 3, 'Response.headers.size should match');
Remove "Response."?

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:39:
assert_equals(headers.get(newKey), newValue);
Maybe throw in a headers.size == 3 assert? Also, a test for .set() mutating an
existing value?

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

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:26:
Object.keys(headerMap).forEach(function(key) {
Response.h implements namedPropertyEnumerator, but I'm not sure it's tested. Can
we iterate over Response instead to exercise that function?

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/HeaderMap.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.idl:13: // FIXME: Some of the method
definitiones (like set) don't strictly
typo "definitiones"

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:37: void
namedPropertyEnumerator(Vector<String>& names, ExceptionState&);
The Blink IDL documentation seems to say you must say 
[Custom=PropertyQuery|PropertyEnumerator] in the IDL to use these.[1] Is this
wrong or am I reading it wrong?

[1]
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Custom...

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:10: #include
"public/platform/WebPrivatePtr.h"
double include WebPrivatePtr.h

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:34: WebServiceWorkerResponse&
operator=(const WebServiceWorkerResponse& r)
I think Blink style is to write out full words.. "r" -> "response"

[kinuko, 2014-05-26 05:45:12]

Thx, updated.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/headermap-expected.txt (right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/headermap-expected.txt:2: PASS HeaderMap in
ServiceWorkerGlobalScope
On 2014/05/23 20:59:49, jsbell wrote:
> Should this PASS be in
> virtual/serviceworker/http/tests/serviceworker/headermap-expected.txt and a
FAIL
> here?

Um... did this again. Fixed.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/headermap-worker.js (right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:4: var
headerMap = {
On 2014/05/23 20:59:49, jsbell wrote:
> Rename this to 'expectedMap'? header vs. headerMap vs. HeaderMap gets
confusing.

Done.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:15: //
'size()'
On 2014/05/23 20:59:49, jsbell wrote:
> nit: 'size' - no ()

Done.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:16:
assert_equals(headers.size, 3, 'Response.headers.size should match');
On 2014/05/24 14:32:36, falken wrote:
> Remove "Response."?

Done.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:34: //
'set()'
On 2014/05/23 20:59:49, jsbell wrote:
> Do we want to toss in a case-sensitivity test?
> 
> (HTTP headers are case-insensitive e.g. in XHR, but I'm not seeing anything in
> the SW or WebIDL spec that would apply that here.)

Done. For now I just added a FIXME comment about case-insensitivity behavior
possibility.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/headermap-worker.js:39:
assert_equals(headers.get(newKey), newValue);
On 2014/05/24 14:32:36, falken wrote:
> Maybe throw in a headers.size == 3 assert? Also, a test for .set() mutating an
> existing value?

Done.

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

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:4: var
headerMap = {
On 2014/05/23 20:59:49, jsbell wrote:
> Rename?

Done.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/response-worker.js:26:
Object.keys(headerMap).forEach(function(key) {
On 2014/05/24 14:32:36, falken wrote:
> Response.h implements namedPropertyEnumerator, but I'm not sure it's tested.
Can
> we iterate over Response instead to exercise that function?

Actually the spec got updated and we no longer seem to implement getter/setter
behavior either, so I just removed this forEach part.

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/virtual/serv...
File
LayoutTests/virtual/serviceworker/http/tests/serviceworker/response-expected.txt
(right):

https://codereview.chromium.org/287363004/diff/20001/LayoutTests/virtual/serv...
LayoutTests/virtual/serviceworker/http/tests/serviceworker/response-expected.txt:2:
PASS HeaderMap in ServiceWorkerGlobalScope
On 2014/05/23 20:59:49, jsbell wrote:
> This should be 'Response in ServiceWorkerGlobalScope' shouldn't it?

Yup, done.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/FooCallback.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/FooCallback.idl:1: //callback interface
FooCallback {
On 2014/05/23 20:59:49, jsbell wrote:
> Included this file by accident?

Oops... yes, removed.

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

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.cpp:70: for (HashMap<String,
String>::const_iterator it = m_headers.begin(); it != m_headers.end(); ++it) {
On 2014/05/23 20:59:49, jsbell wrote:
> Do we need to terminate the loop if the callback throws?

Um, maybe... done.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/HeaderMap.h (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.h:39: HashMap<String, String> m_headers;
On 2014/05/23 20:59:49, jsbell wrote:
> Does this maintain order? ES6's Map type has the pesky requirement that
> iteration order matches insertion order; WebIDL's [MapClass] may require the
> same thing.
> 
> (I wouldn't worry for now, but maybe add a FIXME? Do we need ListHashMap?)

No, and yeah the notion of MapClass ordering worried too (that's one of the
reasons I added a comment about compatibility in .idl). Added explicit FIXME
comment here about the ordering.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/HeaderMap.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.idl:13: // FIXME: Some of the method
definitiones (like set) don't strictly
On 2014/05/24 14:32:36, falken wrote:
> typo "definitiones"

Done.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/HeaderMap.idl:16: // MapClass.
(http://crbug.com/347435)
On 2014/05/23 20:59:49, jsbell wrote:
> Add a comment that this should be MapClass(DOMString, DOMString)
> 
> Also, can you add the spec URL above the attributes (line 4)? The bindings
folks
> have started doing that so it's easy for them to compare spec IDL vs. our IDL
> files.

Done.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:34: void
setHeaders(PassRefPtr<HeaderMap>);
On 2014/05/23 20:59:49, jsbell wrote:
> Where is this used?

The spec's updated and now it's (officially) a readonly attribute, so I removed
it.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:37: void
namedPropertyEnumerator(Vector<String>& names, ExceptionState&);
On 2014/05/24 14:32:36, falken wrote:
> The Blink IDL documentation seems to say you must say 
> [Custom=PropertyQuery|PropertyEnumerator] in the IDL to use these.[1] Is this
> wrong or am I reading it wrong?
> 
> [1]
>
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Custom...

I was likely mistaken, and the spec's updated so we no longer need to worry
about these ones. Removed.

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.idl (right):

https://codereview.chromium.org/287363004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.idl:17: getter DOMString
getHeader(DOMString value);
On 2014/05/23 20:59:49, jsbell wrote:
> Looks like the getter/setter are gone in the latest spec?

Yes, so I updated the code.

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:10: #include
"public/platform/WebPrivatePtr.h"
On 2014/05/24 14:32:36, falken wrote:
> double include WebPrivatePtr.h

Done.

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:34: WebServiceWorkerResponse&
operator=(const WebServiceWorkerResponse& r)
On 2014/05/24 14:32:36, falken wrote:
> I think Blink style is to write out full words.. "r" -> "response"

Done, but for operator= impl using assign() in public/platform headers using a
single-char variable seems to be pretty common.

https://codereview.chromium.org/287363004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:62: //    unsigned short m_status;
On 2014/05/23 20:59:49, jsbell wrote:
> Delete these?

Done.

[kinuko, 2014-05-26 05:52:19]

jochen@: could you do owner's review for changes in:

Source/bindings/v8
Source/platform
public/platform

[falken, 2014-05-26 09:06:58]

Thanks, I think this lg to me but have a question.

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
File Source/platform/exported/WebServiceWorkerResponse.cpp (right):

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
Source/platform/exported/WebServiceWorkerResponse.cpp:10: class
WebServiceWorkerResponsePrivate : public
RefCounted<WebServiceWorkerResponsePrivate> {
I have a naive question: what's the motivation for using this class instead of
just having the private fields directly in WebServiceWorkerResponse? Do we
expect multiple WebServiceWorkerResponse to refer to the same Private? (I see
it's a common pattern in public...)

[jochen (gone - plz use gerrit), 2014-05-26 09:38:08]

lgtm

[kinuko, 2014-05-26 13:51:35]

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
File Source/platform/exported/WebServiceWorkerResponse.cpp (right):

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
Source/platform/exported/WebServiceWorkerResponse.cpp:10: class
WebServiceWorkerResponsePrivate : public
RefCounted<WebServiceWorkerResponsePrivate> {
On 2014/05/26 09:06:59, falken wrote:
> I have a naive question: what's the motivation for using this class instead of
> just having the private fields directly in WebServiceWorkerResponse? Do we
> expect multiple WebServiceWorkerResponse to refer to the same Private? (I see
> it's a common pattern in public...)

We want to store map-like data in WebServiceWorkerResponse but we don't have
public exportable type at public blink interface for that (like WebVector for
Vector), so I moved them into private impl class. (The refcnt'ed shared part is
not really necessary in our case, I just chose it for easier copy handling)

[falken, 2014-05-26 14:59:30]

lgtm

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
File Source/platform/exported/WebServiceWorkerResponse.cpp (right):

https://codereview.chromium.org/287363004/diff/80001/Source/platform/exported...
Source/platform/exported/WebServiceWorkerResponse.cpp:10: class
WebServiceWorkerResponsePrivate : public
RefCounted<WebServiceWorkerResponsePrivate> {
On 2014/05/26 13:51:35, kinuko wrote:
> On 2014/05/26 09:06:59, falken wrote:
> > I have a naive question: what's the motivation for using this class instead
of
> > just having the private fields directly in WebServiceWorkerResponse? Do we
> > expect multiple WebServiceWorkerResponse to refer to the same Private? (I
see
> > it's a common pattern in public...)
> 
> We want to store map-like data in WebServiceWorkerResponse but we don't have
> public exportable type at public blink interface for that (like WebVector for
> Vector), so I moved them into private impl class. (The refcnt'ed shared part
is
> not really necessary in our case, I just chose it for easier copy handling)

Ah I see!

[kinuko, 2014-05-28 00:36:12]

(I'm landing this, jsbell: feel free to uncheck CQ and comments if you have any)

[kinuko, 2014-05-28 00:36:31]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-28 00:37:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/287363004/80001

[jsbell, 2014-05-28 01:29:34]

lgtm!

(The only bit of code I'm not personally confident about is the callback
definition, since I haven't dug into how that actually works in the binding
layer or look at other examples, but I didn't see any problems!)

[commit-bot: I haz the power, 2014-05-28 02:19:16]

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

[commit-bot: I haz the power, 2014-05-28 03:24:12]

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

[commit-bot: I haz the power, 2014-05-28 03:24:13]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[kinuko, 2014-05-28 05:50:08]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-05-28 05:50:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kinuko@chromium.org/287363004/100001

[commit-bot: I haz the power, 2014-05-28 07:07:55]

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

[commit-bot: I haz the power, 2014-05-28 07:07:55]

Could not make sense out of svn commit message.

[falken, 2014-05-29 03:36:11]

On 2014/05/28 07:07:55, I haz the power (commit-bot) wrote:
> Could not make sense out of svn commit message.

Strange error. It turns out the commit landed:
http://src.chromium.org/viewvc/blink?revision=174941&view=revision

(crbug.com/378626 filed)