[ServiceWorker] Plumbing the request credentials mode to the ServiceWorker. [1/2 blink]

[ServiceWorker] Plumbing the request credentials mode to the ServiceWorker. [1/2 blink]

Currently the credentials mode of the FetchEvent's request is always 'omit'.
But this should be 'same-origin' or 'include'.
ex:
- <img src="./test.png">
 mode: no CORS
 credentials mode: same-origin
- <img src="./test.png" crossOrigin="anonymous">
 mode: CORS
 credentials mode: same-origin
- <img src="./test.png" crossOrigin="use-credentials">
 mode: CORS
 credentials mode: include


[1/2] blink: https://codereview.chromium.org/610403002/
[2/2] chromium: https://codereview.chromium.org/615493003/

This value is passed from the renederer to the ServiceWorker in the following steps.

In the renederer process:
  blink::ResourceRequest::setFetchCredentialsMode()  // not called yet
 blink::ResourceRequest::m_fetchCredentialsMode
  content::WebURLLoaderImpl::Context::Start()
   blink::WebURLRequest::fetchCredentialsMode()
    GetFetchCredentialsMode()
 conetnt::RequestInfo::fetch_credentials_mode


In the browser process:
 conetnt::RequestInfo::fetch_credentials_mode
  content::ResourceDispatcherHostImpl::BeginRequest()
   content::ServiceWorkerRequestHandler::InitializeHandler()
    content::ServiceWorkerProviderHost::CreateRequestHandler()
     content::ServiceWorkerControlleeRequestHandler::ServiceWorkerControlleeRequestHandler()
 content::ServiceWorkerControlleeRequestHandler::request_credentials_mode_
  content::ServiceWorkerControlleeRequestHandler::MaybeCreateJob()
   content::ServiceWorkerURLRequestJob::ServiceWorkerURLRequestJob()
 content::ServiceWorkerURLRequestJob::request_credentials_mode_
  content::ServiceWorkerURLRequestJob::CreateFetchRequest()
 content::ServiceWorkerFetchRequest::credentials_mode

In the ServiceWorker process:
 content::ServiceWorkerFetchRequest::credentials_mode
   content::ServiceWorkerScriptContext::OnFetchEvent()
   GetBlinkFetchRequestCredentialsMode()
   blink::WebServiceWorkerRequest::setCredentialsMode()
 blink::WebServiceWorkerRequest::WebServiceWorkerRequestPrivate::m_credentialsMode
  blink::Request::create()
  blink::FetchRequestData::create()
 blink::FetchRequestData::m_credentials

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

[horo, 2014-09-29 11:20:33]

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

[horo, 2014-09-29 11:20:34]

yhirano@
Could you please review?

[yhirano, 2014-09-29 11:39:46]

On 2014/09/29 11:20:34, horo wrote:
> yhirano@
> Could you please review?

Can you please write why you need this change briefly in the CL description?

[horo, 2014-09-30 03:31:03]

On 2014/09/29 11:39:46, yhirano wrote:
> On 2014/09/29 11:20:34, horo wrote:
> > yhirano@
> > Could you please review?
> 
> Can you please write why you need this change briefly in the CL description?

done

[yhirano, 2014-09-30 05:18:52]

What you are doing in this series of CLs is restoring the original credential
mode from a credentials flag to access the mode in the service worker, right?
I'm not sure we can restore the credential mode for the general case - Isn't it
better to pass the credential mode directly instead of the credentials flag?

https://codereview.chromium.org/610403002/diff/1/public/platform/WebURLRequest.h
File public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/610403002/diff/1/public/platform/WebURLReques...
public/platform/WebURLRequest.h:105: // Corresponds to Fetch's "context frame
type": http://fetch.spec.whatwg.org/#concept-request-context-frame-type
Not related to this CL, but can you wrap the comment in 80 columns?

[horo, 2014-09-30 07:14:08]

> What you are doing in this series of CLs is restoring the original credential
> mode from a credentials flag to access the mode in the service worker, right?

Yes.

> I'm not sure we can restore the credential mode for the general case - Isn't
it
> better to pass the credential mode directly instead of the credentials flag?

