[ServiceWorker] Introduce skip_service_worker flag and remove LOAD_BYPASS_CACHE check.

[ServiceWorker] Introduce skip_service_worker flag and remove LOAD_BYPASS_CACHE check.

If the request is a force refresh (shift+refresh) the request should not be handled by the ServiceWorker.
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#navigator-service-worker-controller

Currently we are checking LOAD_BYPASS_CACHE flag of net::URLRequest in ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
But this flag is set not only when shift+refresh but also when devtool's "Disable cache" is checked.

To distinguish them, I added skipServiceWorker flag to blink::WebURLRequest and blink::ResourceRequest in https://codereview.chromium.org/527703002/.

This cl changes the follwings 
- Introduce skip_service_worker flag to content::RequestExtraData and ResourceHostMsg_Request.
- Remove the check of LOAD_BYPASS_CACHE in ServiceWorkerControlleeRequestHandler::MaybeCreateJob().
- Check skip_service_worker flag in ServiceWorkerRequestHandler::InitializeHandler() and call provider_host->SetDocumentUrl() when the request is main resource request.

TEST=none
BUG=402093
Committed: https://crrev.com/258fc42376fc25bb4fdfbc372ce53abe39988797
Cr-Commit-Position: refs/heads/master@{#293490}

[horo, 2014-09-01 05:27:15]

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

[horo, 2014-09-01 05:27:15]

nhiroki@
Could you please review this?

[nhiroki, 2014-09-01 05:58:23]

lgtm

https://codereview.chromium.org/527713002/diff/1/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/527713002/diff/1/net/base/load_flags_list.h#n...
net/base/load_flags_list.h:130: LOAD_FLAG(SKIP_SERVICE_WORKER, 1 << 29)
'BYPASS' seems to be more common word than 'SKIP' in the list.

[horo, 2014-09-01 06:15:47]

thank you!

https://codereview.chromium.org/527713002/diff/1/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/527713002/diff/1/net/base/load_flags_list.h#n...
net/base/load_flags_list.h:130: LOAD_FLAG(SKIP_SERVICE_WORKER, 1 << 29)
On 2014/09/01 05:58:22, nhiroki wrote:
> 'BYPASS' seems to be more common word than 'SKIP' in the list.

Done.

[horo, 2014-09-01 06:29:31]

horo@chromium.org changed reviewers:
+ brettw@chromium.org, mmenke@chromium.org

[horo, 2014-09-01 06:29:31]

brettw@chromium.org: 
Please review changes in content/child/web_url_loader_impl.cc

mmenke@chromium.org:
Please review changes in net/base/load_flags_list.h

Thank you.

[mmenke, 2014-09-01 13:01:22]

On 2014/09/01 06:29:31, horo wrote:
> mailto:brettw@chromium.org: 
> Please review changes in content/child/web_url_loader_impl.cc
> 
> mailto:mmenke@chromium.org:
> Please review changes in net/base/load_flags_list.h
> 
> Thank you.

This is a layering violation.  The network stack shouldn't know or care about
ServiceWorker.  URLRequests have URLRequestInfo, and inherit from UserData. 
Maybe use one of them to store this info?

[horo, 2014-09-03 06:06:24]

On 2014/09/01 13:01:22, mmenke wrote:
> On 2014/09/01 06:29:31, horo wrote:
> > mailto:brettw@chromium.org: 
> > Please review changes in content/child/web_url_loader_impl.cc
> > 
> > mailto:mmenke@chromium.org:
> > Please review changes in net/base/load_flags_list.h
> > 
> > Thank you.
> 
> This is a layering violation.  The network stack shouldn't know or care about
> ServiceWorker.  URLRequests have URLRequestInfo, and inherit from UserData. 
> Maybe use one of them to store this info?

Thank you for your review.

I noticed that we don't need to introduce SKIP_SERVICE_WORKER flag to
net::URLRequest to skip the ServiceWorker.
If we set provider_id of RequestExtraData to kInvalidServiceWorkerProviderId, we
can skip the ServiceWorker.

So I changed this cl to check request.skipServiceWorker() in
RenderFrameImpl::willSendRequest() and removed the check of LOAD_BYPASS_CACHE in
ServiceWorkerControlleeRequestHandler::MaybeCreateJob().

Please review patch set 3.
Thank you.

[nhiroki, 2014-09-03 07:33:55]

LGTM from service worker's point of view.

[mmenke, 2014-09-03 11:44:58]

On 2014/09/03 07:33:55, nhiroki wrote:
> LGTM from service worker's point of view.

This approach LGTM from net/ standpoint.  Not sufficiently familiar with the
code to check for correctness.

[horo, 2014-09-03 12:07:42]

brettw@

Could you please review this?

[michaeln, 2014-09-03 20:18:57]

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

[michaeln, 2014-09-03 20:18:58]

https://codereview.chromium.org/527713002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(left):

https://codereview.chromium.org/527713002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_controllee_request_handler.cc:60:
provider_host_->SetDocumentUrl(
this change puts the browser side into an odd unexpected state where it doesn't
know the document url of a ProviderHost object. i'm not sure this is the best
approach, i think this will regress an old bug.

https://code.google.com/p/chromium/issues/detail?id=392405
https://codereview.chromium.org/362183002

[horo, 2014-09-04 04:34:29]

On 2014/09/03 20:18:58, michaeln wrote:
>
https://codereview.chromium.org/527713002/diff/40001/content/browser/service_...
> File
content/browser/service_worker/service_worker_controllee_request_handler.cc
> (left):
> 
>
https://codereview.chromium.org/527713002/diff/40001/content/browser/service_...
>
content/browser/service_worker/service_worker_controllee_request_handler.cc:60:
> provider_host_->SetDocumentUrl(
> this change puts the browser side into an odd unexpected state where it
doesn't
> know the document url of a ProviderHost object. i'm not sure this is the best
> approach, i think this will regress an old bug.
> 
> https://code.google.com/p/chromium/issues/detail?id=392405
> https://codereview.chromium.org/362183002

Ah, yes. This change is wrong.
Thank you for pointing this.

I changed this cl to introduce skip_service_worker flag to
content::RequestExtraData and ResourceHostMsg_Request.
And move the call of provider_host->SetDocumentUrl() to
ServiceWorkerRequestHandler::InitializeHandler().

Could you please review patch set 4?

[michaeln1, 2014-09-04 19:44:47]

lgtm, thnx!

[michaeln, 2014-09-04 19:45:53]

and lgtm 2

[horo, 2014-09-04 23:20:12]

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

[horo, 2014-09-04 23:20:12]

nasko@
Could you please review content/common/resource_messages.h?

[horo, 2014-09-04 23:20:29]

horo@chromium.org changed reviewers:
- brettw@chromium.org

[horo, 2014-09-04 23:22:01]

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

[horo, 2014-09-04 23:22:02]

jochen@
Could you please review content/child/* and content/renderer/*?

[nasko, 2014-09-04 23:30:25]

IPC LGTM

[jochen (gone - plz use gerrit), 2014-09-05 08:31:22]

jochen@chromium.org changed reviewers:
+ michaeln@google.com

[jochen (gone - plz use gerrit), 2014-09-05 08:31:23]

lgtm

[horo, 2014-09-05 08:54:50]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-05 08:56:20]

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

[commit-bot: I haz the power, 2014-09-05 10:16:33]

Committed patchset #4 (id:60001) as ddb7698c0dfa651553e367fb5db64f06a1ce6ca7

[commit-bot: I haz the power, 2014-09-10 03:38:29]

Patchset 4 (id:??) landed as
https://crrev.com/258fc42376fc25bb4fdfbc372ce53abe39988797
Cr-Commit-Position: refs/heads/master@{#293490}