Provide skeleton functionality for AppCache handling in the network service.

Provide skeleton functionality for AppCache handling in the network service.

We have a new class AppCacheNetworkServiceHandler which gets created on the IO thread.
This class provides functionality for checking if a request can be serviced out of the AppCache.
I was looking at AppCacheRequestHandler for this as well. However that class is tied deeply to
the underlying job which gets a touch complicated for lifetime managemnt if the request is not really
being served out the cache.

Added some comments for that class to see if we could remove it once the pieces begin to align.

This class implements the AppCacheStorage::Delegate interface which tells us if a response is available
for a URL. Currently we just trace if a response is found and call the StartURLRequest callback in both cases.

Going forward the plan is to create a URLLoaderFactory for AppCache and pass it back to the callback.
Optionally we could create the loader right there as well. Thanks to jam for pointing that out.

The other changes are to have the original ResourceRequest pointer in the AppCacheURLLoaderRequest class instead
of a copy. We ensure that ownership is transferred correctly.

BUG=715632
Review-Url: https://codereview.chromium.org/2874663004
Cr-Commit-Position: refs/heads/master@{#471120}
Committed: https://chromium.googlesource.com/chromium/src/+/e3f159a4458149bd184fcd1f523a5d9d0a6687f7

[ananta, 2017-05-11 00:27:39]

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

[ananta, 2017-05-11 00:27:40]

Please gloss over the NavigationURLLoaderNetworkService changes in this
patchset. Once scottmg's patch lands I will consolidate with it.

[ananta, 2017-05-11 00:28:58]

Description was changed from

==========
Provide skeleton functionality for AppCache handling in the network service.

We have a new class AppCacheNetworkServiceHandler which gets created on the IO
thread.
This class provides functionality for checking if a request can be serviced out
of the AppCache.
I was looking at AppCacheRequestHandler for this as well. However that class is
tied deeply to
the underlying job which gets a touch complicated for lifetime managemnt if the
request is not really
being served out the cache.

Added some comments for that class to see if we could remove it once the pieces
begin to align.

This class implements the AppCacheStorage::Delegate interface which tells us if
a response is available
for a URL. Currently we just trace if a response is found and call the
StartURLRequest callback in both cases.

Going forward the plan is to create a URLLoaderFactory for AppCache and pass it
back to the callback.
Optionally we could create the loader right there as well. Thanks to jam for
pointing that out.

BUG=715632
==========

to

==========
Provide skeleton functionality for AppCache handling in the network service.

We have a new class AppCacheNetworkServiceHandler which gets created on the IO
thread.
This class provides functionality for checking if a request can be serviced out
of the AppCache.
I was looking at AppCacheRequestHandler for this as well. However that class is
tied deeply to
the underlying job which gets a touch complicated for lifetime managemnt if the
request is not really
being served out the cache.

Added some comments for that class to see if we could remove it once the pieces
begin to align.

This class implements the AppCacheStorage::Delegate interface which tells us if
a response is available
for a URL. Currently we just trace if a response is found and call the
StartURLRequest callback in both cases.

Going forward the plan is to create a URLLoaderFactory for AppCache and pass it
back to the callback.
Optionally we could create the loader right there as well. Thanks to jam for
pointing that out.

The other changes are to have the original ResourceRequest pointer in the
AppCacheURLLoaderRequest class instead
of a copy. We ensure that ownership is transferred correctly.

BUG=715632
==========

[ananta, 2017-05-11 00:29:10]

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

[commit-bot: I haz the power, 2017-05-11 00:30:13]

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-05-11 00:41:08]

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

