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

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

[1/2] blink: https://codereview.chromium.org/587213003/
[2/2] chromium: https://codereview.chromium.org/588153002/

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

In the renederer process:
  blink::ResourceRequest::setFetchRequestMode()
 blink::ResourceRequest::m_fetchRequestMode
  content::WebURLLoaderImpl::Context::Start()
   blink::WebURLRequest::fetchRequestMode()
    GetFetchRequestMode()
 conetnt::RequestInfo::service_worker_request_mode

In the browser process:
 conetnt::RequestInfo::service_worker_request_mode
  content::ResourceDispatcherHostImpl::BeginRequest()
   content::ServiceWorkerRequestHandler::InitializeHandler()
    content::ServiceWorkerProviderHost::CreateRequestHandler()
     content::ServiceWorkerControlleeRequestHandler::ServiceWorkerControlleeRequestHandler()
 content::ServiceWorkerControlleeRequestHandler::request_mode_
  content::ServiceWorkerControlleeRequestHandler::MaybeCreateJob()
   content::ServiceWorkerURLRequestJob::ServiceWorkerURLRequestJob()
 content::ServiceWorkerURLRequestJob::request_mode_
  content::ServiceWorkerURLRequestJob::CreateFetchRequest()
 content::ServiceWorkerFetchRequest::mode

In the ServiceWorker process:
 content::ServiceWorkerFetchRequest::mode
  content::ServiceWorkerScriptContext::OnFetchEvent()
   GetBlinkFetchRequestMode()
   blink::WebServiceWorkerRequest::setMode()
 blink::WebServiceWorkerRequest::WebServiceWorkerRequestPrivate::m_mode
  blink::ServiceWorkerGlobalScopeProxy::dispatchFetchEvent()
   blilnk::RespondWithObserver::create()
 blilnk::RespondWithObserver::m_requestMode

 blink::WebServiceWorkerRequest::WebServiceWorkerRequestPrivate::m_mode
  blink::Request::create()
  blink::FetchRequestData::create()
 blink::FetchRequestData::m_mode

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

[horo, 2014-09-22 10:38:04]

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

[horo, 2014-09-22 10:38:04]

yhirano@
Could you please review?

[yhirano, 2014-09-25 02:08:44]

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

https://codereview.chromium.org/587213003/diff/1/public/platform/WebURLReques...
public/platform/WebURLRequest.h:113: enum ServiceWorkerRequestMode {
Is this name appropriate? Having FetchRequestMode and skipServiceWorkerFlag
decoupled makes more sense to me.

[horo, 2014-09-25 03:55:18]

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

https://codereview.chromium.org/587213003/diff/1/public/platform/WebURLReques...
public/platform/WebURLRequest.h:113: enum ServiceWorkerRequestMode {
On 2014/09/25 02:08:44, yhirano wrote:
> Is this name appropriate? Having FetchRequestMode and skipServiceWorkerFlag
> decoupled makes more sense to me.

Done.

[horo, 2014-09-25 07:46:43]

Patchset #4 (id:60001) has been deleted

[yhirano, 2014-09-25 11:45:03]

lgtm

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

https://codereview.chromium.org/587213003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/RespondWithObserver.cpp:118: // "If either
|response|'s type is |opaque| and |request|'s mode is not |no CORS| or
|response|'s type is |error|, return a network error.
Please wrap the comment in 80 columns.

[horo, 2014-09-25 11:51:40]

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

https://codereview.chromium.org/587213003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/RespondWithObserver.cpp:118: // "If either
|response|'s type is |opaque| and |request|'s mode is not |no CORS| or
|response|'s type is |error|, return a network error.
On 2014/09/25 11:45:03, yhirano wrote:
> Please wrap the comment in 80 columns.

Done.

[Mike West, 2014-09-25 12:35:52]

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

[Mike West, 2014-09-25 12:35:53]

LGTM with a few nits. You'll still need a platform OWNER to take a closer look,
but I hope this speeds things up a bit.

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:45:
request->setMode(FetchRequestData::SameOriginMode);
I think FrameRequestData should simply share the new platform-level enum. You
don't need to do that refactoring in this CL, but I think it's the right
direction to go.

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/RespondWithObserver.cpp:119: // |no CORS| or
|response|'s type is |error|, return a network error."
Nit: This comment doesn't add much value, it pretty much repeats the condition
in the if. I'd suggest dropping it.

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Response.h:39: const FetchResponseData* response()
{ return m_response; }
This getter should be const.

https://codereview.chromium.org/587213003/diff/100001/Source/platform/exporte...
File Source/platform/exported/WebServiceWorkerRequest.cpp (right):

https://codereview.chromium.org/587213003/diff/100001/Source/platform/exporte...
Source/platform/exported/WebServiceWorkerRequest.cpp:17: :
m_mode(WebURLRequest::FetchRequestModeNoCORS), m_isReload(false) { }
Nit: I guess this is clang format's decision? :)

[horo, 2014-09-25 14:02:17]

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/FetchRequestData.cpp:45:
request->setMode(FetchRequestData::SameOriginMode);
On 2014/09/25 12:35:53, Mike West wrote:
> I think FrameRequestData should simply share the new platform-level enum. You
> don't need to do that refactoring in this CL, but I think it's the right
> direction to go.

Acknowledged.

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/RespondWithObserver.cpp:119: // |no CORS| or
|response|'s type is |error|, return a network error."
On 2014/09/25 12:35:53, Mike West wrote:
> Nit: This comment doesn't add much value, it pretty much repeats the condition
> in the if. I'd suggest dropping it.

This is a copy from the spec of HTTP fetch.
https://fetch.spec.whatwg.org/#http-fetch
I think this comment is useful for reading the codes.

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

https://codereview.chromium.org/587213003/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Response.h:39: const FetchResponseData* response()
{ return m_response; }
On 2014/09/25 12:35:53, Mike West wrote:
> This getter should be const.

Done.

https://codereview.chromium.org/587213003/diff/100001/Source/platform/exporte...
File Source/platform/exported/WebServiceWorkerRequest.cpp (right):

https://codereview.chromium.org/587213003/diff/100001/Source/platform/exporte...
Source/platform/exported/WebServiceWorkerRequest.cpp:17: :
m_mode(WebURLRequest::FetchRequestModeNoCORS), m_isReload(false) { }
On 2014/09/25 12:35:53, Mike West wrote:
> Nit: I guess this is clang format's decision? :)

Done.

[horo, 2014-09-25 14:04:49]

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

[horo, 2014-09-25 14:04:49]

jochen@
Could you review Source/platform/*, Source/web/* and public/platform/*?
Thank you.

[jochen (gone - plz use gerrit), 2014-09-29 12:56:53]

lgtm

[jochen (gone - plz use gerrit), 2014-09-29 13:00:01]

actually, where is this going to be used? Why no tests?

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

On 2014/09/29 13:00:01, jochen wrote:
> actually, where is this going to be used? Why no tests?

I'm planing to use this flag in the constructor of DocumentThreadableLoader and
FetchRequest::setCrossOriginAccessControl().
This is a work in progress patch.
https://codereview.chromium.org/600393004/

[horo, 2014-09-30 02:17:57]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-30 02:19:28]

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

[commit-bot: I haz the power, 2014-09-30 04:42:14]

Committed patchset #7 (id:140001) as 182885