Add support for subresource request fallback in AppCache for the network service..

Add support for subresource request fallback in AppCache for the network service..

This patch builds on patch https://codereview.chromium.org/2956373002/ for adding fallback
support for subresource requests. This is implemented by passing a proxy URLLoaderClient
interface to the network URL service for subresource requests which are sent to the network.
We intercept the OnReceiveResponse, OnReceiveRedirect and OnComplete calls for supporting
fallback responses in these cases. We could potentially look at moving fallback response
reading to the renderer in a later patchset.

The changes in this patchset are as below:
1. Add support for passing the ResourceResponseHead to the AppCacheURLLoaderJob. This is
   required for supporting fallback responses. The request handler looks at headers in
   the response and the status code to make a call whether a fallback is required.

2. AppCacheRequestHandler::MaybeLoadFallbackForResponse() and MaybeLoadFallbackForRedirect() have
   changed to allow for the fact that they are called in the network service code by the
   AppCacheURLLoaderJob. We don't create new job for delivering the fallback in this case.

3. Added methods in the AppCacheRequest class  to return the URLRequest override and URLLoader
   override respectively.

Next step is to add support for main resource fallback.

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

[ananta, 2017-07-09 03:20:12]

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

[ananta, 2017-07-09 03:20:13]

Changes from the https://codereview.chromium.org/2956373002/ patch are also
showing up here. Will look into that. This patch adds support for subresource
fallback. Main resource fallback is next which is a touch involved.

[ananta, 2017-07-14 01:15:03]

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

[commit-bot: I haz the power, 2017-07-14 01:15:27]

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-07-14 01:32:53]

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

[commit-bot: I haz the power, 2017-07-14 01:32:54]

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

[ananta, 2017-07-14 01:37:25]

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

[commit-bot: I haz the power, 2017-07-14 01:37:44]

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-07-14 02:25:38]

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

[commit-bot: I haz the power, 2017-07-14 02:25:40]

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

[ananta, 2017-07-14 20:31:45]

Patch should be ready for review.

