network service: Create URLLoader for service worker navigation case

network service: Create URLLoader for service worker navigation case

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png (i.e. only the navigation requests).
There's no handling for the renderer resource requests yet, nor is there
any handling of context-side requests.

BUG=715640

[scottmg, 2017-04-26 18:00:56]

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

[commit-bot: I haz the power, 2017-04-26 18:01:25]

Dry run: CQ is trying da patch.

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

[scottmg, 2017-04-26 18:01:37]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy

This proxy currently handles navigation requests. A subsequent patch
will connect it to the renderer when a service worker is registered for
the document, and the subsequent followups will delegate the request to
the provider, rather than simply proxying to the network service.

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

to

==========
network service: Create stub of service worker URLLoader proxy

This proxy currently handles navigation requests. A subsequent patch
will connect it to the renderer when a service worker is registered for
the document, and the subsequent followups will delegate the request to
the provider, rather than simply proxying to the network service.

This is "1." in https://bugs.chromium.org/p/chromium/issues/detail?id=715640#c3

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

[scottmg, 2017-04-26 18:01:37]

scottmg@chromium.org changed reviewers:
+ jam@chromium.org

[scottmg, 2017-04-26 18:02:23]

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

[scottmg, 2017-04-26 18:04:26]

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

[commit-bot: I haz the power, 2017-04-26 18:16:16]

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

[commit-bot: I haz the power, 2017-04-26 18:16:17]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[scottmg, 2017-04-26 18:32:02]

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

[commit-bot: I haz the power, 2017-04-26 18:33:30]

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-04-26 18:47:21]

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

[commit-bot: I haz the power, 2017-04-26 18:47:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[scottmg, 2017-04-26 18:50:59]

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

[commit-bot: I haz the power, 2017-04-26 18:51:40]

Dry run: CQ is trying da patch.

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

[scottmg, 2017-04-26 18:53:48]

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

[commit-bot: I haz the power, 2017-04-26 18:54:05]

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-04-26 19:06:07]

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

[commit-bot: I haz the power, 2017-04-26 19:06:08]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2017-04-26 19:08:12]

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

[commit-bot: I haz the power, 2017-04-26 19:09:24]

Dry run: CQ is trying da patch.

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

[scottmg, 2017-04-26 21:04:09]

jam, could you review?

kinuko, michaeln: fyi if I can pique your interest :) My rough current plan is
in the linked bug.

[commit-bot: I haz the power, 2017-04-26 21:12:04]

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

[commit-bot: I haz the power, 2017-04-26 21:12:05]

Dry run: This issue passed the CQ dry run.

[scottmg, 2017-04-27 00:22:51]

scottmg@chromium.org changed reviewers:
- jam@chromium.org, michaeln@chromium.org

[scottmg, 2017-04-27 00:22:51]

-> kinuko who somewhat volunteered to take a look. :)

[scottmg, 2017-04-27 00:25:37]

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

[commit-bot: I haz the power, 2017-04-27 00:26:26]

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-04-27 02:42:55]

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

[commit-bot: I haz the power, 2017-04-27 02:42:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2017-04-27 17:22:50]

On 2017/04/27 00:22:51, scottmg wrote:
> -> kinuko who somewhat volunteered to take a look. :)

(Once I got to attaching the subresource handler in the renderer, I realized
this needs some rework, as too much is happening in the factory creation. I'll
fix that up and post a revision in a bit.)

[kinuko, 2017-05-01 02:06:24]

Sorry for long delay, needed a few days before I could have more time. I think
this is looking good, some comments inline.

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_factory_creation_data.cc (right):

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
content/browser/loader/navigation_factory_creation_data.cc:10: : request(),
nit: I don't think we need these empty ctors here

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_factory_creation_data.h (right):

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
content/browser/loader/navigation_factory_creation_data.h:26:
ServiceWorkerNavigationHandleCore* navigation_handle;
navigation_handle -> service_worker_navigation_handle_core (yep it's long...)

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:227: // fall
back to connecting directly to the network service.
Initially this is probably ok, but we'll need network service version's
URLLoaderFactory in addition to the SW's one (that allows us to reuse more code
for tricky CORS cases)- could we put TODO here?

