Set the request mode and the credentials mode even if the request will not go to ServiceWorker.

Set the request mode and the credentials mode even if the request will not go to ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start that initializes the fetch request mode and fetch credentials mode of a request to outside the "is controlled by service worker" check.

BUG=610400
Committed: https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3
Cr-Commit-Position: refs/heads/master@{#394003}

[horo, 2016-05-12 07:17:08]

Description was changed from

==========
Set the request mode and the credentials mode even if the request willn't go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400
==========

to

==========
Set the request mode and the credentials mode even if the request will not go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400
==========

[horo, 2016-05-12 07:30:31]

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

[horo, 2016-05-12 07:30:31]

tyoshino@
Could you please review this?

[Marijn Kruisselbrink, 2016-05-12 17:30:38]

drive-by comment

https://codereview.chromium.org/1976513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1976513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:208: //
No-CORS requests are allowed only for those contexts when
The comment doesn't match the check. You're comment says (!skipServiceWorker &&
(context == audio || context == ....)), while the implementation instead says
that no-cors is allowed for all these contexts, and any other context if
skipServiceWorker is set.

Also I don't understand why no-cors would need to be okay for other contexts as
long as skipServiceWorker is true?

[horo, 2016-05-16 06:24:19]

https://codereview.chromium.org/1976513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1976513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:208: //
No-CORS requests are allowed only for those contexts when
On 2016/05/12 17:30:37, Marijn Kruisselbrink wrote:
> The comment doesn't match the check. You're comment says (!skipServiceWorker
&&
> (context == audio || context == ....)), while the implementation instead says
> that no-cors is allowed for all these contexts, and any other context if
> skipServiceWorker is set.

Updated comment.


> Also I don't understand why no-cors would need to be okay for other contexts
as
> long as skipServiceWorker is true?

Pepper plugins with private permission can send no-cors requests.
https://crrev.com/606993002
So we needs to allow the requests from these plugins.

[tyoshino (SeeGerritForStatus), 2016-05-16 07:12:42]

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp (right):

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp:324: KURL url =
toKURL("http://www.other.com/CrossOriginSuccess");
What's this change for?

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp:329: // default
context requests. So we set the context as Script here.
"request" is duplicated.

No-CORS is not allowed for the default context requests

or

No-CORS requests aren't allowed for the default context

?

[horo, 2016-05-16 07:23:15]

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp (right):

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp:324: KURL url =
toKURL("http://www.other.com/CrossOriginSuccess");
On 2016/05/16 07:12:42, tyoshino wrote:
> What's this change for?

I think it is weird to send the Script context request to ".html" URL.

https://codereview.chromium.org/1976513002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp:329: // default
context requests. So we set the context as Script here.
On 2016/05/16 07:12:42, tyoshino wrote:
> "request" is duplicated.
> 
> No-CORS is not allowed for the default context requests
> 
> or
> 
> No-CORS requests aren't allowed for the default context
> 
> ?

Done.

[tyoshino (SeeGerritForStatus), 2016-05-16 09:47:08]

lgtm

[horo, 2016-05-16 09:51:13]

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

[horo, 2016-05-16 09:51:14]

kinuko@
Could you please review third_party/WebKit/Source/core/loader/*?

[kinuko, 2016-05-16 09:59:16]

lgtm + nits

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:208: //
No-CORS is allowed for all these contexts, and plugin contexts
nit: "No-CORS requests are allowed for..." for consistency?

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:211:
SECURITY_CHECK(m_requestContext == WebURLRequest::RequestContextAudio ||
m_requestContext == WebURLRequest::RequestContextVideo || m_requestContext ==
WebURLRequest::RequestContextObject || m_requestContext ==
WebURLRequest::RequestContextFavicon || m_requestContext ==
WebURLRequest::RequestContextImage || m_requestContext ==
WebURLRequest::RequestContextScript || (request.skipServiceWorker() &&
m_requestContext == WebURLRequest::RequestContextPlugin));
nit: the line's going too long / too hard to read, might be better to start
considering breaking lines or maybe factor out as an internal routine that
returns true/false if cors's allowed

[horo, 2016-05-16 10:14:34]

Thank you.

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:208: //
No-CORS is allowed for all these contexts, and plugin contexts
On 2016/05/16 09:59:16, kinuko wrote:
> nit: "No-CORS requests are allowed for..." for consistency?

Done.

https://codereview.chromium.org/1976513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:211:
SECURITY_CHECK(m_requestContext == WebURLRequest::RequestContextAudio ||
m_requestContext == WebURLRequest::RequestContextVideo || m_requestContext ==
WebURLRequest::RequestContextObject || m_requestContext ==
WebURLRequest::RequestContextFavicon || m_requestContext ==
WebURLRequest::RequestContextImage || m_requestContext ==
WebURLRequest::RequestContextScript || (request.skipServiceWorker() &&
m_requestContext == WebURLRequest::RequestContextPlugin));
On 2016/05/16 09:59:16, kinuko wrote:
> nit: the line's going too long / too hard to read, might be better to start
> considering breaking lines or maybe factor out as an internal routine that
> returns true/false if cors's allowed

Introduced IsNoCORSAllowedContext().

[horo, 2016-05-16 10:15:23]

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

[horo, 2016-05-16 10:15:24]

haraken@
Could you please review
third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp?

[haraken, 2016-05-16 10:49:16]

On 2016/05/16 10:15:24, horo wrote:
> haraken@
> Could you please review
> third_party/WebKit/Source/web/AssociatedURLLoaderTest.cpp?

LGTM

[horo, 2016-05-16 10:52:02]

The CQ bit was checked by horo@chromium.org

[horo, 2016-05-16 10:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1976513002/#ps80001
(title: "introduce IsNoCORSAllowedContext")

[commit-bot: I haz the power, 2016-05-16 10:52:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976513002/80001

[commit-bot: I haz the power, 2016-05-16 15:19:52]

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

[commit-bot: I haz the power, 2016-05-16 15:19:53]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[horo, 2016-05-16 23:39:14]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-05-16 23:39:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1976513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1976513002/80001

[commit-bot: I haz the power, 2016-05-17 01:15:00]

Description was changed from

==========
Set the request mode and the credentials mode even if the request will not go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400
==========

to

==========
Set the request mode and the credentials mode even if the request will not go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400
==========

[commit-bot: I haz the power, 2016-05-17 01:15:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-17 01:16:18]

Description was changed from

==========
Set the request mode and the credentials mode even if the request will not go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400
==========

to

==========
Set the request mode and the credentials mode even if the request will not go to
ServiceWorker.

As mek@ mentioned in crbug.com/610400#c6, we should move the block in DTL::start
that initializes the fetch request mode and fetch credentials mode of a request
to outside the "is controlled by service worker" check.

BUG=610400

Committed: https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3
Cr-Commit-Position: refs/heads/master@{#394003}
==========

[commit-bot: I haz the power, 2016-05-17 01:16:19]

Patchset 5 (id:??) landed as
https://crrev.com/90bec6d5c3d8220e4e3f1265c0913894c1f4fcd3
Cr-Commit-Position: refs/heads/master@{#394003}