[ServiceWorker] Make fetch() method better conformance with the spec.

[ServiceWorker] Make fetch() method better conformance with the spec.

In this patch, I don't implement the redirection procedure because it is a bit complicated and it could cause security issues.
When fetch() receives a redirect response, we treat it as an error in DocumentThreadableLoader::redirectReceived().

ResourceResponse's wasFetchedViaServiceWorker flag is used in ResourceFetcher::didReceiveResponse() and DocumentThreadableLoader::handleResponse() for CORS and CSP checking in the renderer side.
But this flag will be set correctly in https://codereview.chromium.org/375513002.
After this patch is landed, I will add LayoutTests for CORS and CSP checking as I described in https://codereview.chromium.org/375513002#msg18. 

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

[horo, 2014-07-16 06:52:06]

yhirano@
Could you please review this?

[yhirano, 2014-07-16 13:12:27]

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1317: if (!canRequest(resource->type(),
response.url(), resource->options(), false,
FetchRequest::UseDefaultOriginRestrictionForType)) {
Is this block needed because of the same reason as
DocumentThreadableLoader.cpp:334? If so, can you write a comment?

https://codereview.chromium.org/399543002/diff/100001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:345: if
(m_options.crossOriginRequestPolicy == UseAccessControl) {
This block is very similar to one from L355.
The only difference is this block checks securityOrigin()->canRequest(url) (in
isAllowedRedirect).
If you can add the check for securityOrigin()->canRequest(url) at L355, we can
merge the two blocks.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:213: if
(m_request->unsafeRequestFlag()
Can you merge this block into the above one?

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:284: // "1. Let |HTTPRequest| be
a copy of |request|, except that |HTTPRequest|'s
This is HTTP network and cache fetch.
Shouldn't it be HTTP fetch?

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:101: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
Please write FIXME. We should throw the caught error, not a TypeError.
I will add a method to ExceptionState in the future.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:115: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
ditto

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:129: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
ditto

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:143: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
ditto

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:78: ScriptPromise
fetch(ScriptState*, PassRefPtr<Request>);
Can you rename this function to another name (e.g. fetchInternal)?
Having fetch(ScriptState*, Request*) and fetch(ScriptState*,
PassRefPtr<Request>) is very error-prone.

https://codereview.chromium.org/399543002/diff/100001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:83: if (webResponse.status()
== 0) {
Can you write a comment here?

[horo, 2014-07-18 09:12:18]

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1317: if (!canRequest(resource->type(),
response.url(), resource->options(), false,
FetchRequest::UseDefaultOriginRestrictionForType)) {
On 2014/07/16 13:12:26, yhirano wrote:
> Is this block needed because of the same reason as
> DocumentThreadableLoader.cpp:334? If so, can you write a comment?

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:345: if
(m_options.crossOriginRequestPolicy == UseAccessControl) {
On 2014/07/16 13:12:27, yhirano wrote:
> This block is very similar to one from L355.
> The only difference is this block checks securityOrigin()->canRequest(url) (in
> isAllowedRedirect).
> If you can add the check for securityOrigin()->canRequest(url) at L355, we can
> merge the two blocks.

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:213: if
(m_request->unsafeRequestFlag()
On 2014/07/16 13:12:27, yhirano wrote:
> Can you merge this block into the above one?

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:284: // "1. Let |HTTPRequest| be
a copy of |request|, except that |HTTPRequest|'s
On 2014/07/16 13:12:27, yhirano wrote:
> This is HTTP network and cache fetch.
> Shouldn't it be HTTP fetch?

CORS preflight fetch procedure is implemented inside DocumentThreadableLoader.
So, I think it is ok to name this method as performHTTPFetch().

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:101: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
On 2014/07/16 13:12:27, yhirano wrote:
> Please write FIXME. We should throw the caught error, not a TypeError.
> I will add a method to ExceptionState in the future.

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:115: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
On 2014/07/16 13:12:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:129: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
On 2014/07/16 13:12:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:143: return
ScriptPromise::reject(scriptState,
V8ThrowException::createTypeError(exceptionState.message(),
scriptState->isolate()));
On 2014/07/16 13:12:27, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/399543002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:78: ScriptPromise
fetch(ScriptState*, PassRefPtr<Request>);
On 2014/07/16 13:12:27, yhirano wrote:
> Can you rename this function to another name (e.g. fetchInternal)?
> Having fetch(ScriptState*, Request*) and fetch(ScriptState*,
> PassRefPtr<Request>) is very error-prone.

It should be deleted.
I deleted it.

https://codereview.chromium.org/399543002/diff/100001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeClientImpl.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeClientImpl.cpp:83: if (webResponse.status()
== 0) {
On 2014/07/16 13:12:27, yhirano wrote:
> Can you write a comment here?

Done.

[yhirano, 2014-07-23 05:17:14]

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/399543002/diff/100001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1317: if (!canRequest(resource->type(),
response.url(), resource->options(), false,
FetchRequest::UseDefaultOriginRestrictionForType)) {
On 2014/07/18 09:12:18, horo wrote:
> On 2014/07/16 13:12:26, yhirano wrote:
> > Is this block needed because of the same reason as
> > DocumentThreadableLoader.cpp:334? If so, can you write a comment?
> 
> Done.

Thanks, please wrap comments in 80 columns.

https://codereview.chromium.org/399543002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:491: 
trailing whitespaces

https://codereview.chromium.org/399543002/diff/160001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/160001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:336: if
(response.wasFetchedViaServiceWorker()) {
Just curious, why can't we use
this->resource()->resourceRequest().requestContext() ==
blink::WebURLRequest::RequestContextFetch

? What is the difference between them?

https://codereview.chromium.org/399543002/diff/160001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/160001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:274: // CORS preflight fetch
procedure is implemented inside DocumentThreadableLoader.
Please wrap the comment in 80 columns.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:32: if
(data.headers[i][0] == name) {
Please use '===' instead of '=='.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:500: if
(event.target.state != 'activated')
Please use '!==' instead of '!='.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:531: // The
message is sent from fetch-access-control-iframe.html.
Sorry I don't understand when we reach here. Can you explain?

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:534:
TEST_TARGETS[counter][2].forEach(function(checkFunc){
space before '{'

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:545: function
onWorkerMessage(e) {
What happens if onWorkerMessage is called after the last onMessage call in a
test?

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:553:
TEST_TARGETS[counter][1].forEach(function(checkFunc){
space before '{'

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:560: IFRAME_URL);
Shouldn't this be an origin, not a URL?

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

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:4:
port = message.port;
Please declare |port|.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:58:
if (params['headers'] == 'CUSTOM') {
Please use '===' instead of '=='.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:93:
fetch(request).then(function(res){
space before '{'

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:101:
}).catch(function(e){
space before '{'

[horo, 2014-07-23 09:05:53]

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:32: if
(data.headers[i][0] == name) {
On 2014/07/23 05:17:13, yhirano wrote:
> Please use '===' instead of '=='.

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:500: if
(event.target.state != 'activated')
On 2014/07/23 05:17:13, yhirano wrote:
> Please use '!==' instead of '!='.

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:531: // The
message is sent from fetch-access-control-iframe.html.
On 2014/07/23 05:17:13, yhirano wrote:
> Sorry I don't understand when we reach here. Can you explain?

Done.
Added comment.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:534:
TEST_TARGETS[counter][2].forEach(function(checkFunc){
On 2014/07/23 05:17:13, yhirano wrote:
> space before '{'

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:545: function
onWorkerMessage(e) {
On 2014/07/23 05:17:13, yhirano wrote:
> What happens if onWorkerMessage is called after the last onMessage call in a
> test?

Introduced 2 counters(iframeMessageCounter and workerMessageCounter).
And added workerMessageCounter == TEST_TARGETS.length check.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:553:
TEST_TARGETS[counter][1].forEach(function(checkFunc){
On 2014/07/23 05:17:13, yhirano wrote:
> space before '{'

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:560: IFRAME_URL);
On 2014/07/23 05:17:13, yhirano wrote:
> Shouldn't this be an origin, not a URL?

Done.

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

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:4:
port = message.port;
On 2014/07/23 05:17:13, yhirano wrote:
> Please declare |port|.

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:58:
if (params['headers'] == 'CUSTOM') {
On 2014/07/23 05:17:13, yhirano wrote:
> Please use '===' instead of '=='.

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:93:
fetch(request).then(function(res){
On 2014/07/23 05:17:13, yhirano wrote:
> space before '{'

Done.

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:101:
}).catch(function(e){
On 2014/07/23 05:17:13, yhirano wrote:
> space before '{'

Done.

[yhirano, 2014-07-24 04:07:39]

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:545: function
onWorkerMessage(e) {
On 2014/07/23 09:05:52, horo wrote:
> On 2014/07/23 05:17:13, yhirano wrote:
> > What happens if onWorkerMessage is called after the last onMessage call in a
> > test?
> 
> Introduced 2 counters(iframeMessageCounter and workerMessageCounter).
> And added workerMessageCounter == TEST_TARGETS.length check.

Managing two counters manually is hard to understand. Can you use Promise to
structure the each test execution?
You can use Promise.all to join two asynchronous operations.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:397:
[checkJsonpHeader.bind(this, 'Referer', IFRAME_URL)]],
The ServiceWorker doesn't make a fetch request when ignore=true is specified,
does it?

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:415: [BASE_URL +
'Auth&mode=no-cors&credentials=ommit',
s/ommit/omit/g

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:483:
test.step(function() {
Can you structure the test?
Especially, it will get much easier to read if you separate the setup phase
(registering the service worker, setting up credentials) from the test execution
phase.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:510: function
login1() {
We can setup credentials for localhost:8080 and 127.0.0.1:8080 uniformly if we
do so before registering serviceworker.

[horo, 2014-07-24 06:42:50]

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:545: function
onWorkerMessage(e) {
On 2014/07/24 04:07:38, yhirano wrote:
> On 2014/07/23 09:05:52, horo wrote:
> > On 2014/07/23 05:17:13, yhirano wrote:
> > > What happens if onWorkerMessage is called after the last onMessage call in
a
> > > test?
> > 
> > Introduced 2 counters(iframeMessageCounter and workerMessageCounter).
> > And added workerMessageCounter == TEST_TARGETS.length check.
> 
> Managing two counters manually is hard to understand. Can you use Promise to
> structure the each test execution?
> You can use Promise.all to join two asynchronous operations.

Done.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:397:
[checkJsonpHeader.bind(this, 'Referer', IFRAME_URL)]],
On 2014/07/24 04:07:38, yhirano wrote:
> The ServiceWorker doesn't make a fetch request when ignore=true is specified,
> does it?

When ignore=true is set, the ServiceWorker doesn't handle the fetch event.

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#handle-a-fetch
[[HandleFetch]] returns null in 17-1 because respondWithEntered is false.

http://fetch.spec.whatwg.org/#http-fetch
And so HTTP fetch step 3 will be executed in the iframe.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:415: [BASE_URL +
'Auth&mode=no-cors&credentials=ommit',
On 2014/07/24 04:07:38, yhirano wrote:
> s/ommit/omit/g

Done.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:483:
test.step(function() {
On 2014/07/24 04:07:38, yhirano wrote:
> Can you structure the test?
> Especially, it will get much easier to read if you separate the setup phase
> (registering the service worker, setting up credentials) from the test
execution
> phase.

Done.

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:510: function
login1() {
On 2014/07/24 04:07:38, yhirano wrote:
> We can setup credentials for localhost:8080 and 127.0.0.1:8080 uniformly if we
> do so before registering serviceworker. 

Done.

[yhirano, 2014-07-24 07:51:48]

lgtm

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:397:
[checkJsonpHeader.bind(this, 'Referer', IFRAME_URL)]],
On 2014/07/24 06:42:49, horo wrote:
> On 2014/07/24 04:07:38, yhirano wrote:
> > The ServiceWorker doesn't make a fetch request when ignore=true is
specified,
> > does it?
> 
> When ignore=true is set, the ServiceWorker doesn't handle the fetch event.
> 
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#handle-a-fetch
> [[HandleFetch]] returns null in 17-1 because respondWithEntered is false.
> 
> http://fetch.spec.whatwg.org/#http-fetch
> And so HTTP fetch step 3 will be executed in the iframe.

Thanks, I see. Maybe it would be good to have checkFetchResult.bind(undefined,
'ignore') ?

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:508:
service_worker_unregister_and_register(test, workerScript, SCOPE)
Promise.all([login1, login2]).then(service_worker_unregister_and_register... ?

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:528: var
readyFromWorkerRecieved = undefined;
s/Recieved/Received/g

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:568: if (counter
== TEST_TARGETS.length) {
Please use === instead of ==

[horo, 2014-07-24 08:26:54]

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/220001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:397:
[checkJsonpHeader.bind(this, 'Referer', IFRAME_URL)]],
On 2014/07/24 07:51:48, yhirano wrote:
> On 2014/07/24 06:42:49, horo wrote:
> > On 2014/07/24 04:07:38, yhirano wrote:
> > > The ServiceWorker doesn't make a fetch request when ignore=true is
> specified,
> > > does it?
> > 
> > When ignore=true is set, the ServiceWorker doesn't handle the fetch event.
> > 
> >
>
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#handle-a-fetch
> > [[HandleFetch]] returns null in 17-1 because respondWithEntered is false.
> > 
> > http://fetch.spec.whatwg.org/#http-fetch
> > And so HTTP fetch step 3 will be executed in the iframe.
> 
> Thanks, I see. Maybe it would be good to have checkFetchResult.bind(undefined,
> 'ignore') ?

Done.

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:508:
service_worker_unregister_and_register(test, workerScript, SCOPE)
On 2014/07/24 07:51:48, yhirano wrote:
> Promise.all([login1, login2]).then(service_worker_unregister_and_register... ?

Done.

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:528: var
readyFromWorkerRecieved = undefined;
On 2014/07/24 07:51:48, yhirano wrote:
> s/Recieved/Received/g

Done.

https://codereview.chromium.org/399543002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:568: if (counter
== TEST_TARGETS.length) {
On 2014/07/24 07:51:48, yhirano wrote:
> Please use === instead of ==

Done.

[horo, 2014-07-24 08:27:29]

falken@
Could you please review this?
Thank you.

[horo, 2014-07-24 13:27:17]

jochen@
Could you please review these files in this patch?
  Source/core/fetch/Resource.h
  Source/core/fetch/Resource.cpp
  Source/core/fetch/ResourceFetcher.cpp
  Source/core/loader/DocumentThreadableLoader.cpp
  Source/web/ServiceWorkerGlobalScopeClientImpl.cpp
Thank you.

[jochen (gone - plz use gerrit), 2014-07-24 14:37:26]

https://codereview.chromium.org/399543002/diff/280001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1306:
context().dispatchDidFail(m_documentLoader, resource->identifier(),
ResourceError(errorDomainBlinkInternal, 0, response.url().string(), "Original
url check of ServiceWorker fetched resource failed."));
the error message would leave me a bit confused about what actually went wrong.
Can we do better?

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:168: ||
(m_request->url().protocol() == "data" && m_request->sameOriginDataURLFlag())
protocolIsData()

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:169: ||
(m_request->url().protocol() == "about")) {
protocolIsAbout()

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:192: if
(m_request->url().protocol() != "http" && m_request->url().protocol() !=
"https") {
protocolIsInHTTPFamily()

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:286:
request.addHTTPHeaderField(AtomicString(list[i]->first),
AtomicString(list[i]->second));
will this copy the referrer header? If yes, we need to copy the policy as well

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:81: request->m_referrer =
m_referrer;
I assume this is a struct Referrer, right?

[horo, 2014-07-24 15:50:23]

https://codereview.chromium.org/399543002/diff/280001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1306:
context().dispatchDidFail(m_documentLoader, resource->identifier(),
ResourceError(errorDomainBlinkInternal, 0, response.url().string(), "Original
url check of ServiceWorker fetched resource failed."));
On 2014/07/24 14:37:26, jochen wrote:
> the error message would leave me a bit confused about what actually went
wrong.
> Can we do better?

Done.

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:168: ||
(m_request->url().protocol() == "data" && m_request->sameOriginDataURLFlag())
On 2014/07/24 14:37:26, jochen wrote:
> protocolIsData()

Done.

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:169: ||
(m_request->url().protocol() == "about")) {
On 2014/07/24 14:37:26, jochen wrote:
> protocolIsAbout()

Done.

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:192: if
(m_request->url().protocol() != "http" && m_request->url().protocol() !=
"https") {
On 2014/07/24 14:37:26, jochen wrote:
> protocolIsInHTTPFamily()

Done.

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:286:
request.addHTTPHeaderField(AtomicString(list[i]->first),
AtomicString(list[i]->second));
On 2014/07/24 14:37:26, jochen wrote:
> will this copy the referrer header? If yes, we need to copy the policy as well

No. There could not be a referrer header.
Because we can't append a referrer header to the request headers.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/399543002/diff/280001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:81: request->m_referrer =
m_referrer;
On 2014/07/24 14:37:26, jochen wrote:
> I assume this is a struct Referrer, right?

Yes, it is FetchRequestData::Referrer.

[jochen (gone - plz use gerrit), 2014-07-24 15:56:56]

lgtm

[falken, 2014-07-25 01:46:40]

Haven't looked at everything yet.

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:6: <body>
don't need <body>

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:19: };
Sorry I didn't see this review sooner. New layout tests should use the style
guide on our wiki:
https://sites.google.com/a/chromium.org/dev/blink/serviceworker/testing

In this case, I think you'd indent 2 spaces in the body. For functions that are
arguments, you'd indent 4 (2 space indent for body + 2 space indent for
wrapping).

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:510:
.then(test.step_func(function() {
in the new style guide we omit these step_func since the terminal catch takes
care of unhandled exceptions

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:512: }))
in the new style guide we indent these

https://codereview.chromium.org/399543002/diff/330001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/330001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:191: // FIXME: Support redirect
in Fech API.
s/Fech/Fetch. These two comments say the same thing, so I'd delete line 190.

[horo, 2014-07-25 02:24:49]

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:6: <body>
On 2014/07/25 01:46:40, falken wrote:
> don't need <body>

We need this.
Without this, document.body.appendChild() in with_iframe() fails.

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:19: };
On 2014/07/25 01:46:40, falken wrote:
> Sorry I didn't see this review sooner. New layout tests should use the style
> guide on our wiki:
> https://sites.google.com/a/chromium.org/dev/blink/serviceworker/testing
> 
> In this case, I think you'd indent 2 spaces in the body. For functions that
are
> arguments, you'd indent 4 (2 space indent for body + 2 space indent for
> wrapping).

Done.

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:510:
.then(test.step_func(function() {
On 2014/07/25 01:46:40, falken wrote:
> in the new style guide we omit these step_func since the terminal catch takes
> care of unhandled exceptions

Done.

https://codereview.chromium.org/399543002/diff/330001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:512: }))
On 2014/07/25 01:46:40, falken wrote:
> in the new style guide we indent these

Done.

https://codereview.chromium.org/399543002/diff/330001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/330001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:191: // FIXME: Support redirect
in Fech API.
On 2014/07/25 01:46:40, falken wrote:
> s/Fech/Fetch. These two comments say the same thing, so I'd delete line 190.

Done.

[falken, 2014-07-25 05:41:29]

basically looks good modulo style nits

The CL description says you will add tests, but it seems they are already added?

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:16: // Functions
to check the result of from the ServiceWorker.
s/of//?

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:19: };
(As I understand it) since this function is not an argument, we don't indent the
additional 2 spaces. I.e.,

var checkFetchResult = function (expected, url, data) {
  assert_equals(data.fetchResult, expected, url + ' should be ' + expected);
};

and

forEach(function(expected) {
    assert_equals(...);
  };

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:486:
with_iframe(origin + '/serviceworker/resources/fetch-access-control-login.html')
80 col

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:546: }
optional nit: empty line between functions may make it easier to read

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:551:
readyFromWorkerReceived();
2 space indent

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:580: }))
the indents throughout this Promise look wrong. this is should be indented
another two

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:582:
frameWindow.postMessage(
indent another 2

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:585: }));
indent another 2

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-iframe.html
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-iframe.html:19:
</script>
This is also a new file and should conform to new style guide (2 space indent)

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html:8:
}, false);
indent this by 2 space

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html:14:
}, false);
new style guide indents this line by 2 space

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

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:5:
var message = e.data;
this should just be 2 space indent, as it's not an argument

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:64:
var init =  getRequestInit(params);
nit: space after =

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:79:
.then(function(res) {
.then should be indented 2

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-access-control.php
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control.php:6:
header("Access-Control-Allow-Origin: " . $origins[$i], false);
this looks indented too much

https://codereview.chromium.org/399543002/diff/350001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/350001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:191: // FIXME: Support redirect
in Fetch API.
These two comments say the same thing.

https://codereview.chromium.org/399543002/diff/350001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/350001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:139: // "1. If |request|'s url
contains a Known HSTS Host, modify it perthe
s/perthe/per the/

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

i'm planning to add more tests such as CSP and canvas tainting in another CL.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-access-control.html (right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:16: // Functions
to check the result of from the ServiceWorker.
On 2014/07/25 05:41:29, falken wrote:
> s/of//?

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:19: };
On 2014/07/25 05:41:28, falken wrote:
> (As I understand it) since this function is not an argument, we don't indent
the
> additional 2 spaces. I.e.,
> 
> var checkFetchResult = function (expected, url, data) {
>   assert_equals(data.fetchResult, expected, url + ' should be ' + expected);
> };
> 
> and
> 
> forEach(function(expected) {
>     assert_equals(...);
>   };

Ah, I misunderstood.
Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:486:
with_iframe(origin + '/serviceworker/resources/fetch-access-control-login.html')
On 2014/07/25 05:41:28, falken wrote:
> 80 col

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:546: }
On 2014/07/25 05:41:28, falken wrote:
> optional nit: empty line between functions may make it easier to read

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:551:
readyFromWorkerReceived();
On 2014/07/25 05:41:28, falken wrote:
> 2 space indent

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:580: }))
On 2014/07/25 05:41:29, falken wrote:
> the indents throughout this Promise look wrong. this is should be indented
> another two

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:582:
frameWindow.postMessage(
On 2014/07/25 05:41:28, falken wrote:
> indent another 2

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-access-control.html:585: }));
On 2014/07/25 05:41:28, falken wrote:
> indent another 2

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-iframe.html
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-iframe.html:19:
</script>
On 2014/07/25 05:41:29, falken wrote:
> This is also a new file and should conform to new style guide (2 space indent)

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html:8:
}, false);
On 2014/07/25 05:41:29, falken wrote:
> indent this by 2 space

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-login.html:14:
}, false);
On 2014/07/25 05:41:29, falken wrote:
> new style guide indents this line by 2 space

Done.

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

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:5:
var message = e.data;
On 2014/07/25 05:41:29, falken wrote:
> this should just be 2 space indent, as it's not an argument

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:64:
var init =  getRequestInit(params);
On 2014/07/25 05:41:29, falken wrote:
> nit: space after =

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control-worker.js:79:
.then(function(res) {
On 2014/07/25 05:41:29, falken wrote:
> .then should be indented 2

Done.

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-access-control.php
(right):

https://codereview.chromium.org/399543002/diff/350001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-access-control.php:6:
header("Access-Control-Allow-Origin: " . $origins[$i], false);
On 2014/07/25 05:41:29, falken wrote:
> this looks indented too much

Done.

https://codereview.chromium.org/399543002/diff/350001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/399543002/diff/350001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:191: // FIXME: Support redirect
in Fetch API.
On 2014/07/25 05:41:29, falken wrote:
> These two comments say the same thing.

Done.

https://codereview.chromium.org/399543002/diff/350001/Source/modules/servicew...
File Source/modules/serviceworkers/FetchManager.cpp (right):

https://codereview.chromium.org/399543002/diff/350001/Source/modules/servicew...
Source/modules/serviceworkers/FetchManager.cpp:139: // "1. If |request|'s url
contains a Known HSTS Host, modify it perthe
On 2014/07/25 05:41:29, falken wrote:
> s/perthe/per the/

Done.

[falken, 2014-07-25 06:08:22]

thanks lgtm

[horo, 2014-07-25 06:09:24]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-07-25 06:10:12]

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

[commit-bot: I haz the power, 2014-07-25 07:17:19]

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/18894)

[commit-bot: I haz the power, 2014-07-25 08:00:46]

Change committed as 178904