https://codereview.chromium.org/2843043002/diff/180001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:235: // Bind the
URLClient implementation to this object to pass to the URLLoader.
nit: URLClient -> URLLoaderClient

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
File
content/browser/service_worker/service_worker_url_loader_factory_creation.cc
(right):

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory_creation.cc:126:
ControlleeHandlerFactory(ServiceWorkerProviderHost* provider_host)
explicit

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory_creation.cc:157:
ServiceWorkerURLLoaderFactoryCreation::InitializeForNavigation(
would be good if we could reuse some of the code in
ServiceWorkerRequestHandler::InitForNavigation

[kinuko, 2017-05-01 15:07:57]

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
File
content/browser/service_worker/service_worker_url_loader_factory_creation.cc
(right):

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory_creation.cc:36:
ControlleeURLLoaderImpl(mojom::URLLoaderAssociatedRequest loader,
Someone needs to own this?

https://codereview.chromium.org/2843043002/diff/180001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory_creation.cc:182:
mojo::MakeRequest(&controllee));
I'm still not fully sure why we need this proxy class, here don't we need to
connect to Network provider if we couldn't find registration, or set up factory
request (and intermediate factory in the browser that just talks to the network
for now) in SWVersion if we find one?

[scottmg, 2017-05-03 00:11:02]

Patchset #12 (id:220001) has been deleted

[scottmg, 2017-05-04 20:10:36]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy

This proxy currently handles navigation requests. A subsequent patch
will connect it to the renderer when a service worker is registered for
the document, and the subsequent followups will delegate the request to
the provider, rather than simply proxying to the network service.

This is "1." in https://bugs.chromium.org/p/chromium/issues/detail?id=715640#c3

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

to

==========
network service: Create stub of service worker URLLoader proxy



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

[yzshen1, 2017-05-04 22:30:36]

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

[yzshen1, 2017-05-04 22:30:37]

https://codereview.chromium.org/2843043002/diff/360001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2843043002/diff/360001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:276:
url_loader_factory_ = std::move(g_direct_network_url_loader_factory.Get());
nit: This takes the g_direct_network_url_loader_factory and sets it to null, so
next time we will need to re-do the BindInterface() above.

This changes the meaning of OverrideURLLoaderFactoryForTesting(), it now only
overrides the next request, instead of all subsequent ones.

[scottmg, 2017-05-05 21:43:38]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy



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

to

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered service
worker.

http://i.imgur.com/ViO1q5u.png

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

[scottmg, 2017-05-05 21:50:53]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered service
worker.

http://i.imgur.com/ViO1q5u.png

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

to

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png. There's no handling for the renderer
resource requests yet, nor is there any handling of context-side
requests.

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

[scottmg, 2017-05-05 21:51:32]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png. There's no handling for the renderer
resource requests yet, nor is there any handling of context-side
requests.

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

to

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png (i.e. only the navigation requests).
There's no handling for the renderer resource requests yet, nor is there
any handling of context-side requests.

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

[falken, 2017-05-08 08:20:32]

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

[falken, 2017-05-08 08:20:33]

I just have a quick question. I think Kinuko is thinking about the overall
design more.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
(right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:177:
DCHECK(body_as_stream.is_null());
I'm not sure I understand the "SWFetchDispatcher only offers blobs for normal
fetches" comment.

SWFetchDispatcher can respond with a stream, in
ServiceWorkerFetchDispatcher::Complete it'll call the callback with the stream
response from the SW if available.

A stream response would happen if the service worker did
respondWith(fetch(some_url)) or (I think) respondWith(ReadableStream). A blob
response happens if the service worker responds with a response from Cache
Storage or generates a response manually like respondWith('hello world'))

[kinuko, 2017-05-08 14:55:48]

The screenshot looks very exciting!  And yeah I think we can start with
something like this.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:38:
class JobWrapper {
nit: make it an inner class of SWControlleeRequestHandler

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:250:
ServiceWorkerControlleeRequestHandler::MaybeCreateURLLoader(
MaybeGetURLLoaderFactory ...from what it does?

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:91:
on_created.Run(std::move(handler));
I think this method and InitializeForNavigationInternal can return SWReqHandler
and the caller (InitializeForNavigation*) can call either AttachToURLRequest or
CreateURLLoader, which is easier to follow?

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.h (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.h:164: static void
InitializeForNavigationImpl(
nit: s/Impl/Internal/

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_loader_factory.h (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory.h:15: class
ServiceWorkerURLLoaderFactory : public mojom::URLLoaderFactory {
Will we need this base class?

[scottmg, 2017-05-08 20:12:31]

Thanks to both of you for taking a look. I addressed all the comments, and added
one more small change to support triggering a fallback to the network. This was
necessary for loading the first navigation at startup as SWStorage defers the
first request until its data has been loaded, so the SWControlleeRequestHandler
isn't able to fallback to network before returning the initial URLLoaderFactory.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:38:
class JobWrapper {
On 2017/05/08 14:55:47, kinuko wrote:
> nit: make it an inner class of SWControlleeRequestHandler

Done.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:250:
ServiceWorkerControlleeRequestHandler::MaybeCreateURLLoader(
On 2017/05/08 14:55:47, kinuko wrote:
> MaybeGetURLLoaderFactory ...from what it does?

Done.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
(right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:177:
DCHECK(body_as_stream.is_null());
On 2017/05/08 08:20:33, falken wrote:
> I'm not sure I understand the "SWFetchDispatcher only offers blobs for normal
> fetches" comment.
> 
> SWFetchDispatcher can respond with a stream, in
> ServiceWorkerFetchDispatcher::Complete it'll call the callback with the stream
> response from the SW if available.
> 
> A stream response would happen if the service worker did
> respondWith(fetch(some_url)) or (I think) respondWith(ReadableStream). A blob
> response happens if the service worker responds with a response from Cache
> Storage or generates a response manually like respondWith('hello world'))

Oh, ok, thanks. This was just my ignorance. I only tested with a few simple
pages, and only saw Blob responses. I updated to pass the pipe through when it's
provided. (It does make sense that we will eventually pass a pipe all the way
through, right?)

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:91:
on_created.Run(std::move(handler));
On 2017/05/08 14:55:47, kinuko wrote:
> I think this method and InitializeForNavigationInternal can return
SWReqHandler
> and the caller (InitializeForNavigation*) can call either AttachToURLRequest
or
> CreateURLLoader, which is easier to follow?

The only reason I did that the complicated way was that the call to
navigation_handle_core->DidCreateProviderHost() now occurs before
AttachToURLRequest/CreateURLLoader. However, looking at what it does, this seems
to be OK, so I've updated it to the more direct method you suggested.

In fact, FinalizeHandlerInitialization() wasn't doing anything useful any more,
so I just called provider_host->CreateRequestHandler() directly.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.h (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.h:164: static void
InitializeForNavigationImpl(
On 2017/05/08 14:55:48, kinuko wrote:
> nit: s/Impl/Internal/

Done.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File content/browser/service_worker/service_worker_url_loader_factory.h (right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_url_loader_factory.h:15: class
ServiceWorkerURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/05/08 14:55:48, kinuko wrote:
> Will we need this base class?

Removed. (Not needed now, I was thinking about some sharing we might do, but
better to avoid this complexity for now and we can add it later if we really
need it.)

[scottmg, 2017-05-08 20:12:57]

Description was changed from

==========
network service: Create stub of service worker URLLoader proxy

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png (i.e. only the navigation requests).
There's no handling for the renderer resource requests yet, nor is there
any handling of context-side requests.

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

to

==========
network service: Create URLLoader for service worker navigation case

This forwards navigation requests optionally through a registered
service worker. It does this by reusing code in
ServiceWorkerRequestHandler through a new entry point
(ServiceWorkerRequestHandler::InitializeForNavigationNetworkService),
which is given the opportunity to create ia URLLoaderFactory for the
navigation.

Internally, ServiceWorkerRequestHandler still uses much of the same
machinery (ServiceWorkerStorage, ServiceWorkerVersion) as in the
non-network service case, with the addition of some shim code in
ServiceWorkerControlleeRequestHandler to forward to a new
ServiceWorkerControlleeURLLoaderFactory which uses
ServiceWorkerFetchDispatcher to retrieve the data (replacing
ServiceWorkerURLRequestJob in this path).

There's some placeholder ugliness in SWControllerURLLoaderFactory to
grab the blob returned from the Fetch and turn it into a data pipe. This
can be removed once the blob store is mojoified.

This is about as far as it gets at the moment
http://i.imgur.com/ViO1q5u.png (i.e. only the navigation requests).
There's no handling for the renderer resource requests yet, nor is there
any handling of context-side requests.

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

[falken, 2017-05-09 08:20:15]

FYI I made some revisions to get the patch to compile on Linux:
https://codereview.chromium.org/2867853004/

I know this is a POC patch but when it comes time to land something, it'd be
good if the CL description and comments are clear about when this new path is
taken and which code belongs to the new path.. it's a bit hard to figure out
just by reading the patch. IIUC this path is followed only when both servificied
network and PlzNavigate are enabled.

I didn't really have additional comments beyond that, I just got the patch to
compile locally to start to confirm the new path.

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
File
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
(right):

https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:177:
DCHECK(body_as_stream.is_null());
On 2017/05/08 20:12:31, scottmg wrote:
> On 2017/05/08 08:20:33, falken wrote:
> > I'm not sure I understand the "SWFetchDispatcher only offers blobs for
normal
> > fetches" comment.
> > 
> > SWFetchDispatcher can respond with a stream, in
> > ServiceWorkerFetchDispatcher::Complete it'll call the callback with the
stream
> > response from the SW if available.
> > 
> > A stream response would happen if the service worker did
> > respondWith(fetch(some_url)) or (I think) respondWith(ReadableStream). A
blob
> > response happens if the service worker responds with a response from Cache
> > Storage or generates a response manually like respondWith('hello world'))
> 
> Oh, ok, thanks. This was just my ignorance. I only tested with a few simple
> pages, and only saw Blob responses. I updated to pass the pipe through when
it's
> provided. (It does make sense that we will eventually pass a pipe all the way
> through, right?)

Yea if I understand what Blob Servicification means, eventually the Blob would
be a sort of pipe and we don't need complex blob reading handling here.

[kinuko, 2017-05-09 14:45:25]

Thanks.  I/we still have lots to figure out while coding, for landing could we
start with smaller, easier parts, something like:

1. initial skeleton patch that adds code to create custom URLLoaderFactory for
interceptors on IO thread if necessary.  This patch doesn't really need to have
SW specific code yet (can just have TODO comment and return nullptr), and I
think appcache could possibly share the plumbing bits.

2. another patch that makes SWRequestHandler reusable for URLLoader{Factory}. It
might be nicer/cleaner to make JobWrapper a public class in its own file with
moderately good comments for each method.  URLLoader{Factory} impl can be null
at this point.

While working on these patches I think we could iterate on this poc to evolve
the URLLoader{Factory} implementation part.

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:46: using
CompleteNavigationStart =
nit: CompleteNavigationStartCallback

(preferring to use the names that look more like callbacks)

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:50: void
NavigationStartupWorkOnIO(
nit: maybe... PrepareNavigationStartOnIOThread

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:80:
base::Passed(std::move(mojom::URLLoaderFactoryPtrInfo()))));
I think it'd be probably nicer to have some code structure that allows
interceptor URLLoaderFactory implementors to restart the request with network
service rather than using this kind of callback, then we can possibly reuse that
for appcache?  Say, have some Delegate class that has RestartRequest() method
and let NavURLLoaderImpl implement it?  (We'll need to figure out how to handle
redirect / restart / logic in this code structure, current SWRequestHandler code
assumes its entry point is called multiple times for such cases)

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:265:

We should always be loading main resource here, so
DCHECK(is_main_resource_load_) ?

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:302:
PrepareForSubResource();
This path shouldn't be necessary?

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
File
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
(right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:30:
// that's not done yet to keep things a bit simpler for now.
Maybe the main work should be done by URLLoader impl, and that part could be
called SWURLLoaderJob or something like that (to make it analogous to
SWURLRequestJob)?

[scottmg, 2017-05-09 21:56:02]

On 2017/05/09 08:20:15, falken wrote:
> FYI I made some revisions to get the patch to compile on Linux:
> https://codereview.chromium.org/2867853004/

Sorry about that, I hadn't tested on non-Windows yet.

> 
> I know this is a POC patch but when it comes time to land something, it'd be
> good if the CL description and comments are clear about when this new path is
> taken and which code belongs to the new path.. it's a bit hard to figure out
> just by reading the patch. IIUC this path is followed only when both
servificied
> network and PlzNavigate are enabled.

Yeah, it's only going to be used when --enable-network-service is specified.
Having that flag on also forces on --enable-features=LoadingWithMojo and
--enable-browser-side-navigation. I'll add a few more comments so it's more
clear which functions are only used in this mode.

> 
> I didn't really have additional comments beyond that, I just got the patch to
> compile locally to start to confirm the new path.
> 
>
https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
> File
> content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2843043002/diff/440001/content/browser/servic...
>
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:177:
> DCHECK(body_as_stream.is_null());
> On 2017/05/08 20:12:31, scottmg wrote:
> > On 2017/05/08 08:20:33, falken wrote:
> > > I'm not sure I understand the "SWFetchDispatcher only offers blobs for
> normal
> > > fetches" comment.
> > > 
> > > SWFetchDispatcher can respond with a stream, in
> > > ServiceWorkerFetchDispatcher::Complete it'll call the callback with the
> stream
> > > response from the SW if available.
> > > 
> > > A stream response would happen if the service worker did
> > > respondWith(fetch(some_url)) or (I think) respondWith(ReadableStream). A
> blob
> > > response happens if the service worker responds with a response from Cache
> > > Storage or generates a response manually like respondWith('hello world'))
> > 
> > Oh, ok, thanks. This was just my ignorance. I only tested with a few simple
> > pages, and only saw Blob responses. I updated to pass the pipe through when
> it's
> > provided. (It does make sense that we will eventually pass a pipe all the
way
> > through, right?)
> 
> Yea if I understand what Blob Servicification means, eventually the Blob would
> be a sort of pipe and we don't need complex blob reading handling here.

[scottmg, 2017-05-09 22:14:10]

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:46: using
CompleteNavigationStart =
On 2017/05/09 14:45:25, kinuko wrote:
> nit: CompleteNavigationStartCallback
> 
> (preferring to use the names that look more like callbacks)

Done.

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:50: void
NavigationStartupWorkOnIO(
On 2017/05/09 14:45:25, kinuko wrote:
> nit: maybe... PrepareNavigationStartOnIOThread

Done.

https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:80:
base::Passed(std::move(mojom::URLLoaderFactoryPtrInfo()))));
On 2017/05/09 14:45:25, kinuko wrote:
> I think it'd be probably nicer to have some code structure that allows
> interceptor URLLoaderFactory implementors to restart the request with network
> service rather than using this kind of callback, then we can possibly reuse
that
> for appcache?  Say, have some Delegate class that has RestartRequest() method
> and let NavURLLoaderImpl implement it?  (We'll need to figure out how to
handle
> redirect / restart / logic in this code structure, current SWRequestHandler
code
> assumes its entry point is called multiple times for such cases)

Yes, we'll need something like that in the full approach.

I was also thinking we will need a clearer ordering (i.e. this can't just
directly fallback to network). For example, when extensions and appcache and SW
are all potentially active for a URL, we'll need to give each a crack at it, so
the fallback for SW needs to try extensions, and the fallback for extensions
needs to try appcache (or whatever order we decide is correct).

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
File content/browser/service_worker/service_worker_controllee_request_handler.cc
(right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:265:

On 2017/05/09 14:45:25, kinuko wrote:
> We should always be loading main resource here, so
> DCHECK(is_main_resource_load_) ?

Yup, good point, done. I was thinking maybe we'd share with the renderer factory
at some point, but who knows so might as well remove it for now.

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_request_handler.cc:302:
PrepareForSubResource();
On 2017/05/09 14:45:25, kinuko wrote:
> This path shouldn't be necessary?

Done.

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
File
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
(right):

https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:30:
// that's not done yet to keep things a bit simpler for now.
On 2017/05/09 14:45:25, kinuko wrote:
> Maybe the main work should be done by URLLoader impl, and that part could be
> called SWURLLoaderJob or something like that (to make it analogous to
> SWURLRequestJob)?

Yeah, agreed. It just adds even more boilerplate, so I was avoiding it for now.
Makes sense once we get to landing this part.

[scottmg, 2017-05-09 22:16:40]

On 2017/05/09 14:45:25, kinuko wrote:
> Thanks.  I/we still have lots to figure out while coding, for landing could we
> start with smaller, easier parts, something like:
> 
> 1. initial skeleton patch that adds code to create custom URLLoaderFactory for
> interceptors on IO thread if necessary.  This patch doesn't really need to
have
> SW specific code yet (can just have TODO comment and return nullptr), and I
> think appcache could possibly share the plumbing bits.
> 
> 2. another patch that makes SWRequestHandler reusable for URLLoader{Factory}.
It
> might be nicer/cleaner to make JobWrapper a public class in its own file with
> moderately good comments for each method.  URLLoader{Factory} impl can be null
> at this point.
> 
> While working on these patches I think we could iterate on this poc to evolve
> the URLLoader{Factory} implementation part.

Thanks for taking a look. That seems like a reasonable way to start landing
something. I guess I'll leave this CL here for reference for now, and make a new
CL that covers "#1" for the time being.


> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
> File content/browser/loader/navigation_url_loader_network_service.cc (right):
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
> content/browser/loader/navigation_url_loader_network_service.cc:46: using
> CompleteNavigationStart =
> nit: CompleteNavigationStartCallback
> 
> (preferring to use the names that look more like callbacks)
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
> content/browser/loader/navigation_url_loader_network_service.cc:50: void
> NavigationStartupWorkOnIO(
> nit: maybe... PrepareNavigationStartOnIOThread
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/loader...
> content/browser/loader/navigation_url_loader_network_service.cc:80:
> base::Passed(std::move(mojom::URLLoaderFactoryPtrInfo()))));
> I think it'd be probably nicer to have some code structure that allows
> interceptor URLLoaderFactory implementors to restart the request with network
> service rather than using this kind of callback, then we can possibly reuse
that
> for appcache?  Say, have some Delegate class that has RestartRequest() method
> and let NavURLLoaderImpl implement it?  (We'll need to figure out how to
handle
> redirect / restart / logic in this code structure, current SWRequestHandler
code
> assumes its entry point is called multiple times for such cases)
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
> File
content/browser/service_worker/service_worker_controllee_request_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
>
content/browser/service_worker/service_worker_controllee_request_handler.cc:265:
> 
> We should always be loading main resource here, so
> DCHECK(is_main_resource_load_) ?
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
>
content/browser/service_worker/service_worker_controllee_request_handler.cc:302:
> PrepareForSubResource();
> This path shouldn't be necessary?
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
> File
> content/browser/service_worker/service_worker_controllee_url_loader_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2843043002/diff/500001/content/browser/servic...
>
content/browser/service_worker/service_worker_controllee_url_loader_factory.cc:30:
> // that's not done yet to keep things a bit simpler for now.
> Maybe the main work should be done by URLLoader impl, and that part could be
> called SWURLLoaderJob or something like that (to make it analogous to
> SWURLRequestJob)?

[falken, 2017-05-18 08:28:05]

For reference, I very roughly rebased this patch on the latest ToT (with the
landed stub patch):
https://codereview.chromium.org/2886843006/

I removed the network fallback support though, just to try to get something to
work (wasn't sure how to do fallback with the new stub).