[commit-bot: I haz the power, 2017-05-11 00:41:09]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2017-05-11 01:42:13]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.cc:61: // Pass a
URLLoaderFactory pointer which supports serving URL requests from
similar to my comment to scott on the other cl for SW, if we find on the IO
thread that the page is served out of Appcache, we should avoid 2 other thread
hops. We can create the URLLoader  directly here.

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_network_service.cc:61: if
(appcache_handle_core) {
to make sure i understand: is this only true if the origin has an appcache
installed? and the other check will confirm if that particular url does?

[ananta, 2017-05-11 02:12:31]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.cc:61: // Pass a
URLLoaderFactory pointer which supports serving URL requests from
On 2017/05/11 01:42:13, jam wrote:
> similar to my comment to scott on the other cl for SW, if we find on the IO
> thread that the page is served out of Appcache, we should avoid 2 other thread
> hops. We can create the URLLoader  directly here.

Noted

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_network_service.cc:61: if
(appcache_handle_core) {
On 2017/05/11 01:42:13, jam wrote:
> to make sure i understand: is this only true if the origin has an appcache
> installed? and the other check will confirm if that particular url does?

Sadly no. For HTTP/HTTPs with PlzNavigate we will always have an
AppCacheNavigateHandleCore pointer as the host is precreated. The other code
will check both the origin and then whether the response can be served.

[kinuko, 2017-05-11 02:14:14]

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

[kinuko, 2017-05-11 02:14:14]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.cc:61: // Pass a
URLLoaderFactory pointer which supports serving URL requests from
On 2017/05/11 01:42:13, jam wrote:
> similar to my comment to scott on the other cl for SW, if we find on the IO
> thread that the page is served out of Appcache, we should avoid 2 other thread
> hops. We can create the URLLoader  directly here.

Yeah, maybe that's better.

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.h (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.h:29: // of that class
gets a touch complicated. The AppCacheRequestHandler is
I'm not yet sure how this class and request_handler code is going to look like,
it sounds like we might be just not reusing the request handler code?  (For SW
case what request handler does is a lot more like what this class does / works
mainly for resolving the final SW to serve)

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_request_handler.h:76: // Same as
InitializeForNavigation() but instead of attaching to a URLRequest,
nit: Incomplete sentence

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_network_service.cc:61: if
(appcache_handle_core) {
On 2017/05/11 01:42:13, jam wrote:
> to make sure i understand: is this only true if the origin has an appcache
> installed? and the other check will confirm if that particular url does?

No if appcache_handle is non-null appcache_handle_core is always non-null as
well.  Existence of appcache is lazily discovered on IO thread (as well as in SW
cases)

[ananta, 2017-05-11 02:30:24]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.h (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.h:29: // of that class
gets a touch complicated. The AppCacheRequestHandler is
On 2017/05/11 02:14:14, kinuko wrote:
> I'm not yet sure how this class and request_handler code is going to look
like,
> it sounds like we might be just not reusing the request handler code?  (For SW
> case what request handler does is a lot more like what this class does / works
> mainly for resolving the final SW to serve)

We are going to reuse the request handler code when the requests are being
served out.
Right now it is not fully clear how that interaction is going to look like. I
plan to revisit this
once we have something functional.

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_request_handler.h:76: // Same as
InitializeForNavigation() but instead of attaching to a URLRequest,
On 2017/05/11 02:14:14, kinuko wrote:
> nit: Incomplete sentence

Done.

[ananta, 2017-05-11 02:32:00]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.h (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.h:29: // of that class
gets a touch complicated. The AppCacheRequestHandler is
On 2017/05/11 02:30:24, ananta wrote:
> On 2017/05/11 02:14:14, kinuko wrote:
> > I'm not yet sure how this class and request_handler code is going to look
> like,
> > it sounds like we might be just not reusing the request handler code?  (For
SW
> > case what request handler does is a lot more like what this class does /
works
> > mainly for resolving the final SW to serve)
> 
> We are going to reuse the request handler code when the requests are being
> served out.
> Right now it is not fully clear how that interaction is going to look like. I
> plan to revisit this
> once we have something functional.

To clarify when a request is served out of the AppCache, the plan is to use the
AppCacheRequestHandler and
the AppCacheURLLoaderJob to handle this. This class is just for checking
AppCache existence and nothing else.

[ananta, 2017-05-11 02:33:05]

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

[commit-bot: I haz the power, 2017-05-11 02:34:50]

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-05-11 02:46:28]

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

[commit-bot: I haz the power, 2017-05-11 02:46:29]

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...)

[kinuko, 2017-05-11 03:23:13]

On 2017/05/11 02:32:00, ananta wrote:
>
https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
> File content/browser/appcache/appcache_network_service_handler.h (right):
> 
>
https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
> content/browser/appcache/appcache_network_service_handler.h:29: // of that
class
> gets a touch complicated. The AppCacheRequestHandler is
> On 2017/05/11 02:30:24, ananta wrote:
> > On 2017/05/11 02:14:14, kinuko wrote:
> > > I'm not yet sure how this class and request_handler code is going to look
> > like,
> > > it sounds like we might be just not reusing the request handler code? 
(For
> SW
> > > case what request handler does is a lot more like what this class does /
> works
> > > mainly for resolving the final SW to serve)
> > 
> > We are going to reuse the request handler code when the requests are being
> > served out.
> > Right now it is not fully clear how that interaction is going to look like.
I
> > plan to revisit this
> > once we have something functional.
> 
> To clarify when a request is served out of the AppCache, the plan is to use
the
> AppCacheRequestHandler and
> the AppCacheURLLoaderJob to handle this. This class is just for checking
> AppCache existence and nothing else.

I see, thanks!

[ananta, 2017-05-11 05:38:41]

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

[commit-bot: I haz the power, 2017-05-11 05:39:41]

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-05-11 05:53:34]

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

[commit-bot: I haz the power, 2017-05-11 05:53:34]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-05-11 06:15:26]

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

[commit-bot: I haz the power, 2017-05-11 06:16:17]

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-05-11 06:28:22]

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

[commit-bot: I haz the power, 2017-05-11 06:28:24]

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

[ananta, 2017-05-11 06:34:39]

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

[commit-bot: I haz the power, 2017-05-11 06:35:40]

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-05-11 07:54:04]

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