[michaeln, 2017-07-17 21:04:55]

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:134: // the existing job to
deliver the fallback response.
I think we might be able to better encapsulate this difference about the
lifecycle of the job object. Could the logic about reusing the existing job or
not was inside the CreateJob() method, wdyt?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:138: job.reset(job_.get());
does the dcheck on line 130 no longer hold true?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:151: // 6.9.6 step 3 and 5:
Fetch the resource normally.
In the net::urlrequest based impl, exsting code returns nullptr in this case,
but in the CL we're returning a non-null job ptr in that case. Is this change
intentional?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:199: if
(!base::FeatureList::IsEnabled(features::kNetworkService)) {
Ditto question about encapsulating reuse vs recreate inside of CreateJob()?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:349: if (job_.get() &&
!is_main_resource() &&
can job_.get() test false here?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:193: if
(!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr))
is this one really needed, does the code in OnReceiveResponse cover this case

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:239:
url_loader_request_instance_{url_loader_request} {}
are the brackets needed, can we use parens here?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:324: void
AppCacheURLLoaderJob::OnConnectionError() {
I think this method is being used for two different kinds of errors.
1. the renderer crashes or deletes the urlloader
2. this objects network_loader craps out for some reason

In the latter case, the remote side in the renderer is still there. Is this
handling good for that case too?

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:77: // mojom::URLLoaderClient
implementation
maybe comment on how these methods apply to the network url loader this class
internally creates when delivering a network response

[ananta, 2017-07-18 00:19:04]

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:134: // the existing job to
deliver the fallback response.
On 2017/07/17 21:04:55, michaeln wrote:
> I think we might be able to better encapsulate this difference about the
> lifecycle of the job object. Could the logic about reusing the existing job or
> not was inside the CreateJob() method, wdyt?

Did not want to touch the CreateJob() function. I added another method
MaybeCreateJobForFallback() which is called from here and from
MaybeLoadFallbackForResponse()

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:138: job.reset(job_.get());
On 2017/07/17 21:04:55, michaeln wrote:
> does the dcheck on line 130 no longer hold true?

It does. I changed the DCHECK for the network service case along with a comment

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:151: // 6.9.6 step 3 and 5:
Fetch the resource normally.
On 2017/07/17 21:04:55, michaeln wrote:
> In the net::urlrequest based impl, exsting code returns nullptr in this case,
> but in the CL we're returning a non-null job ptr in that case. Is this change
> intentional?

I restored the code back to how it was originally. We call the newly added
method MaybeCreateJobForFallback() in the if and else statements.

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:199: if
(!base::FeatureList::IsEnabled(features::kNetworkService)) {
On 2017/07/17 21:04:55, michaeln wrote:
> Ditto question about encapsulating reuse vs recreate inside of CreateJob()?

Moved this code to MaybeCreateJobForFallback().

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:349: if (job_.get() &&
!is_main_resource() &&
On 2017/07/17 21:04:55, michaeln wrote:
> can job_.get() test false here?

Yeah. Removed it.

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:193: if
(!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr))
On 2017/07/17 21:04:55, michaeln wrote:
> is this one really needed, does the code in OnReceiveResponse cover this case

It is needed. We don't receive the OnReceiveResponse() call reliably. For e.g.
no network

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:239:
url_loader_request_instance_{url_loader_request} {}
On 2017/07/17 21:04:55, michaeln wrote:
> are the brackets needed, can we use parens here?

eek. typo. Fixed

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:324: void
AppCacheURLLoaderJob::OnConnectionError() {
On 2017/07/17 21:04:55, michaeln wrote:
> I think this method is being used for two different kinds of errors.
> 1. the renderer crashes or deletes the urlloader
> 2. this objects network_loader craps out for some reason
> 
> In the latter case, the remote side in the renderer is still there. Is this
> handling good for that case too?

Thanks for noticing this. I removed the bind call to this method from
CreateLoaderClientProxy()

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:77: // mojom::URLLoaderClient
implementation
On 2017/07/17 21:04:55, michaeln wrote:
> maybe comment on how these methods apply to the network url loader this class
> internally creates when delivering a network response

Done.

[ananta, 2017-07-18 00:19:19]

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

[commit-bot: I haz the power, 2017-07-18 00:19:29]

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-07-18 00:46:35]

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

[commit-bot: I haz the power, 2017-07-18 00:46:36]

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

[michaeln, 2017-07-18 01:40:37]

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:193: if
(!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr))
On 2017/07/18 00:19:03, ananta wrote:
> On 2017/07/17 21:04:55, michaeln wrote:
> > is this one really needed, does the code in OnReceiveResponse cover this
case
> 
> It is needed. We don't receive the OnReceiveResponse() call reliably. For e.g.
> no network

What happens to the network_loader when we do invoke a fallback on
OnReceiveResponse or OnReceiveRedirect. How does the network_load get killed and
do we get an OnComplete(CANCELLED) notification out of it?

Do we need to delete, or otherwise kill, the network_loader inside of
DeliverAppCachedResponse or DeliverErrorResponse if/when those methods are
called while processing OnReceiveResponse or OnReceiveRedirect.

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:355: job =
CreateJob(network_delegate);
maybe rearrange, early return, for readiability, the comment applies more to the
else clause than the if

// We create a new job with the net::URLRequest impl.
if (!networkServiceEnabled)
  return CreateJob();

// But reuse the existing job with the networkService impl.
DCHECK(job.get());
return job.get();

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:357: DCHECK(!job_.get()); 
// our jobs never generate redirects
should this check for not-nullness?

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:42: 
if networkServiceEnabled what do we do with network_loader_request_ here? It
should be non-null if we're invoking a fallback, and we should stop it from
making anymore progress.

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:79: } else {
can we DCHECK(!network_loader_request_) prior to creating it?

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:91: void
AppCacheURLLoaderJob::DeliverErrorResponse() {
ditto what if network_loader_request_ is non-null?

[ananta, 2017-07-18 02:06:50]

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:193: if
(!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr))
On 2017/07/18 01:40:36, michaeln wrote:
> On 2017/07/18 00:19:03, ananta wrote:
> > On 2017/07/17 21:04:55, michaeln wrote:
> > > is this one really needed, does the code in OnReceiveResponse cover this
> case
> > 
> > It is needed. We don't receive the OnReceiveResponse() call reliably. For
e.g.
> > no network
> 
> What happens to the network_loader when we do invoke a fallback on
> OnReceiveResponse or OnReceiveRedirect. How does the network_load get killed
and
> do we get an OnComplete(CANCELLED) notification out of it?
> 
> Do we need to delete, or otherwise kill, the network_loader inside of
> DeliverAppCachedResponse or DeliverErrorResponse if/when those methods are
> called while processing OnReceiveResponse or OnReceiveRedirect.

We should not be receiving those calls as they are on the same thread. I added
code to close the mojo binding handle to the loader if we create a fallback job.

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:355: job =
CreateJob(network_delegate);
On 2017/07/18 01:40:37, michaeln wrote:
> maybe rearrange, early return, for readiability, the comment applies more to
the
> else clause than the if
> 
> // We create a new job with the net::URLRequest impl.
> if (!networkServiceEnabled)
>   return CreateJob();
> 
> // But reuse the existing job with the networkService impl.
> DCHECK(job.get());
> return job.get();

Done.

https://codereview.chromium.org/2974733002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:357: DCHECK(!job_.get()); 
// our jobs never generate redirects
On 2017/07/18 01:40:37, michaeln wrote:
> should this check for not-nullness?

Yes. Done

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:42: 
On 2017/07/18 01:40:37, michaeln wrote:
> if networkServiceEnabled what do we do with network_loader_request_ here? It
> should be non-null if we're invoking a fallback, and we should stop it from
> making anymore progress.

The binding is closed between the network service and us when we create a
fallback response. We should not be receiving any further notifications from
that point on from the network.

https://codereview.chromium.org/2974733002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:91: void
AppCacheURLLoaderJob::DeliverErrorResponse() {
On 2017/07/18 01:40:37, michaeln wrote:
> ditto what if network_loader_request_ is non-null?

Shouldn't matter because we wold have closed the binding.

[ananta, 2017-07-18 02:11:39]

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.cc:193: if
(!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr))
On 2017/07/18 02:06:49, ananta wrote:
> On 2017/07/18 01:40:36, michaeln wrote:
> > On 2017/07/18 00:19:03, ananta wrote:
> > > On 2017/07/17 21:04:55, michaeln wrote:
> > > > is this one really needed, does the code in OnReceiveResponse cover this
> > case
> > > 
> > > It is needed. We don't receive the OnReceiveResponse() call reliably. For
> e.g.
> > > no network
> > 
> > What happens to the network_loader when we do invoke a fallback on
> > OnReceiveResponse or OnReceiveRedirect. How does the network_load get killed
> and
> > do we get an OnComplete(CANCELLED) notification out of it?
> > 
> > Do we need to delete, or otherwise kill, the network_loader inside of
> > DeliverAppCachedResponse or DeliverErrorResponse if/when those methods are
> > called while processing OnReceiveResponse or OnReceiveRedirect.
> 
> We should not be receiving those calls as they are on the same thread. I added
> code to close the mojo binding handle to the loader if we create a fallback
job.

Thanks for catching this. I was only testing the case when there was no network.
Looking at the code there are cases where fallback could be delivered even for
valid responses.

[ananta, 2017-07-18 02:12:03]

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

[commit-bot: I haz the power, 2017-07-18 02:12:21]

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-07-18 03:12:45]

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

[commit-bot: I haz the power, 2017-07-18 03:12:46]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-07-18 19:19:23]

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:206:
loader_client_proxy_binding_.Close();
This turns off the network loaders delegate callbacks (so to speak), but should
we also reset the actual loader to really kill it and free up all resources
associated with it?

