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

content/browser/appcache/appcache_request_handler.cc

Index: content/browser/appcache/appcache_request_handler.cc
diff --git a/content/browser/appcache/appcache_request_handler.cc b/content/browser/appcache/appcache_request_handler.cc
index 652eff24a7acf59b8edd790ac25b29a544e3b5f2..bb951098881a961e79d01be7396d5d9b944218f4 100644
--- a/content/browser/appcache/appcache_request_handler.cc
+++ b/content/browser/appcache/appcache_request_handler.cc
@@ -130,16 +130,22 @@ AppCacheJob* AppCacheRequestHandler::MaybeLoadFallbackForRedirect(
   DCHECK(!job_.get());  // our jobs never generate redirects
 
   std::unique_ptr<AppCacheJob> job;
+  // In network service land, the job initiates a fallback request. We reuse
+  // the existing job to deliver the fallback response.

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

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?

[ananta, 2017/07/18 00:19:03]

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

+  if (!base::FeatureList::IsEnabled(features::kNetworkService)) {
+    job = CreateJob(network_delegate);
+  } else {
+    job.reset(job_.get());

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

does the dcheck on line 130 no longer hold true?

[ananta, 2017/07/18 00:19:03]

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

+  }
+
   if (found_fallback_entry_.has_response_id()) {
     // 6.9.6, step 4: If this results in a redirect to another origin,
     // get the resource of the fallback entry.
-    job = CreateJob(network_delegate);
     DeliverAppCachedResponse(found_fallback_entry_, found_cache_id_,
                              found_manifest_url_, true,
                              found_namespace_entry_url_);
   } else if (!found_network_namespace_) {
     // 6.9.6, step 6: Fail the resource load.
-    job = CreateJob(network_delegate);
     DeliverErrorResponse();
   } else {
     // 6.9.6 step 3 and 5: Fetch the resource normally.

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

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?

[ananta, 2017/07/18 00:19:03]

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

@@ -163,7 +169,7 @@ AppCacheJob* AppCacheRequestHandler::MaybeLoadFallbackForResponse(
   }
 
   // We don't fallback for responses that we delivered.
-  if (job_.get()) {
+  if (job_.get() && !base::FeatureList::IsEnabled(features::kNetworkService)) {
     DCHECK(!job_->IsDeliveringNetworkResponse());
     return NULL;
   }
@@ -186,7 +192,17 @@ AppCacheJob* AppCacheRequestHandler::MaybeLoadFallbackForResponse(
 
   // 6.9.6, step 4: If this results in a 4xx or 5xx status code
   // or there were network errors, get the resource of the fallback entry.
-  std::unique_ptr<AppCacheJob> job = CreateJob(network_delegate);
+
+  // In network service land, the job initiates a fallback request. We reuse
+  // the existing job to deliver the fallback response.
+  std::unique_ptr<AppCacheJob> job;
+  if (!base::FeatureList::IsEnabled(features::kNetworkService)) {

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

Ditto question about encapsulating reuse vs recreate inside of CreateJob()?

[ananta, 2017/07/18 00:19:03]

Moved this code to MaybeCreateJobForFallback().

+    job = CreateJob(network_delegate);
+  } else {
+    DCHECK(job_.get());
+    job.reset(job_.get());
+  }
+
   DeliverAppCachedResponse(found_fallback_entry_, found_cache_id_,
                            found_manifest_url_, true,
                            found_namespace_entry_url_);
@@ -330,7 +346,7 @@ std::unique_ptr<AppCacheJob> AppCacheRequestHandler::CreateJob(
       base::Bind(&AppCacheRequestHandler::OnPrepareToRestart,
                  base::Unretained(this)));
   job_ = job->GetWeakPtr();
-  if (!is_main_resource() &&
+  if (job_.get() && !is_main_resource() &&

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

can job_.get() test false here?

[ananta, 2017/07/18 00:19:03]

Yeah. Removed it.

       base::FeatureList::IsEnabled(features::kNetworkService)) {
     AppCacheURLLoaderJob* loader_job = job_->AsURLLoaderJob();