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

content/browser/appcache/appcache_url_loader_job.cc

Index: content/browser/appcache/appcache_url_loader_job.cc
diff --git a/content/browser/appcache/appcache_url_loader_job.cc b/content/browser/appcache/appcache_url_loader_job.cc
index 1d1932f7aa137d40dff546ab3ca0b9a31351ed4b..786927bf9c353b947bf8afb95e9b405329168b3c 100644
--- a/content/browser/appcache/appcache_url_loader_job.cc
+++ b/content/browser/appcache/appcache_url_loader_job.cc
@@ -7,6 +7,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "content/browser/appcache/appcache_histograms.h"
 #include "content/browser/appcache/appcache_subresource_url_factory.h"
+#include "content/browser/appcache/appcache_url_loader_request.h"
 #include "content/browser/url_loader_factory_getter.h"
 #include "content/common/net_adapters.h"
 #include "content/public/common/resource_type.h"
@@ -82,7 +83,7 @@ void AppCacheURLLoaderJob::DeliverNetworkResponse() {
             mojo::MakeRequest(&network_loader_request_),
             subresource_load_info_->routing_id,
             subresource_load_info_->request_id, subresource_load_info_->options,
-            subresource_load_info_->request, std::move(client_info_),
+            subresource_load_info_->request, CreateLoaderClientProxy(),
             subresource_load_info_->traffic_annotation);
   }
 }
@@ -130,6 +131,82 @@ void AppCacheURLLoaderJob::SetPriority(net::RequestPriority priority,
     network_loader_request_->SetPriority(priority, intra_priority_value);
 }
 
+void AppCacheURLLoaderJob::OnReceiveResponse(
+    const ResourceResponseHead& response_head,
+    const base::Optional<net::SSLInfo>& ssl_info,
+    mojom::DownloadedTempFilePtr downloaded_file) {
+  url_loader_request_instance_->set_response(response_head);
+  // The MaybeLoadFallbackForResponse() call below can pass a fallback
+  // response to us. Reset the delivery_type_ to ensure that we can
+  // receive it
+  delivery_type_ = AWAITING_DELIVERY_ORDERS;
+  if (!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr)) {
+    client_info_->OnReceiveResponse(response_head, ssl_info,
+                                    std::move(downloaded_file));
+  } else {
+    // Close the pipe to the network loader as we are delivering a fallback
+    // response to the client.
+    loader_client_proxy_binding_.Close();
+  }
+}
+
+void AppCacheURLLoaderJob::OnReceiveRedirect(
+    const net::RedirectInfo& redirect_info,
+    const ResourceResponseHead& response_head) {
+  url_loader_request_instance_->set_response(response_head);
+  // The MaybeLoadFallbackForRedirect() call below can pass a fallback
+  // response to us. Reset the delivery_type_ to ensure that we can
+  // receive it
+  delivery_type_ = AWAITING_DELIVERY_ORDERS;
+  if (!sub_resource_handler_->MaybeLoadFallbackForRedirect(
+          nullptr, redirect_info.new_url)) {
+    client_info_->OnReceiveRedirect(redirect_info, response_head);
+  } else {
+    // Close the pipe to the network loader as we are delivering a fallback
+    // response to the client.
+    loader_client_proxy_binding_.Close();
+  }
+}
+
+void AppCacheURLLoaderJob::OnDataDownloaded(int64_t data_len,
+                                            int64_t encoded_data_len) {
+  client_info_->OnDataDownloaded(data_len, encoded_data_len);
+}
+
+void AppCacheURLLoaderJob::OnUploadProgress(
+    int64_t current_position,
+    int64_t total_size,
+    OnUploadProgressCallback ack_callback) {
+  client_info_->OnUploadProgress(current_position, total_size,
+                                 std::move(ack_callback));
+}
+
+void AppCacheURLLoaderJob::OnReceiveCachedMetadata(
+    const std::vector<uint8_t>& data) {
+  client_info_->OnReceiveCachedMetadata(data);
+}
+
+void AppCacheURLLoaderJob::OnTransferSizeUpdated(int32_t transfer_size_diff) {
+  client_info_->OnTransferSizeUpdated(transfer_size_diff);
+}
+
+void AppCacheURLLoaderJob::OnStartLoadingResponseBody(
+    mojo::ScopedDataPipeConsumerHandle body) {
+  client_info_->OnStartLoadingResponseBody(std::move(body));
+}
+
+void AppCacheURLLoaderJob::OnComplete(
+    const ResourceRequestCompletionStatus& status) {
+  delivery_type_ = AWAITING_DELIVERY_ORDERS;
+  if (!sub_resource_handler_->MaybeLoadFallbackForResponse(nullptr)) {
+    client_info_->OnComplete(status);
+  } else {
+    // Close the pipe to the network loader as we are delivering a fallback
+    // response to the client.
+    loader_client_proxy_binding_.Close();

[michaeln, 2017/07/18 19:19:22]

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;

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

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

+  }
+}
+
 void AppCacheURLLoaderJob::SetSubresourceLoadInfo(
     std::unique_ptr<SubresourceLoadInfo> subresource_load_info,
     URLLoaderFactoryGetter* default_url_loader) {
@@ -159,8 +236,10 @@ void AppCacheURLLoaderJob::Start(mojom::URLLoaderRequest request,
     SendResponseInfo();
 }
 
-AppCacheURLLoaderJob::AppCacheURLLoaderJob(const ResourceRequest& request,
-                                           AppCacheStorage* storage)
+AppCacheURLLoaderJob::AppCacheURLLoaderJob(
+    const ResourceRequest& request,
+    AppCacheURLLoaderRequest* url_loader_request,

[michaeln, 2017/07/18 19:19:22]

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?

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

Makes sense. The names are confusing. Done.

+    AppCacheStorage* storage)
     : request_(request),
       storage_(storage->GetWeakPtr()),
       start_time_tick_(base::TimeTicks::Now()),
@@ -168,7 +247,9 @@ AppCacheURLLoaderJob::AppCacheURLLoaderJob(const ResourceRequest& request,
       is_fallback_(false),
       binding_(this),
       writable_handle_watcher_(FROM_HERE,
-                               mojo::SimpleWatcher::ArmingPolicy::MANUAL) {}
+                               mojo::SimpleWatcher::ArmingPolicy::MANUAL),
+      loader_client_proxy_binding_(this),
+      url_loader_request_instance_(url_loader_request) {}
 
 void AppCacheURLLoaderJob::OnResponseInfoLoaded(
     AppCacheResponseInfo* response_info,
@@ -293,6 +374,8 @@ void AppCacheURLLoaderJob::SendResponseInfo() {
 
   response_head.load_timing = load_timing_info_;
 
+  url_loader_request_instance_->set_response(response_head);
+
   client_info_->OnReceiveResponse(response_head, http_info->ssl_info,
                                   mojom::DownloadedTempFilePtr());
 
@@ -366,4 +449,10 @@ void AppCacheURLLoaderJob::NotifyCompleted(int error_code) {
   client_info_->OnComplete(request_complete_data);
 }
 
+mojom::URLLoaderClientPtr AppCacheURLLoaderJob::CreateLoaderClientProxy() {

[michaeln, 2017/07/18 19:19:22]

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?

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

Thanks. done.

+  mojom::URLLoaderClientPtr client_ptr;
+  loader_client_proxy_binding_.Bind(mojo::MakeRequest(&client_ptr));
+  return client_ptr;
+}
+
 }  // namespace content