network_loader_client_binding_.Close();
network_loader_.reset(); 
or
network_loader_ = nullptr;

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:241:
AppCacheURLLoaderRequest* url_loader_request,
Calling this 'url_loader_request' is misleading, it makes it sounds like a
specific kind of mojom::URLLoaderRequest which is something completely
different. This is a straight c++ AppCacheRequest derivative.

Maybe just 'appcache_request' and 'appcache_request_' wdyt?

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:452:
mojom::URLLoaderClientPtr AppCacheURLLoaderJob::CreateLoaderClientProxy() {
Consider moving the body of this method directly into the
DeliverNetworkResponse() method. I think there is no other callsite and hoisting
into that method would make the relationship to the network_loader more clear.

Some readbility/naming nits:

Its not obvious that we're setting up 'client' of the network_loader here,
consider network_loader_client_binding_.

About the name 'network_loader_request_', for all practical purposes, this is
the network_loader from appcache job's point of view. I don't think having
'request' in the name helps and I think stripping request from this name would
be more consistent with how mojom interface ptrs are named elsewhere in the
project.

I put naming nit suggestions for a handful of data members in the .h file, wdyt?

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:176:
mojom::URLLoaderClientPtr client_info_;
naming: client_
(_info sounds more like a data structure than an interface ptr)

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:202:
mojom::URLLoaderAssociatedPtr network_loader_request_;
naming: network_loader_

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:211:
mojo::Binding<mojom::URLLoaderClient> loader_client_proxy_binding_;
naming: network_loader_client_binding_

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:215:
AppCacheURLLoaderRequest* url_loader_request_instance_;
naming: appcache_request_

[ananta, 2017-07-18 19:52:28]

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

[ananta, 2017-07-18 19:52:40]

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:206:
loader_client_proxy_binding_.Close();
On 2017/07/18 19:19:22, michaeln wrote:
> This turns off the network loaders delegate callbacks (so to speak), but
should
> we also reset the actual loader to really kill it and free up all resources
> associated with it?
> 
> network_loader_client_binding_.Close();
> network_loader_.reset(); 
> or
> network_loader_ = nullptr;

Thanks. Done. I moved this to a function DisconnectFromNetworkLoader().

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:241:
AppCacheURLLoaderRequest* url_loader_request,
On 2017/07/18 19:19:22, michaeln wrote:
> Calling this 'url_loader_request' is misleading, it makes it sounds like a
> specific kind of mojom::URLLoaderRequest which is something completely
> different. This is a straight c++ AppCacheRequest derivative.
> 
> Maybe just 'appcache_request' and 'appcache_request_' wdyt?

Makes sense. The names are confusing. Done.

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:452:
mojom::URLLoaderClientPtr AppCacheURLLoaderJob::CreateLoaderClientProxy() {
On 2017/07/18 19:19:22, michaeln wrote:
> Consider moving the body of this method directly into the
> DeliverNetworkResponse() method. I think there is no other callsite and
hoisting
> into that method would make the relationship to the network_loader more clear.
> 
> Some readbility/naming nits:
> 
> Its not obvious that we're setting up 'client' of the network_loader here,
> consider network_loader_client_binding_.
> 
> About the name 'network_loader_request_', for all practical purposes, this is
> the network_loader from appcache job's point of view. I don't think having
> 'request' in the name helps and I think stripping request from this name would
> be more consistent with how mojom interface ptrs are named elsewhere in the
> project.
> 
> I put naming nit suggestions for a handful of data members in the .h file,
wdyt?

Thanks. done.

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:176:
mojom::URLLoaderClientPtr client_info_;
On 2017/07/18 19:19:22, michaeln wrote:
> naming: client_
> (_info sounds more like a data structure than an interface ptr)

Done.

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:202:
mojom::URLLoaderAssociatedPtr network_loader_request_;
On 2017/07/18 19:19:22, michaeln wrote:
> naming: network_loader_

Done.

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:211:
mojo::Binding<mojom::URLLoaderClient> loader_client_proxy_binding_;
On 2017/07/18 19:19:22, michaeln wrote:
> naming: network_loader_client_binding_

Done.

https://codereview.chromium.org/2974733002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:215:
AppCacheURLLoaderRequest* url_loader_request_instance_;
On 2017/07/18 19:19:22, michaeln wrote:
> naming: appcache_request_

Done.

[commit-bot: I haz the power, 2017-07-18 19:52:47]

Dry run: CQ is trying da patch.

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

[michaeln, 2017-07-18 21:41:54]

lgtm and thanx for fixing up the names!

[commit-bot: I haz the power, 2017-07-18 21:50:46]

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

[commit-bot: I haz the power, 2017-07-18 21:50:47]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-07-18 22:38:22]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-07-18 22:38:33]

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-07-18 22:43:39]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1500417502748960,
"parent_rev": "09d5925010193da41d5f4e765ef2b68cd8ceca8c", "commit_rev":
"b42794ede836d48b113e1619cac04a962c705f6c"}

