[ServiceWorker] Implement Headers class.

[ServiceWorker] Implement Headers class.

I will replace HeaderMap class with this in another change.

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

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

[horo, 2014-06-25 07:28:00]

yhirano@
Could you please review this?

[yhirano, 2014-06-26 07:22:13]

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:22: RefPtr<FetchHeaderList>
list = adoptRef(new FetchHeaderList());
Can you use create?

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:95: {
Let's clear |result| here.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:147: return
isOnAccessControlSimpleRequestHeaderWhitelist(AtomicString(name),
AtomicString(value));
What isOnAccessControlSimpleRequestHeaderWhilelist does is different from the
description. Is it OK?

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.h (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:17: class FetchHeaderList :
public RefCounted<FetchHeaderList> {
FINAL?

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:21: PassRefPtr<FetchHeaderList>
createCopy();
This function is never called.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:36: bool
containsNonSimpleHeader() const;
This function is never called.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:48: Vector<OwnPtr<Header> >
m_headerList;
Can you tell me why you use OwnPtr here?

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:224: {
Can this method be called when |this| is not empty?
If not, please describe it (if this is a public method) and add an assertion.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:238: {
ditto

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:257: if (i != 0 &&
!dictionary.get(keys[i], keyValuePair)) {
[optional] I would split these conditions.
 // We've already got the keyValuePair for key[0].
 if (i > 0) {
    if (!get(...)) {
      es.throwTypeError(...);
      return;
    }
 }

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:261: if (keyValuePair.size() != 2) {
I'm not sure whether the spec says the size must be 2 or the size must be
greater than 1.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:25: typedef std::pair<String, String>
Header;
Do you need this typedef?

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:48: void clearHeaderList();
This function is never called.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:49: void fillWith(const Headers*,
ExceptionState&);
I think these two fillWith functions can be private.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:54: PassRefPtr<FetchHeaderList> list()
const { return m_headerList; }
I'm not sure if we need this function.

[horo, 2014-06-26 08:30:08]

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:22: RefPtr<FetchHeaderList>
list = adoptRef(new FetchHeaderList());
On 2014/06/26 07:22:12, yhirano wrote:
> Can you use create?

deleted.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:95: {
On 2014/06/26 07:22:12, yhirano wrote:
> Let's clear |result| here.

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:147: return
isOnAccessControlSimpleRequestHeaderWhitelist(AtomicString(name),
AtomicString(value));
On 2014/06/26 07:22:12, yhirano wrote:
> What isOnAccessControlSimpleRequestHeaderWhilelist does is different from the
> description. Is it OK?

Could you please let me know what is the difference?
I think it is doing the same thing.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.h (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:17: class FetchHeaderList :
public RefCounted<FetchHeaderList> {
On 2014/06/26 07:22:12, yhirano wrote:
> FINAL?

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:21: PassRefPtr<FetchHeaderList>
createCopy();
On 2014/06/26 07:22:12, yhirano wrote:
> This function is never called.

removed.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:36: bool
containsNonSimpleHeader() const;
On 2014/06/26 07:22:12, yhirano wrote:
> This function is never called.

done.removed.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.h:48: Vector<OwnPtr<Header> >
m_headerList;
On 2014/06/26 07:22:12, yhirano wrote:
> Can you tell me why you use OwnPtr here?

I wanted to reduce the cost of reallocating the existing headers while appending
other header.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:224: {
On 2014/06/26 07:22:13, yhirano wrote:
> Can this method be called when |this| is not empty?
> If not, please describe it (if this is a public method) and add an assertion.

Added 
    ASSERT(m_headerList->size() == 0);

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:238: {
On 2014/06/26 07:22:13, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:257: if (i != 0 &&
!dictionary.get(keys[i], keyValuePair)) {
On 2014/06/26 07:22:13, yhirano wrote:
> [optional] I would split these conditions.
>  // We've already got the keyValuePair for key[0].
>  if (i > 0) {
>     if (!get(...)) {
>       es.throwTypeError(...);
>       return;
>     }
>  }

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.cpp:261: if (keyValuePair.size() != 2) {
On 2014/06/26 07:22:13, yhirano wrote:
> I'm not sure whether the spec says the size must be 2 or the size must be
> greater than 1.

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:25: typedef std::pair<String, String>
Header;
On 2014/06/26 07:22:13, yhirano wrote:
> Do you need this typedef?

Done.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:48: void clearHeaderList();
On 2014/06/26 07:22:13, yhirano wrote:
> This function is never called.

deleted.

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:49: void fillWith(const Headers*,
ExceptionState&);
On 2014/06/26 07:22:13, yhirano wrote:
> I think these two fillWith functions can be private.

moved to private

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Headers.h:54: PassRefPtr<FetchHeaderList> list()
const { return m_headerList; }
On 2014/06/26 07:22:13, yhirano wrote:
> I'm not sure if we need this function.

deleted.

[yhirano, 2014-06-26 08:55:10]

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:147: return
isOnAccessControlSimpleRequestHeaderWhitelist(AtomicString(name),
AtomicString(value));
On 2014/06/26 08:30:08, horo wrote:
> On 2014/06/26 07:22:12, yhirano wrote:
> > What isOnAccessControlSimpleRequestHeaderWhilelist does is different from
the
> > description. Is it OK?
> 
> Could you please let me know what is the difference?
> I think it is doing the same thing.
"origin" and "referer" are whitelisted in the function.

https://codereview.chromium.org/358573002/diff/150001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/150001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:12: #include
"modules/serviceworkers/FetchManager.h"
Do you need this include?

[horo, 2014-06-26 09:16:52]

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchHeaderList.cpp:147: return
isOnAccessControlSimpleRequestHeaderWhitelist(AtomicString(name),
AtomicString(value));
On 2014/06/26 08:55:10, yhirano wrote:
> On 2014/06/26 08:30:08, horo wrote:
> > On 2014/06/26 07:22:12, yhirano wrote:
> > > What isOnAccessControlSimpleRequestHeaderWhilelist does is different from
> the
> > > description. Is it OK?
> > 
> > Could you please let me know what is the difference?
> > I think it is doing the same thing.
> "origin" and "referer" are whitelisted in the function.

Oh, I didn't noticed that.
Thank you for pointing out.

https://codereview.chromium.org/358573002/diff/150001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/150001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:12: #include
"modules/serviceworkers/FetchManager.h"
On 2014/06/26 08:55:10, yhirano wrote:
> Do you need this include?

deleted.

[yhirano, 2014-06-26 11:51:30]

lgtm

[horo, 2014-06-26 12:06:27]

falken@
Could you please review this?

[falken, 2014-06-27 02:08:06]

looks good

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/headers-worker.js (right):

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:91:
assert_equals(headers.size, 6);
can you add a description indicating the size should increase by one?

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:93:
assert_equals(headers.get('X-ServiceWorker-Test'), 'response test field -
updated');
add a description that the value of the first header added should be returned

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:93:
assert_equals(headers.get('X-ServiceWorker-Test'), 'response test field -
updated');
nit: seems to read better if line 92 and 93 are swapped

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:98:
assert_equals(headers.size, 5);
seems a description would be useful here, and for several below (size should
remain the same, increase/decrease by one, etc.)

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:134:
'Headers.append with an invalid name should throw');
are there any invalid names other than ''? could be worth using different names
for more coverage

https://codereview.chromium.org/358573002/diff/190001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoaderClientWrapper.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoaderClientWrapper.h:100: m_done = true;
this change looks unrelated to Headers?

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.cpp:31: // a new header whose name
is name and value is value, to list.
I didn't realize at first this is a quote from the spec. I recommend you enclose
spec quotes in quotation marks and put a comment that such comments in this file
are quotes from the spec. Where reasonable, I'd also write italicized words like
|this|. Otherwise it's really hard to parse.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.cpp:70: }
nit: blink code typically omits these braces

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchHeaderList.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.h:19: typedef std::pair<String,
String> Header;
nit: I think by include-what-you-use rules you should #include <utility>

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:143: return String();
could this just be return result like getAll below?

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:249: //   1. If header does not
contain two items, throw a TypeError.
spec is a unclear if this means exactly two items or at least two items. I filed
a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26217

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:250: //   2. Append header's first
item/header's second time to headers.
spec has a typo: s/time/item. I filed a spec bug. Can you fix in comment?

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.h:53: Headers(FetchHeaderList*);
nit: explicit

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.idl (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.idl:21: // The followings are not defined
in the spec yet.
grammar nit: "following"

[horo, 2014-06-27 04:19:11]

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/headers-worker.js (right):

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:91:
assert_equals(headers.size, 6);
On 2014/06/27 02:08:06, falken wrote:
> can you add a description indicating the size should increase by one?

Done.

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:93:
assert_equals(headers.get('X-ServiceWorker-Test'), 'response test field -
updated');
On 2014/06/27 02:08:05, falken wrote:
> nit: seems to read better if line 92 and 93 are swapped

Done.

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:93:
assert_equals(headers.get('X-ServiceWorker-Test'), 'response test field -
updated');
On 2014/06/27 02:08:06, falken wrote:
> add a description that the value of the first header added should be returned

Done.

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:98:
assert_equals(headers.size, 5);
On 2014/06/27 02:08:05, falken wrote:
> seems a description would be useful here, and for several below (size should
> remain the same, increase/decrease by one, etc.)

Done.

https://codereview.chromium.org/358573002/diff/190001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:134:
'Headers.append with an invalid name should throw');
On 2014/06/27 02:08:05, falken wrote:
> are there any invalid names other than ''? could be worth using different
names
> for more coverage

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/core/loader/Thre...
File Source/core/loader/ThreadableLoaderClientWrapper.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/core/loader/Thre...
Source/core/loader/ThreadableLoaderClientWrapper.h:100: m_done = true;
On 2014/06/27 02:08:06, falken wrote:
> this change looks unrelated to Headers?

Removed this file.
It was accidental included to this patch.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchHeaderList.cpp (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.cpp:31: // a new header whose name
is name and value is value, to list.
On 2014/06/27 02:08:06, falken wrote:
> I didn't realize at first this is a quote from the spec. I recommend you
enclose
> spec quotes in quotation marks and put a comment that such comments in this
file
> are quotes from the spec. Where reasonable, I'd also write italicized words
like
> |this|. Otherwise it's really hard to parse.

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.cpp:70: }
On 2014/06/27 02:08:06, falken wrote:
> nit: blink code typically omits these braces

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchHeaderList.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/FetchHeaderList.h:19: typedef std::pair<String,
String> Header;
On 2014/06/27 02:08:06, falken wrote:
> nit: I think by include-what-you-use rules you should #include <utility>

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.cpp (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:143: return String();
On 2014/06/27 02:08:06, falken wrote:
> could this just be return result like getAll below?

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:249: //   1. If header does not
contain two items, throw a TypeError.
On 2014/06/27 02:08:06, falken wrote:
> spec is a unclear if this means exactly two items or at least two items. I
filed
> a spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26217

Thank you.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.cpp:250: //   2. Append header's first
item/header's second time to headers.
On 2014/06/27 02:08:06, falken wrote:
> spec has a typo: s/time/item. I filed a spec bug. Can you fix in comment?

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.h:53: Headers(FetchHeaderList*);
On 2014/06/27 02:08:06, falken wrote:
> nit: explicit

Done.

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.idl (right):

https://codereview.chromium.org/358573002/diff/190001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.idl:21: // The followings are not defined
in the spec yet.
On 2014/06/27 02:08:06, falken wrote:
> grammar nit: "following"

Done.

[falken, 2014-06-27 07:21:21]

lgtm

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/headers-worker.js (right):

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:21:
assert_equals(headers.get(key), expectedMap[key]);
how about also testing assert_false(headers.has('dummy'))

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:206: 
nit: extra newline here?

https://codereview.chromium.org/358573002/diff/220001/Source/modules/servicew...
File Source/modules/serviceworkers/HeadersForEachCallback.idl (right):

https://codereview.chromium.org/358573002/diff/220001/Source/modules/servicew...
Source/modules/serviceworkers/HeadersForEachCallback.idl:4: 
Can you add also put a comment here that this IDL is not spec'd yet?

[horo, 2014-06-27 07:50:38]

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/headers-worker.js (right):

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:21:
assert_equals(headers.get(key), expectedMap[key]);
On 2014/06/27 07:21:21, falken wrote:
> how about also testing assert_false(headers.has('dummy'))

Done.

https://codereview.chromium.org/358573002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/headers-worker.js:206: 
On 2014/06/27 07:21:21, falken wrote:
> nit: extra newline here?

Done.

https://codereview.chromium.org/358573002/diff/220001/Source/modules/servicew...
File Source/modules/serviceworkers/HeadersForEachCallback.idl (right):

https://codereview.chromium.org/358573002/diff/220001/Source/modules/servicew...
Source/modules/serviceworkers/HeadersForEachCallback.idl:4: 
On 2014/06/27 07:21:21, falken wrote:
> Can you add also put a comment here that this IDL is not spec'd yet?

Done.

[horo, 2014-06-27 07:51:09]

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

[haraken, 2014-06-27 08:54:58]

LGTM for bindings/.

https://codereview.chromium.org/358573002/diff/240001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.h:22: class Headers FINAL : public
ScriptWrappable, public RefCounted<Headers> {

Put RefCounted<Header> before ScriptWrappable (just a convention though).

[horo, 2014-06-27 09:00:04]

https://codereview.chromium.org/358573002/diff/240001/Source/modules/servicew...
File Source/modules/serviceworkers/Headers.h (right):

https://codereview.chromium.org/358573002/diff/240001/Source/modules/servicew...
Source/modules/serviceworkers/Headers.h:22: class Headers FINAL : public
ScriptWrappable, public RefCounted<Headers> {
On 2014/06/27 08:54:57, haraken wrote:
> 
> Put RefCounted<Header> before ScriptWrappable (just a convention though).

Done.

[horo, 2014-06-27 09:00:21]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-06-27 09:00:32]

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

[horo, 2014-06-27 09:25:38]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-06-27 09:26:35]

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

[commit-bot: I haz the power, 2014-06-27 10:40:19]

Change committed as 177088