Humm..
I think it is better than having two properties for the same purpose in
blink::ResourceRequest and conetnt::RequestInfo.

https://codereview.chromium.org/610403002/diff/1/public/platform/WebURLRequest.h
File public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/610403002/diff/1/public/platform/WebURLReques...
public/platform/WebURLRequest.h:105: // Corresponds to Fetch's "context frame
type": http://fetch.spec.whatwg.org/#concept-request-context-frame-type
On 2014/09/30 05:18:52, yhirano wrote:
> Not related to this CL, but can you wrap the comment in 80 columns?

Done.

[horo, 2014-09-30 07:20:43]

> > I'm not sure we can restore the credential mode for the general case - Isn't
> it
> > better to pass the credential mode directly instead of the credentials flag?
> 
> Humm..
> I think it is better than having two properties for the same purpose in
> blink::ResourceRequest and conetnt::RequestInfo.

Ah, as we talked offline, we should have credentialMode in
blink::ResourceRequest and conetnt::RequestInfo to support fetch() API in
Window.

[horo, 2014-09-30 09:22:09]

> I'm not sure we can restore the credential mode for the general case - Isn't
it
> better to pass the credential mode directly instead of the credentials flag?

Done.
Please review again.
Thank you.

[yhirano, 2014-10-01 04:54:18]

On 2014/09/30 09:22:09, horo wrote:
> > I'm not sure we can restore the credential mode for the general case - Isn't
> it
> > better to pass the credential mode directly instead of the credentials flag?
> 
> Done.
> Please review again.
> Thank you.

I think we should remove WebURLRequest::allowStoredCredentials and
ResourceLoaderOptions::allowCredentials in the future, right?
You don't have to start the work in this CL, but do you have a plan?

[horo, 2014-10-01 05:18:31]

On 2014/10/01 04:54:18, yhirano wrote:
> On 2014/09/30 09:22:09, horo wrote:
> > > I'm not sure we can restore the credential mode for the general case -
Isn't
> > it
> > > better to pass the credential mode directly instead of the credentials
flag?
> > 
> > Done.
> > Please review again.
> > Thank you.
> 
> I think we should remove WebURLRequest::allowStoredCredentials and
> ResourceLoaderOptions::allowCredentials in the future, right?
> You don't have to start the work in this CL, but do you have a plan?

No we can't remove allowStoredCredentials.
When the credentials_mode is same-origin, we can't know whether we should send
the credentials without this flag.

[yhirano, 2014-10-01 06:37:31]

On 2014/10/01 05:18:31, horo wrote:
> On 2014/10/01 04:54:18, yhirano wrote:
> > On 2014/09/30 09:22:09, horo wrote:
> > > > I'm not sure we can restore the credential mode for the general case -
> Isn't
> > > it
> > > > better to pass the credential mode directly instead of the credentials
> flag?
> > > 
> > > Done.
> > > Please review again.
> > > Thank you.
> > 
> > I think we should remove WebURLRequest::allowStoredCredentials and
> > ResourceLoaderOptions::allowCredentials in the future, right?
> > You don't have to start the work in this CL, but do you have a plan?
> 
> No we can't remove allowStoredCredentials.
> When the credentials_mode is same-origin, we can't know whether we should send
> the credentials without this flag.

I see, thanks.
I think we should set the credentials mode correctly in existing resource
requests.
You need to add a member to Request. It will be done in the future CL, right?

[horo, 2014-10-01 06:46:44]

> I think we should set the credentials mode correctly in existing resource
> requests.

Yes I will set it by calling ResourceRequest::setFetchCredentialsMode() in the
future CL.

> You need to add a member to Request. It will be done in the future CL, right?

Request::credentials() returns the FetchRequestData's m_credentials.
So when I will set ResourceRequest's FetchCredentialsMode in the page which is
controlled by the ServiceWorker, Request::credentials() will be changed in the
ServiceWorker.

[yhirano, 2014-10-01 08:00:12]

