Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index adf9698cb5d7d908e21130451b162156a70df191..7da97660167139f6ee3ed064d18f15e836c04efe 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -21,14 +21,19 @@
 #include "base/supports_user_data.h"
 #include "base/synchronization/lock.h"
 #include "build/build_config.h"
+#include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/byte_stream.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_item_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_stats.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/browser/loader/throttling_resource_handler.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
+#include "content/browser/resource_context_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
@@ -39,6 +44,8 @@
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_dispatcher_host_delegate.h"
+#include "content/public/browser/resource_throttle.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/referrer.h"
 #include "net/base/elements_upload_data_stream.h"
@@ -46,12 +53,38 @@
 #include "net/base/request_priority.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/url_request/url_request_context.h"
+#include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/blob/blob_url_request_job_factory.h"
 #include "url/origin.h"
 
 namespace content {
 namespace {
 
+// Helper function to create an instance of the DownloadResourceHandler for
+// handling download requests.
+std::unique_ptr<ResourceHandler> CreateResourceHandlerForDownload(
+    net::URLRequest* request,
+    bool is_content_initiated,
+    bool must_download) {

[Randy Smith (Not in Mondays), 2016/08/17 15:53:54]

I believe this function needs to be called on the IO thread; willing to add a
DCHECK() to that effect?  (Also for the other functions in this file.)

(The download code runs on four different threads, so I'm a bit paranoid about
being very clear what thread code is supposed to be called on.)

[ananta, 2016/08/17 20:31:36]

Added DCHECK's

+  std::unique_ptr<ResourceHandler> handler(
+      new DownloadResourceHandler(request));
+  if (ResourceDispatcherHostImpl::Get()->delegate()) {
+    const ResourceRequestInfoImpl* request_info(
+        ResourceRequestInfoImpl::ForRequest(request));
+
+    ScopedVector<ResourceThrottle> throttles;
+    ResourceDispatcherHostImpl::Get()->delegate()->DownloadStarting(
+        request, request_info->GetContext(), request_info->GetChildID(),
+        request_info->GetRouteID(), is_content_initiated, must_download,
+        &throttles);
+    if (!throttles.empty()) {
+      handler.reset(new ThrottlingResourceHandler(std::move(handler), request,
+                                                  std::move(throttles)));
+    }
+  }
+  return handler;
+}
+
 std::unique_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread> BeginDownload(
     std::unique_ptr<DownloadUrlParameters> params,
     content::ResourceContext* resource_context,
@@ -73,14 +106,15 @@ std::unique_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread> BeginDownload(
   // ResourceDispatcherHostImpl which will in turn pass it along to the
   // ResourceLoader.
   if (params->render_process_host_id() >= 0) {
-    DownloadInterruptReason reason =
-        ResourceDispatcherHostImpl::Get()->BeginDownload(
-            std::move(url_request), params->referrer(),
-            params->content_initiated(), resource_context,
-            params->render_process_host_id(),
-            params->render_view_host_routing_id(),
-            params->render_frame_host_routing_id(),
-            params->do_not_prompt_for_login());
+    DownloadInterruptReason reason = DownloadManagerImpl::BeginDownloadRequest(

[Randy Smith (Not in Mondays), 2016/08/17 15:53:54]

If this is the only place this function is called (which fits my memory and my
quick scan of this CL), can it be moved to a file static function in this file? 
Or even just hoist the guts into this function?  I don't have any major
objections to a static function on DMI that's called on the IO thread (the class
lives on the UI thread) but I think if it doesn't need to be on DMI it shouldn't
be.

[ananta, 2016/08/18 01:53:25]

This function is also called from resource_dispatcher_host_unittest.cc in the
download tests. I agree with the static function ugliness. However for this use
case we need a way for the function to be accessible outside this file.

[Randy Smith (Not in Mondays), 2016/08/18 15:33:57]

Given that the function is no longer in rdhi.cc, shouldn't the unit test for it
be moved out of rdhu.cc?  (I'm not sure how the unit test affects making it
static to this file, I'll look at that more closely in the next round of
review.)

The merging of the two functions is probably reasonable to leave to the
downloads folks if it requires test re-evaluations, but moving the test makes
sense to me?

+        std::move(url_request),
+        params->referrer(),
+        resource_context,
+        params->content_initiated(),
+        params->render_process_host_id(),
+        params->render_view_host_routing_id(),
+        params->render_frame_host_routing_id(),
+        params->do_not_prompt_for_login());
 
     // If the download was accepted, the DownloadResourceHandler is now
     // responsible for driving the request to completion.
@@ -521,6 +555,79 @@ void DownloadManagerImpl::RemoveUrlDownloader(UrlDownloader* downloader) {
   }
 }
 
+// static
+void DownloadManagerImpl::ResourceDispatcherHostCreated() {
+  ResourceDispatcherHostImpl::Get()->RegisterCreateDownloadHandlerInterceptor(
+      base::Bind(&CreateResourceHandlerForDownload));
+}
+
+// static
+DownloadInterruptReason DownloadManagerImpl::BeginDownloadRequest(
+    std::unique_ptr<net::URLRequest> url_request,
+    const Referrer& referrer,
+    ResourceContext* resource_context,
+    bool is_content_initiated,
+    int render_process_id,
+    int render_view_route_id,
+    int render_frame_route_id,
+    bool do_not_prompt_for_login) {
+  if (ResourceDispatcherHostImpl::Get()->is_shutdown())
+    return DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
+
+  if (referrer.url.is_valid())
+    url_request->SetReferrer(referrer.url.spec());
+
+  // We treat a download as a main frame load, and thus update the policy URL on
+  // redirects.
+  //
+  // TODO(davidben): Is this correct? If this came from a
+  // ViewHostMsg_DownloadUrl in a frame, should it have first-party URL set
+  // appropriately?
+  url_request->set_first_party_url_policy(
+      net::URLRequest::UPDATE_FIRST_PARTY_URL_ON_REDIRECT);
+
+  const GURL& url = url_request->original_url();
+
+  // Check if the renderer is permitted to request the requested URL.
+  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+          render_process_id, url)) {
+    DVLOG(1) << "Denied unauthorized download request for "
+             << url.possibly_invalid_spec();
+    return DOWNLOAD_INTERRUPT_REASON_NETWORK_INVALID_REQUEST;
+  }
+
+  ResourceRequestInfoImpl* extra_info =
+      ResourceDispatcherHostImpl::Get()->CreateRequestInfo(
+          render_process_id,
+          render_view_route_id,
+          render_frame_route_id,
+          true,
+          resource_context);
+  extra_info->set_do_not_prompt_for_login(do_not_prompt_for_login);
+  // Request takes ownership.
+  extra_info->AssociateWithRequest(url_request.get());
+
+  if (url.SchemeIs(url::kBlobScheme) &&
+      !storage::BlobProtocolHandler::GetRequestBlobDataHandle(
+          url_request.get())) {
+    ChromeBlobStorageContext* blob_context =
+        GetChromeBlobStorageContextForResourceContext(resource_context);
+    storage::BlobProtocolHandler::SetRequestedBlobDataHandle(
+        url_request.get(),
+        blob_context->context()->GetBlobDataFromPublicURL(url));
+  }
+
+  // From this point forward, the |DownloadResourceHandler| is responsible for
+  // |started_callback|.
+  std::unique_ptr<ResourceHandler> handler(
+      ResourceDispatcherHostImpl::Get()->CreateResourceHandlerForDownload(
+          url_request.get(), is_content_initiated, true));
+
+  ResourceDispatcherHostImpl::Get()->BeginURLRequest(std::move(url_request),
+                                                     std::move(handler));
+  return DOWNLOAD_INTERRUPT_REASON_NONE;
+}
+
 namespace {
 
 bool EmptyFilter(const GURL& url) {