[commit-bot: I haz the power, 2017-05-11 07:54:06]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-11 14:50:44]

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_network_service_handler.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_network_service_handler.cc:61: // Pass a
URLLoaderFactory pointer which supports serving URL requests from
On 2017/05/11 02:12:31, ananta wrote:
> On 2017/05/11 01:42:13, jam wrote:
> > similar to my comment to scott on the other cl for SW, if we find on the IO
> > thread that the page is served out of Appcache, we should avoid 2 other
thread
> > hops. We can create the URLLoader  directly here.
> 
> Noted

Just to make sure I understand what you mean: do you agree that this is how it
should be done? If so, this code needs to change then?

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2874663004/diff/1/content/browser/loader/navi...
content/browser/loader/navigation_url_loader_network_service.cc:61: if
(appcache_handle_core) {
On 2017/05/11 02:12:31, ananta wrote:
> On 2017/05/11 01:42:13, jam wrote:
> > to make sure i understand: is this only true if the origin has an appcache
> > installed? and the other check will confirm if that particular url does?
> 
> Sadly no. For HTTP/HTTPs with PlzNavigate we will always have an
> AppCacheNavigateHandleCore pointer as the host is precreated. The other code
> will check both the origin and then whether the response can be served.

ah, I misunderstood that the appcache code would return synchronously if the
origin doesn't have an appcache.

orthogonal to this work, it would be nice to in the future lower the number of
thread hops needed by appcache checking. i.e. why is this a callback that's
posted on a thread, instead of a method that returns a value? maybe it had to be
this way for URLRequestJob, but for network service path we can just return
synchronously.

[jam, 2017-05-11 19:35:55]

lgtm

(per IM, the threading will be clarified in the next cls)

[scottmg, 2017-05-11 21:13:10]

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

[scottmg, 2017-05-11 21:13:11]

Sorry, hadn't seen this CL.

We can't make the URLLoader on the IO because it needs to be on UI for
navigation. I don't think it can be associated while on a foreign thread, but
maybe I'm misinterpreting the dcheck.

I think at the moment, we either have to hold off, or we're stuck with 2 hops. I
think we need to do one/some of:
- allow association from a foreign thread
- live with the 2 hops for now
- further investigate instantiating important data objects on the same thread as
navigation

[ananta, 2017-05-11 21:42:14]

On 2017/05/11 21:13:11, scottmg wrote:
> Sorry, hadn't seen this CL.
> 
> We can't make the URLLoader on the IO because it needs to be on UI for
> navigation. I don't think it can be associated while on a foreign thread, but
> maybe I'm misinterpreting the dcheck.
> 
> I think at the moment, we either have to hold off, or we're stuck with 2 hops.
I
> think we need to do one/some of:
> - allow association from a foreign thread
> - live with the 2 hops for now
> - further investigate instantiating important data objects on the same thread
as
> navigation

Thanks scottmg. I disabled the posting to the IO thread for appcache handle
which means that we will only
go to the IO thread for post requests. 

Based on our discussion, we already have this model today in the navigation code
for plznavigate, where the impl object lives
on the UI thread and the core object on the IO thread and tasks flow back and
forth. I gather that this is a touch annoying. If there is a way
to make mojo use the current thread for its IO, then it is the same hops as
today?

[ananta, 2017-05-11 21:42:37]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-05-11 21:42:38]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2874663004/#ps100001
(title: "Disable posting to the IO thread if appcache is enabled in the n/w
service code")

[commit-bot: I haz the power, 2017-05-11 21:43:18]

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-05-11 23:40:11]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494538957905350,
"parent_rev": "03bf758ea78ac01e2b970e0ce3eb22456dc518c6", "commit_rev":
"e3f159a4458149bd184fcd1f523a5d9d0a6687f7"}