[commit-bot: I haz the power, 2017-07-18 22:43:54]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1500417502748960,
"parent_rev": "d654e49c79144d2f354c54e713080a2e2af9dfbc", "commit_rev":
"2ec636380ffa2f4df27e3525a32f7aae6658b58b"}

[commit-bot: I haz the power, 2017-07-18 22:44:09]

Description was changed from

==========
Add support for subresource request fallback in AppCache for the network
service..

This patch builds on patch https://codereview.chromium.org/2956373002/ for
adding fallback
support for subresource requests. This is implemented by passing a proxy
URLLoaderClient
interface to the network URL service for subresource requests which are sent to
the network.
We intercept the OnReceiveResponse, OnReceiveRedirect and OnComplete calls for
supporting
fallback responses in these cases. We could potentially look at moving fallback
response
reading to the renderer in a later patchset.

The changes in this patchset are as below:
1. Add support for passing the ResourceResponseHead to the AppCacheURLLoaderJob.
This is
   required for supporting fallback responses. The request handler looks at
headers in
   the response and the status code to make a call whether a fallback is
required.

2. AppCacheRequestHandler::MaybeLoadFallbackForResponse() and
MaybeLoadFallbackForRedirect() have
   changed to allow for the fact that they are called in the network service
code by the
   AppCacheURLLoaderJob. We don't create new job for delivering the fallback in