On 2014/10/01 06:46:44, horo wrote:
> > You need to add a member to Request. It will be done in the future CL,
right?
> 
> Request::credentials() returns the FetchRequestData's m_credentials.
> So when I will set ResourceRequest's FetchCredentialsMode in the page which is
> controlled by the ServiceWorker, Request::credentials() will be changed in the
> ServiceWorker.

Oh, I overlooked, sorry.
lgtm

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

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

[horo, 2014-10-01 08:02:16]

jochen@
Could you please review this?

[Mike West, 2014-10-01 08:16:33]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-01 08:16:34]

LGTM with nits.

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchRequestData.cpp:56: ASSERT_NOT_REACHED();
Why add the default? Isn't this switch an exhaustive list of FetchRequestMode's
values?

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchRequestData.cpp:70: ASSERT_NOT_REACHED();
Ditto.

https://codereview.chromium.org/610403002/diff/40001/Source/platform/network/...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/610403002/diff/40001/Source/platform/network/...
Source/platform/network/ResourceRequest.h:207: void
setFetchCredentialsMode(blink::WebURLRequest::FetchCredentialsMode mode)
Nit: Since Blink doesn't have an 80-column limit, and these are trivial methods,
can you just put these (and fetchRequestMode) on one line, like 'frameType'
above?

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
File public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
public/platform/WebURLRequest.h:68: //
http://fetch.spec.whatwg.org/#concept-request-context
Nit: Why change this?

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
public/platform/WebURLRequest.h:107: //
http://fetch.spec.whatwg.org/#concept-request-context-frame-type
Nit: And this?

[horo, 2014-10-01 08:32:22]

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchRequestData.cpp (right):

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchRequestData.cpp:56: ASSERT_NOT_REACHED();
On 2014/10/01 08:16:34, Mike West wrote:
> Why add the default? Isn't this switch an exhaustive list of
FetchRequestMode's
> values?

Removed

https://codereview.chromium.org/610403002/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchRequestData.cpp:70: ASSERT_NOT_REACHED();
On 2014/10/01 08:16:34, Mike West wrote:
> Ditto.


Removed

https://codereview.chromium.org/610403002/diff/40001/Source/platform/network/...
File Source/platform/network/ResourceRequest.h (right):

https://codereview.chromium.org/610403002/diff/40001/Source/platform/network/...
Source/platform/network/ResourceRequest.h:207: void
setFetchCredentialsMode(blink::WebURLRequest::FetchCredentialsMode mode)
On 2014/10/01 08:16:34, Mike West wrote:
> Nit: Since Blink doesn't have an 80-column limit, and these are trivial
methods,
> can you just put these (and fetchRequestMode) on one line, like 'frameType'
> above?

Done.

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
File public/platform/WebURLRequest.h (right):

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
public/platform/WebURLRequest.h:68: //
http://fetch.spec.whatwg.org/#concept-request-context
On 2014/10/01 08:16:34, Mike West wrote:
> Nit: Why change this?

Followed yhirano's comment in PS1.
But we don't need to wrap in 80 columns.
Reverted.

https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
public/platform/WebURLRequest.h:107: //
http://fetch.spec.whatwg.org/#concept-request-context-frame-type
On 2014/10/01 08:16:34, Mike West wrote:
> Nit: And this?

Done.

[Mike West, 2014-10-01 08:46:02]

On 2014/10/01 08:32:22, horo wrote:
>
https://codereview.chromium.org/610403002/diff/40001/public/platform/WebURLRe...
> public/platform/WebURLRequest.h:68: //
> http://fetch.spec.whatwg.org/#concept-request-context
> On 2014/10/01 08:16:34, Mike West wrote:
> > Nit: Why change this?
> 
> Followed yhirano's comment in PS1.
> But we don't need to wrap in 80 columns.
> Reverted.

Hah! Sorry, yhirano@, I didn't see your comment. :)

[jochen (gone - plz use gerrit), 2014-10-02 07:26:10]

lgtm

[horo, 2014-10-02 07:43:57]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-02 07:44:50]

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

[commit-bot: I haz the power, 2014-10-02 08:26:44]

Committed patchset #4 (id:20002) as 183122