[commit-bot: I haz the power, 2017-05-11 23:40:22]

Description was changed from

==========
Provide skeleton functionality for AppCache handling in the network service.

We have a new class AppCacheNetworkServiceHandler which gets created on the IO
thread.
This class provides functionality for checking if a request can be serviced out
of the AppCache.
I was looking at AppCacheRequestHandler for this as well. However that class is
tied deeply to
the underlying job which gets a touch complicated for lifetime managemnt if the
request is not really
being served out the cache.

Added some comments for that class to see if we could remove it once the pieces
begin to align.

This class implements the AppCacheStorage::Delegate interface which tells us if
a response is available
for a URL. Currently we just trace if a response is found and call the
StartURLRequest callback in both cases.

Going forward the plan is to create a URLLoaderFactory for AppCache and pass it
back to the callback.
Optionally we could create the loader right there as well. Thanks to jam for
pointing that out.

The other changes are to have the original ResourceRequest pointer in the
AppCacheURLLoaderRequest class instead
of a copy. We ensure that ownership is transferred correctly.

BUG=715632
==========

to

==========
Provide skeleton functionality for AppCache handling in the network service.

We have a new class AppCacheNetworkServiceHandler which gets created on the IO
thread.
This class provides functionality for checking if a request can be serviced out
of the AppCache.
I was looking at AppCacheRequestHandler for this as well. However that class is
tied deeply to
the underlying job which gets a touch complicated for lifetime managemnt if the
request is not really
being served out the cache.

Added some comments for that class to see if we could remove it once the pieces
begin to align.

This class implements the AppCacheStorage::Delegate interface which tells us if
a response is available
for a URL. Currently we just trace if a response is found and call the
StartURLRequest callback in both cases.

Going forward the plan is to create a URLLoaderFactory for AppCache and pass it
back to the callback.
Optionally we could create the loader right there as well. Thanks to jam for
pointing that out.

The other changes are to have the original ResourceRequest pointer in the
AppCacheURLLoaderRequest class instead
of a copy. We ensure that ownership is transferred correctly.

BUG=715632

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

[commit-bot: I haz the power, 2017-05-11 23:40:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e3f159a4458149bd184fcd1f523a...