Implement dumb URLLoader{Factory} for ServiceWorker script loading

Implement dumb URLLoader{Factory} for ServiceWorker script loading

Currently it only proxies request/response to/from the network factory,
plus does necessary HttpResponseInfo passing.

The plan is to use this URLLoader for script loading/caching during
installation. (Currently this always sets-up the URLLoader{Factory}
if servicification is enabled because we need it for correctly setting
HttpResponseInfo of the main resource of ServiceWorker)

(Patch on top of https://codereview.chromium.org/2779763004/
Try: https://codereview.chromium.org/2936623002/)

BUG=715640
Review-Url: https://codereview.chromium.org/2943463002
Cr-Commit-Position: refs/heads/master@{#481477}
Committed: https://chromium.googlesource.com/chromium/src/+/e7ab19876d2a092ef09f584619ea82890e89050e

[kinuko, 2017-06-15 08:41:49]

Description was changed from

==========
Implement dumb URLLoader{Factory} for ServiceWorker script loading

Currently it only proxies request/response to/from the network factory,
plus does necessary HttpResponseInfo passing.

The plan is to use this URLLoader for script loading/caching during
installation. (Currently this always sets-up the URLLoader{Factory}
if servicification is enabled because we need it for correctly setting
HttpResponseInfo of the main resource of ServiceWorker)

BUG=715640
==========

to

==========
Implement dumb URLLoader{Factory} for ServiceWorker script loading

Currently it only proxies request/response to/from the network factory,
plus does necessary HttpResponseInfo passing.

The plan is to use this URLLoader for script loading/caching during
installation. (Currently this always sets-up the URLLoader{Factory}
if servicification is enabled because we need it for correctly setting
HttpResponseInfo of the main resource of ServiceWorker)

(Patch on top of https://codereview.chromium.org/2779763004/
Try: https://codereview.chromium.org/2936623002/)

BUG=715640
==========

[kinuko, 2017-06-15 08:41:49]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org, shimazu@chromium.org

[kinuko, 2017-06-15 08:42:05]

PTL

[falken, 2017-06-16 04:24:56]

lgtm

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:104: //
eventually). For now this is just a proxy loader for the network loader.
It's a little hard to grok the parenthetical comment.

I think it means it's used for all script loading, but eventually it will only
be used for script loading during installation.

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:113: const
ResourceRequest& url_request,
Slightly nicer to say script_url instead of url_request?

Adds some more meaning beyond the generic CreateLoaderAndStart names.

I don't understand routing_id and options but I suppose users of
CreateLoaderAndStart are used to them.

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:222: const
ResourceRequest& url_request,
script_url?

[falken, 2017-06-16 04:26:54]

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:113: const
ResourceRequest& url_request,
On 2017/06/16 04:24:56, falken wrote:
> Slightly nicer to say script_url instead of url_request?
> 
> Adds some more meaning beyond the generic CreateLoaderAndStart names.
> 
> I don't understand routing_id and options but I suppose users of
> CreateLoaderAndStart are used to them.

Ah, maybe not nicer. A ResourceRequest is not a URL, and script_request looks
more confusing.

[shimazu, 2017-06-16 04:45:07]

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:863:
mojom::URLLoaderFactoryAssociatedPtr loader_factory_ptr;
I think you can use mojom::URLLoaderFactoryAssociatedPtrInfo directly here.

https://codereview.chromium.org/2943463002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_provider.mojom (right):

https://codereview.chromium.org/2943463002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_provider.mojom:25: associated
URLLoaderFactory script_loader_factory_ptr_info;
Should it be associated with EmbeddedWorkerInstanceClient? 
I assume it's fine to use an isolated pipe because the loader's IPCs are
originally async and other logics don't rely on the order of IPCs.

// I'm not strongly confident of it, so sorry if I missed something!

[kinuko, 2017-06-16 09:15:16]

Thanks! Updated.

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:104: //
eventually). For now this is just a proxy loader for the network loader.
On 2017/06/16 04:24:56, falken wrote:
> It's a little hard to grok the parenthetical comment.
> 
> I think it means it's used for all script loading, but eventually it will only
> be used for script loading during installation.

Yep, rephrased it a bit to make it a little clearer.

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:113: const
ResourceRequest& url_request,
On 2017/06/16 04:26:54, falken wrote:
> On 2017/06/16 04:24:56, falken wrote:
> > Slightly nicer to say script_url instead of url_request?
> > 
> > Adds some more meaning beyond the generic CreateLoaderAndStart names.
> > 
> > I don't understand routing_id and options but I suppose users of
> > CreateLoaderAndStart are used to them.
> 
> Ah, maybe not nicer. A ResourceRequest is not a URL, and script_request looks
> more confusing.

Ack. (I renamed it to resource_request just to clarify it's ResourceRequest)

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:222: const
ResourceRequest& url_request,
On 2017/06/16 04:24:56, falken wrote:
> script_url?

Renamed it to resource_request.

https://codereview.chromium.org/2943463002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:863:
mojom::URLLoaderFactoryAssociatedPtr loader_factory_ptr;
On 2017/06/16 04:45:07, shimazu wrote:
> I think you can use mojom::URLLoaderFactoryAssociatedPtrInfo directly here.

Cool, done.

https://codereview.chromium.org/2943463002/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_provider.mojom (right):

https://codereview.chromium.org/2943463002/diff/20001/content/common/service_...
content/common/service_worker/service_worker_provider.mojom:25: associated
URLLoaderFactory script_loader_factory_ptr_info;
On 2017/06/16 04:45:07, shimazu wrote:
> Should it be associated with EmbeddedWorkerInstanceClient? 
> I assume it's fine to use an isolated pipe because the loader's IPCs are
> originally async and other logics don't rely on the order of IPCs.
> 
> // I'm not strongly confident of it, so sorry if I missed something!

At some point I think we can make this independent, but for now it seems it'd be
safer to keep ordering with, say, OnScriptLoaded etc?

[kinuko, 2017-06-16 09:16:06]

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

[kinuko, 2017-06-16 09:16:38]

+dcheng@ for .mojom review

[dcheng, 2017-06-17 09:37:48]

ipc lgtm

just to understand correctly, this is intended to eventually replace the current
hooks that service worker uses in the //net layer? it would be good to mention
the reason for doing this in the CL description and/or the comments for the
URLLoader implementation.

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:244:
base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
Nit: either pass by const ref or std::move() below.

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_url_loader_job.cc (right):

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_url_loader_job.cc:186:
mojom::DownloadedTempFilePtr());
Nit: this is pre-existing code, but consider writing 'nullptr' here.

[shimazu, 2017-06-19 02:01:34]

lgtm

[kinuko, 2017-06-21 08:35:23]

Thanks!  Addressed comments, rebased & also added comments about the plan (i.e.
this code should be replacing the old URLRequestJob code path).

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:244:
base::WeakPtr<storage::BlobStorageContext> blob_storage_context,
On 2017/06/17 09:37:48, dcheng wrote:
> Nit: either pass by const ref or std::move() below.

Done.

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_url_loader_job.cc (right):

https://codereview.chromium.org/2943463002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_url_loader_job.cc:186:
mojom::DownloadedTempFilePtr());
On 2017/06/17 09:37:48, dcheng wrote:
> Nit: this is pre-existing code, but consider writing 'nullptr' here.