this case.

3. Added methods in the AppCacheRequest class  to return the URLRequest override
and URLLoader
   override respectively.

Next step is to add support for main resource fallback.

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

to

==========
Add support for subresource request fallback in AppCache for the network
service..

This patch builds on patch https://codereview.chromium.org/2956373002/ for
adding fallback
support for subresource requests. This is implemented by passing a proxy
URLLoaderClient
interface to the network URL service for subresource requests which are sent to
the network.
We intercept the OnReceiveResponse, OnReceiveRedirect and OnComplete calls for
supporting
fallback responses in these cases. We could potentially look at moving fallback
response
reading to the renderer in a later patchset.

The changes in this patchset are as below:
1. Add support for passing the ResourceResponseHead to the AppCacheURLLoaderJob.
This is
   required for supporting fallback responses. The request handler looks at
headers in
   the response and the status code to make a call whether a fallback is
required.

2. AppCacheRequestHandler::MaybeLoadFallbackForResponse() and
MaybeLoadFallbackForRedirect() have
   changed to allow for the fact that they are called in the network service
code by the
   AppCacheURLLoaderJob. We don't create new job for delivering the fallback in
this case.

3. Added methods in the AppCacheRequest class  to return the URLRequest override
and URLLoader
   override respectively.

Next step is to add support for main resource fallback.

BUG=715632

Review-Url: https://codereview.chromium.org/2974733002
Cr-Commit-Position: refs/heads/master@{#487640}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ec636380ffa2f4df27e3525a32f...
==========

[commit-bot: I haz the power, 2017-07-18 22:44:10]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2ec636380ffa2f4df27e3525a32f...