[ServiceWorker] Set FetchRequestMode and handle wasFetchedViaServiceWorker.

[ServiceWorker] Set FetchRequestMode and handle wasFetchedViaServiceWorker.

This cl sets FetchRequestMode in the renderer process.
These flags are used in the ServiceWorker.

This cl also includes the network fallback logic in the renderer process.
Currently the network fallback which is executed when FetchEvent.respondWith() is not called in the ServiceWorker is executed in the browser process.

But if we send a CORS fetch request the network fallback should be executed in the renderer process.
In such a case wasFetchedViaServiceWorker flag of the response from the browser process is set.
This cl adds the check of the flag and introduces the fallback logic in ResourceLoader and DocumentThreadableLoader.


This cl depends on 634813002, 629953003 and 635693003.

BUG=416371, 408507
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183502

[horo, 2014-10-06 07:13:33]

Patchset #1 (id:1) has been deleted

[horo, 2014-10-06 07:13:41]

Patchset #1 (id:20001) has been deleted

[horo, 2014-10-06 07:13:51]

Patchset #1 (id:40001) has been deleted

[horo, 2014-10-06 07:13:59]

Patchset #1 (id:60001) has been deleted

[horo, 2014-10-06 07:14:07]

Patchset #1 (id:80001) has been deleted

[horo, 2014-10-06 09:17:21]

Patchset #1 (id:100001) has been deleted

[horo, 2014-10-06 09:17:28]

Patchset #1 (id:120001) has been deleted

[horo, 2014-10-06 09:17:35]

Patchset #1 (id:140001) has been deleted

[horo, 2014-10-06 09:17:41]

Patchset #1 (id:160001) has been deleted

[horo, 2014-10-06 09:17:47]

Patchset #1 (id:180001) has been deleted

[horo, 2014-10-06 09:17:53]

Patchset #1 (id:200001) has been deleted

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

Patchset #1 (id:220001) has been deleted

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

Patchset #1 (id:240001) has been deleted

[horo, 2014-10-06 09:35:02]

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

[horo, 2014-10-06 09:35:03]

yhirano@
Could you please review this?

[yhirano, 2014-10-07 05:57:09]

On 2014/10/06 09:35:03, horo wrote:
> yhirano@
> Could you please review this?

In the description:
 - "This cl also include" should be "This cl also includes".
 - "But if we send CORS request" should be "But if we send a CORS request".
 - "In such case" should be "In such a case" or "In such cases".
 - "This cl adds the check of the flag and introduce" should be "This cl adds
the check of the flag and introduces"

PingLoader and BeaconLoader seem to use setAllowStoredCredentials. Don't we need
set FetchCredentialsMode there as well?

[horo, 2014-10-07 06:35:34]

On 2014/10/07 05:57:09, yhirano wrote:
> On 2014/10/06 09:35:03, horo wrote:
> > yhirano@
> > Could you please review this?
> 
> In the description:
>  - "This cl also include" should be "This cl also includes".
>  - "But if we send CORS request" should be "But if we send a CORS request".
>  - "In such case" should be "In such a case" or "In such cases".
>  - "This cl adds the check of the flag and introduce" should be "This cl adds
> the check of the flag and introduces"

Done. Thank you!

> PingLoader and BeaconLoader seem to use setAllowStoredCredentials. Don't we
need
> set FetchCredentialsMode there as well?

Added setFetchCredentialsMode() in PingLoader::PingLoader().

[yhirano, 2014-10-07 08:03:15]

https://codereview.chromium.org/600393004/diff/300001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/600393004/diff/300001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:67:
request.setAllowStoredCredentials(allowCredentials == AllowStoredCredentials);
Requiring to call two methods in all occurrences makes me sad, although I don't
have a great idea.

What do you think about removing these setters from ResourceRequest and adding
 - setFetchCredentialsToInclude()
 - setFetchCredentialsToOmit()
 - setFetchCredentialsToSameOrigin(bool)
instead, for example?