Done.

[kinuko, 2017-06-21 08:35:40]

The CQ bit was checked by kinuko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 08:35:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 08:38:04]

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

[commit-bot: I haz the power, 2017-06-21 08:38:04]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kinuko, 2017-06-22 06:22:28]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-06-22 06:22:29]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
shimazu@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2943463002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2017-06-22 06:22:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 08:02:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498112548677370,
"parent_rev": "3e097a99d8e07eed0197a0ff856e0e8cb988766b", "commit_rev":
"e7ab19876d2a092ef09f584619ea82890e89050e"}

[commit-bot: I haz the power, 2017-06-22 08:02:34]

Description was changed from

==========
Implement dumb URLLoader{Factory} for ServiceWorker script loading

Currently it only proxies request/response to/from the network factory,
plus does necessary HttpResponseInfo passing.

The plan is to use this URLLoader for script loading/caching during
installation. (Currently this always sets-up the URLLoader{Factory}
if servicification is enabled because we need it for correctly setting
HttpResponseInfo of the main resource of ServiceWorker)

(Patch on top of https://codereview.chromium.org/2779763004/
Try: https://codereview.chromium.org/2936623002/)

BUG=715640
==========

to

==========
Implement dumb URLLoader{Factory} for ServiceWorker script loading

Currently it only proxies request/response to/from the network factory,
plus does necessary HttpResponseInfo passing.

The plan is to use this URLLoader for script loading/caching during
installation. (Currently this always sets-up the URLLoader{Factory}
if servicification is enabled because we need it for correctly setting
HttpResponseInfo of the main resource of ServiceWorker)

(Patch on top of https://codereview.chromium.org/2779763004/
Try: https://codereview.chromium.org/2936623002/)

BUG=715640

Review-Url: https://codereview.chromium.org/2943463002
Cr-Commit-Position: refs/heads/master@{#481477}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7ab19876d2a092ef09f584619ea...
==========

[commit-bot: I haz the power, 2017-06-22 08:02:36]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e7ab19876d2a092ef09f584619ea...

[shimazu, 2017-06-27 04:13:12]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2959903002/ by shimazu@chromium.org.

The reason for reverting is: This patch depends on https://crrev.com/1ebf3cb
which is being reverted. 
See also the bug: https://crbug.com/736649.