https://codereview.chromium.org/600393004/diff/300001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:68:
request.setFetchCredentialsMode(allowCredentials == AllowStoredCredentials ?
WebURLRequest::FetchCredentialsModeInclude :
WebURLRequest::FetchCredentialsModeSameOrigin);
This behavior (setting same-origin if allowCredentials is false) is specified on
the XHR spec, right? If it is true, is it appropriate to implement a XHR
behavior here?

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

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:103: if (m_async &&
!request.skipServiceWorker() &&
m_document.fetcher()->isControlledByServiceWorker()) {
Can you write a brief comment about this block?

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:103: if (m_async &&
!request.skipServiceWorker() &&
m_document.fetcher()->isControlledByServiceWorker()) {
why m_async is needed, is it a workaround?

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:108: ResourceRequest newRequest
= ResourceRequest(request);
ResourceRequest newRequest(request) is enough.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:109: if (options.preflightPolicy
== ForcePreflight)
Sorry I don't understand this flag manipulation. Where is it specified?

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:114: if
(resourceLoaderOptions.credentialsRequested == ClientRequestedCredentials)
Ditto

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:118: 
Shouldn't we check the credentials flag correctness?

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:392: if
(response.wasFallbackRequiredByServiceWorker() && m_fallbackRequest) {
What happens if wasFallbackRequiredByServiceWorker() is true but
m_fallbackRequest is null? It is correct to return the response?

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:469: OwnPtr<ResourceRequest>
fallbackRequest;
fallbackRequest(m_fallbackRequest.release());

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:474: }
makeCrossOriginAccessRequest expects that the policy is USE. Please assert that
here as well.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:132: // Holds the original request
for fallback of fetching via the ServiceWorker.
Please wrap this comment in 80 columns.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Ping...
File Source/core/loader/PingLoader.cpp (right):

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Ping...
Source/core/loader/PingLoader.cpp:149:
wrappedRequest.setFetchCredentialsMode(credentialsAllowed ==
AllowStoredCredentials ? WebURLRequest::FetchCredentialsModeInclude :
WebURLRequest::FetchCredentialsModeSameOrigin);
Can you tell me why same-origin is indicated when credentialsAllowed !=
AllowStoredCredentials?

[horo, 2014-10-08 02:33:24]

Patchset #4 (id:320001) has been deleted

[horo, 2014-10-08 02:34:56]

As I talked offline I changed this cl to focus only on setting FetchRequestMode.
It is because we need FetchRequestMode for P1 security issue
(http://crbug.com/416371).
I will create another cl which sets FetchCredentialsMode.

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

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:103: if (m_async &&
!request.skipServiceWorker() &&
m_document.fetcher()->isControlledByServiceWorker()) {
On 2014/10/07 08:03:14, yhirano wrote:
> why m_async is needed, is it a workaround?

ServiceWorker's onFetch event doesn't support sync requests.
http://crbug.com/413103

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:103: if (m_async &&
!request.skipServiceWorker() &&
m_document.fetcher()->isControlledByServiceWorker()) {
On 2014/10/07 08:03:15, yhirano wrote:
> Can you write a brief comment about this block?

Done.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:108: ResourceRequest newRequest
= ResourceRequest(request);
On 2014/10/07 08:03:15, yhirano wrote:
> ResourceRequest newRequest(request) is enough.

Done.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:109: if (options.preflightPolicy
== ForcePreflight)
On 2014/10/07 08:03:14, yhirano wrote:
> Sorry I don't understand this flag manipulation. Where is it specified?

Added the comment.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:114: if
(resourceLoaderOptions.credentialsRequested == ClientRequestedCredentials)
On 2014/10/07 08:03:14, yhirano wrote:
> Ditto

removed

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:118: 
On 2014/10/07 08:03:14, yhirano wrote:
> Shouldn't we check the credentials flag correctness?

removed setFetchCredentialsMode

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:392: if
(response.wasFallbackRequiredByServiceWorker() && m_fallbackRequest) {
On 2014/10/07 08:03:14, yhirano wrote:
> What happens if wasFallbackRequiredByServiceWorker() is true but
> m_fallbackRequest is null? It is correct to return the response?

m_fallbackRequest must not be null when wasFallbackRequiredByServiceWorker() is
true.
I changed to use ASSERT().

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:469: OwnPtr<ResourceRequest>
fallbackRequest;
On 2014/10/07 08:03:15, yhirano wrote:
> fallbackRequest(m_fallbackRequest.release());

Done.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:474: }
On 2014/10/07 08:03:15, yhirano wrote:
> makeCrossOriginAccessRequest expects that the policy is USE. Please assert
that
> here as well.

Done.

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/600393004/diff/300001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:132: // Holds the original request
for fallback of fetching via the ServiceWorker.
On 2014/10/07 08:03:15, yhirano wrote:
> Please wrap this comment in 80 columns.

Done.

[yhirano, 2014-10-08 05:28:55]

https://codereview.chromium.org/600393004/diff/340001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/600393004/diff/340001/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoader.cpp:153: m_fallbackRequest = adoptPtr(new
ResourceRequest(m_request));
Can you tell me why we have to have the fallback logic here, in addition to
DocumentThreadableLoader?

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

https://codereview.chromium.org/600393004/diff/340001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:116: if (options.preflightPolicy
== ForcePreflight)
I still don't understand why this flag manipulation is valid. Imagine window has
fetch function. It can issue a request with mode = same-origin. Why do we ignore
the mode here?

[horo, 2014-10-08 06:49:40]

Patchset #4 (id:340001) has been deleted

[horo, 2014-10-08 06:50:09]

Patchset #4 (id:360001) has been deleted

[horo, 2014-10-08 06:52:15]

Ah. Sorry I deleted patchset 340001 by mistake.

On 2014/10/08 05:28:55, yhirano wrote:
>
https://codereview.chromium.org/600393004/diff/340001/Source/core/fetch/Resou...
> File Source/core/fetch/ResourceLoader.cpp (right):
> 
>
https://codereview.chromium.org/600393004/diff/340001/Source/core/fetch/Resou...
> Source/core/fetch/ResourceLoader.cpp:153: m_fallbackRequest = adoptPtr(new
> ResourceRequest(m_request));
> Can you tell me why we have to have the fallback logic here, in addition to
> DocumentThreadableLoader?


Both DocumentThreadableLoader and ResourceLoader have their own CORS checking
logic.
When DocumentThreadableLoader calls ResourceLoader
responseNeedsAccessControlCheck() is false and the CORS checking logic in
ResourceLoader is not executed.
So we must have the fallback logic in both DocumentThreadableLoader and
ResourceLoader.


> 
>
https://codereview.chromium.org/600393004/diff/340001/Source/core/loader/Docu...
> File Source/core/loader/DocumentThreadableLoader.cpp (right):
> 
>
https://codereview.chromium.org/600393004/diff/340001/Source/core/loader/Docu...
> Source/core/loader/DocumentThreadableLoader.cpp:116: if
(options.preflightPolicy
> == ForcePreflight)
> I still don't understand why this flag manipulation is valid. Imagine window
has
> fetch function. It can issue a request with mode = same-origin. Why do we
ignore
> the mode here?

Added FIXME commment

[yhirano, 2014-10-08 09:31:39]

Please rewrite the title and the description.

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html (right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html:6: <body>
[optional] You can omit the tag.
http://dev.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...

Ditto for other tests

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html:45:
return new Promise(function(resolve, reject) {
You can write like this:

function create_serial_promise(test_cases) {
  var promise = Promise.resolve();
  test_cases.forEach(function(test_case) {
    promise = promise.then(function() {
      return create_test_promise(test_case[0], test_case[1], test_case[2]);
    });
  });
  return promise;
}

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html:183:
if (!TEST_CASES[i][3]) {
Why don't you run all test cases serially?

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-image-worker.js
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-request-image-worker.js:17:
mode: event.request.mode,
Maybe this will be used for the request mode transfer and can be removed from
this CL?

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-worker.js
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-worker.js:15:
mode: event.request.mode,
Maybe this will be used for the request mode transfer and can be removed from
this CL?

[horo, 2014-10-08 09:58:26]

Patchset #5 (id:400001) has been deleted

[horo, 2014-10-08 10:02:47]

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html (right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html:6: <body>
On 2014/10/08 09:31:39, yhirano wrote:
> [optional] You can omit the tag.
>
http://dev.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h...
> 
> Ditto for other tests

We need <body>.
Otherwise document.body.appendChild() in with_iframe() called from test_login()
fails.

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html:45:
return new Promise(function(resolve, reject) {
On 2014/10/08 09:31:39, yhirano wrote:
> You can write like this:
> 
> function create_serial_promise(test_cases) {
>   var promise = Promise.resolve();
>   test_cases.forEach(function(test_case) {
>     promise = promise.then(function() {
>       return create_test_promise(test_case[0], test_case[1], test_case[2]);
>     });
>   });
>   return promise;
> }

Done.

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-cors-xhr-iframe.html:183:
if (!TEST_CASES[i][3]) {
On 2014/10/08 09:31:39, yhirano wrote:
> Why don't you run all test cases serially?

To reduce the execution time.

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-request-image-worker.js
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-request-image-worker.js:17:
mode: event.request.mode,
On 2014/10/08 09:31:39, yhirano wrote:
> Maybe this will be used for the request mode transfer and can be removed from
> this CL?

This file is used from fetch-request-image.html.
This test is for the FetchEvent's request mode form the image tag which is set
in FetchRequest::setCrossOriginAccessControl()

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-worker.js
(right):

https://codereview.chromium.org/600393004/diff/380001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-worker.js:15:
mode: event.request.mode,
On 2014/10/08 09:31:39, yhirano wrote:
> Maybe this will be used for the request mode transfer and can be removed from
> this CL?

This is used from mode_test() in fetch-request-xhr-iframe.html.
This test checks that the all FetchEvent's request mode from XHR must be "cors".

[horo, 2014-10-08 10:12:13]

Patchset #6 (id:440001) has been deleted

[yhirano, 2014-10-09 03:00:18]

lgtm

[horo, 2014-10-09 03:07:03]

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

[horo, 2014-10-09 03:07:04]

tkent@
Could you please review Source/core/*?

[yhirano, 2014-10-09 03:22:19]

On 2014/10/09 03:07:04, horo wrote:
> tkent@
> Could you please review Source/core/*?

tkent@ seems unavailable.

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

On 2014/10/09 03:22:19, yhirano wrote:
> On 2014/10/09 03:07:04, horo wrote:
> > tkent@
> > Could you please review Source/core/*?
> 
> tkent@ seems unavailable.

Ah, thank you.

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

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

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

haraken@
Could you please review Source/core/*?

[haraken, 2014-10-09 04:30:45]

On 2014/10/09 03:24:28, horo wrote:
> haraken@
> Could you please review Source/core/*?

Hmm, I'm not familiar with the loader code. I can rubberstamp based on
hirano-san's LG, but do we probably want to get an approval from ServiceWorker
experts?

[horo, 2014-10-09 04:43:07]

On 2014/10/09 04:30:45, haraken wrote:
> On 2014/10/09 03:24:28, horo wrote:
> > haraken@
> > Could you please review Source/core/*?
> 
> Hmm, I'm not familiar with the loader code. I can rubberstamp based on
> hirano-san's LG, but do we probably want to get an approval from ServiceWorker
> experts?

OK.

[horo, 2014-10-09 04:43:25]

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

[horo, 2014-10-09 04:43:26]

falken@
Could you please review?

[falken, 2014-10-09 05:38:34]

Does this CL mean the fallback to network code on the browser-side is no longer
needed?

https://codereview.chromium.org/600393004/diff/460001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/600393004/diff/460001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.h:156: virtual bool
isControlledByServiceWorker() override;
can this be const?

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

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:105: // will be handled by the
ServiceWorker. In such a case FetchRequestMode of
nit: Much of the first sentence just repeats the code. I would revise to "If the
fetch request will be handled by a Service Worker, the FetchRequestMode of
request must be set to FetchRequestModeCORS or
FetchRequestModeCORSWithForcedPreflight. Otherwise..."

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:108: // return the opaque
responce which is from the other origin site and the
nit: "return the" -> "return an", "responce" -> "response"

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:396: bool isCrossOriginResponse
= false;
It looks like isCrossOriginResponse is no longer useful, and the comment above
it can be deleted.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:96: void loadFallbackRequest();
This is a pretty general name for a specific Service Worker thing. Maybe
"loadFallbackRequestForServiceWorker"

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:133: // ServiceWorker.
Maybe, "Holds the original request for fallback in case the Service Worker does
not respond" is more clear.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:134: OwnPtr<ResourceRequest>
m_fallbackRequest;
As above, "m_fallbackRequestForServiceWorker" may be a better name.

[horo, 2014-10-09 07:27:53]

> Does this CL mean the fallback to network code on the browser-side is no
longer
> needed?
No. When the request mode is FETCH_REQUEST_MODE_SAME_ORIGIN or
FETCH_REQUEST_MODE_NO_CORS we can fallback to the network in the browser
process.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

https://codereview.chromium.org/600393004/diff/460001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.h (right):

https://codereview.chromium.org/600393004/diff/460001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.h:156: virtual bool
isControlledByServiceWorker() override;
On 2014/10/09 05:38:34, falken wrote:
> can this be const?

It can't be now because DocumentState::FromDataSource()'s argument is non const
WebDataSource*.
 
I think it should be changed to const pointer.
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

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

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:105: // will be handled by the
ServiceWorker. In such a case FetchRequestMode of
On 2014/10/09 05:38:34, falken wrote:
> nit: Much of the first sentence just repeats the code. I would revise to "If
the
> fetch request will be handled by a Service Worker, the FetchRequestMode of
> request must be set to FetchRequestModeCORS or
> FetchRequestModeCORSWithForcedPreflight. Otherwise..."

Done.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:108: // return the opaque
responce which is from the other origin site and the
On 2014/10/09 05:38:34, falken wrote:
> nit: "return the" -> "return an", "responce" -> "response"

Done.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.cpp:396: bool isCrossOriginResponse
= false;
On 2014/10/09 05:38:34, falken wrote:
> It looks like isCrossOriginResponse is no longer useful, and the comment above
> it can be deleted.

Done.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:96: void loadFallbackRequest();
On 2014/10/09 05:38:34, falken wrote:
> This is a pretty general name for a specific Service Worker thing. Maybe
> "loadFallbackRequestForServiceWorker"

Done.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:133: // ServiceWorker.
On 2014/10/09 05:38:34, falken wrote:
> Maybe, "Holds the original request for fallback in case the Service Worker
does
> not respond" is more clear.

Done.

https://codereview.chromium.org/600393004/diff/460001/Source/core/loader/Docu...
Source/core/loader/DocumentThreadableLoader.h:134: OwnPtr<ResourceRequest>
m_fallbackRequest;
On 2014/10/09 05:38:34, falken wrote:
> As above, "m_fallbackRequestForServiceWorker" may be a better name.

Done.

[falken, 2014-10-09 07:56:51]

lgtm. i skimmed over the longer tests as yhirano reviewed

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html (right):

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html:19: return 
service_worker_unregister_and_register(t, SCRIPT, SCOPE);
nit: extra space after return

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html (right):

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html:32:
assert_equals(e.data.results, 'finish');
delete the frame here

[horo, 2014-10-09 08:12:28]

Thank you!

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html (right):

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-canvas-tainting.html:19: return 
service_worker_unregister_and_register(t, SCRIPT, SCOPE);
On 2014/10/09 07:56:51, falken wrote:
> nit: extra space after return

Done.

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html (right):

https://codereview.chromium.org/600393004/diff/480001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-cors-xhr.html:32:
assert_equals(e.data.results, 'finish');
On 2014/10/09 07:56:51, falken wrote:
> delete the frame here

Done.

[horo, 2014-10-09 08:13:00]

haraken@
Could you please take a look?

[haraken, 2014-10-09 08:24:17]

LGTM given the LG of falken and hirano-san.

[horo, 2014-10-09 08:32:07]

On 2014/10/09 08:24:17, haraken wrote:
> LGTM given the LG of falken and hirano-san.

Thank you!

[horo, 2014-10-09 08:32:15]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-09 08:32:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/500001

[commit-bot: I haz the power, 2014-10-09 09:09:32]

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

[commit-bot: I haz the power, 2014-10-09 09:09:34]

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

[horo, 2014-10-09 09:11:08]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-09 09:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/500001

[commit-bot: I haz the power, 2014-10-09 10:31:54]

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

[commit-bot: I haz the power, 2014-10-09 10:31:55]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[horo, 2014-10-09 14:56:41]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-09 14:57:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/520001

[commit-bot: I haz the power, 2014-10-09 15:06:49]

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

[commit-bot: I haz the power, 2014-10-09 15:06:51]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[horo, 2014-10-09 15:26:42]

Patchset #9 (id:520001) has been deleted

[horo, 2014-10-09 15:29:11]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-09 15:29:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/540001

[commit-bot: I haz the power, 2014-10-09 16:31:01]

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

[commit-bot: I haz the power, 2014-10-09 16:31:02]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[horo, 2014-10-09 20:45:19]

Patchset #9 (id:540001) has been deleted

[horo, 2014-10-09 20:45:26]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-09 20:46:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/620001

[commit-bot: I haz the power, 2014-10-09 21:56:15]

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

[commit-bot: I haz the power, 2014-10-09 21:56:17]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[horo, 2014-10-10 00:30:19]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-10 00:32:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/820001

[horo, 2014-10-10 02:58:42]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-10-10 02:58:44]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-10 02:58:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/600393004/820001

[commit-bot: I haz the power, 2014-10-10 03:51:16]

Committed patchset #10 (id:820001) as 183502