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..03add30d1c732d9f1bfbf37381e1af22ab5c6bc2 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,40 @@
 #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) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  std::unique_ptr<ResourceHandler> handler(
+      new DownloadResourceHandler(request));
+  if (ResourceDispatcherHostImpl::Get()->delegate()) {

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

I don't think it makes sense to move this call and the below delegate call out
of RDHI--the delegate is *for* RDHI to call, and no-one else.  So if you move
CreateResourceHandlerForDownload out you should also move these methods off
ResourceDispatcherHostDelegate and onto the DownloadManagerDelegate (though
that's problematic because the DownloadManager is a UI thread object *and* it's
not clear the RDHI can figure out the correct DownloadManager at the point this
function is called).  All of which brings me back to "Why can't we just push all
this logic into the ResourceDispatcherHostDelegate?"

[ananta, 2016/08/18 22:27:09]

Please correct me if I am wrong here. The ResourceDispatcherHostDelegate takes
on
a variety of forms, one of them being in Chrome. The RDH delegate mechanism
allows content to get into the embedder (chrome). The embedder can only get to
content via public interfaces. Till now downloads, save file requests were fully
handled in content. I don't see how we can push the
CreateResourceHandlerForDownload onto the
delegate. While the interceptor approach is hacky at best, it is not intended by
a live for ever thing. Once we hash out how the public interface to RDH is going
to look like after it moves out of content, we can revisit this.

[Randy Smith (Not in Mondays), 2016/08/21 23:32:57]

No, that's right.  Another way of putting this is that ResourceHandler isn't a
class that's visible to Chrome, so Chrome can't create an instance of it.  Sorry
I missed this on my first pass.

But I'd still like to have a clear sense of the overall plan in this space.  As
I understand it, the short term goal that this CL is driving towards is moving
content/browser/loader out of content and into a component, with the goal beyond
that one being to wrap content/browser/loader in a mojo interface.  Is your
thought that we'll maintain the interceptor registration architecture through
the componentization of content/browser/loader, and then change to something
else for the mojo wrapping?  If so, could you give me a sense as to what the
something else would look like?  

It seems as if ResourceHandlers are going to stay a core part of the
content/browser/loader architecture, so I'd think that (after mojoization)
anything that created a ResourceHandler would have to be *inside*
content/browser/loader.  So that would argue for pulling the information about
downloads and the download resource handler into content/browser/loader, and
finding some other place within the downloads system to make the architectural
cut.  Do you have a different image of what mojoization might look like?

("The interceptor is what is planned for componentization and I don't yet know
what we'll do for mojoization" is a fine response, I just want to make sure
that's the status rather than assuming it.)

[ananta, 2016/08/22 19:14:57]

It does seem plausible that we may want to have DownloadResourceHandler or a
variant of it live inside the content/browser/loader folder. The
download_resource_handler.h/.cc files pull in other files from the
content/browser/download folder. If it is ok with you we can do this in a second
pass once we have the code in content/browser/loader ready to be moved out. In
any case the interface of RDHI or whatever we chose to call it once it moves it
is by no means ironed out. The interceptor is just an initial step to help us
move the loader code out.

[ananta, 2016/08/23 04:27:38]

Discussed this a bit with jam. The goal of the network service work is to come
up with a higher level API for issuing/handling network requests. Clients of
this service could be chrome, blink, content, etc. In this context downloads
could be a client of this network service. Additionally the downloads code today
spans content and chrome and it may make sense for this code to live in the same
process (browser) for performance reasons. The network service eventually will
move to its own process.

[Randy Smith (Not in Mondays), 2016/08/23 19:54:45]

It's ok with me--I just want to have a clear idea where we're going.

[Randy Smith (Not in Mondays), 2016/08/23 19:54:45]

So this strikes me as orthogonal to the issues we were discussing.  I'm not
saying that the entire downloads system should be included in
content/browser/loader, just that content/browser/loader needs to have some
*concept* of downloads; that (given Content-Disposition: attachment) there's no
other choice.  In a mojo context, this might mean that there's some downloads
service that the content/browser/loader has a reference to.  The reason I raise
this is that if content/browser/loader needs to have some concept of downloads,
having an interceptor to call is a fine way to manage it; it's naturally
parallel to having a reference to a service in mojo context (AIUI).  

If this *doesn't* fit with your image of where we're eventually going to go with
the c/b/l <-> downloads interface, could you sketch out how you think we'll
handle Content-Disposition: attachment + Content-Type that there's no registered
plugin for?  Just at a high level.  

(This issue doesn't need to be resolved for this CL.)

+    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 +108,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(
+        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 +557,77 @@ 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())

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

Can this be pulled down into RDHI::BeginURLRequest, maybe by making that
function have a return value?  That would avoid accessing what's arguably a
variable that should be local to RDHI.

[ananta, 2016/08/18 22:27:09]

Done.

[Randy Smith (Not in Mondays), 2016/08/21 23:32:57]

I apologize, but I'd like to take this request back.  I'd rather have a
"is_shutdown()" boolean on RDHI than create a new type to be filled in with a
single enum (shutdown).  I had imagined we could return a boolean from the
BeginURLRequest function, but an actual whole enum I think is uglier than just
having this accessor.  Sorry/thanks ...

(Keeping RRII inside of RDHI strikes me as goodness I'd like to keep, but I
think that's orthogonal to whether BeginURLRequest returns an enum.)

[ananta, 2016/08/22 19:14:57]

Sure thing. Reverted to BeginURLRequest being a void function.

+    return DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
+
+  // 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);

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

Can these simply be added to the interface for BeginURLRequest? 
ResourceRequestInfoImpl usage is currently basically restricted to
content/browser/loader, and widening its exposure seems like something to avoid
if we can.  Just the fact that it's an Impl class suggests that :-J.

[ananta, 2016/08/18 22:27:09]

Done. I left the core of the logic in BeginURLRequest in the old
BeginRequestInternal function which has come back.

+  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));

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

It feels broken for download code to delegate creation of the download resource
handler to another class which just calls back into a function in this file in
order to pass the created handler into BeginURLRequest.  Would you add a TODO
comment to clean this up?  If we leave CreateResourceHandlerForDownload() in
this file (which I still think is a mistake) I'd vote in favor of just calling
it directly.

[ananta, 2016/08/18 22:27:09]

Added a TODO here. The CreateResourceHandlerForDownload function in this file
only
creates the download handler. The rest of the logic of notifying the delegate
and creating a throttled handler etc has moved back to the caller in
ResourceDispatcherHostImpl. 

+
+  ResourceDispatcherHostImpl::Get()->BeginURLRequest(std::move(url_request),
+                                                     referrer,
+                                                     std::move(handler));
+  return DOWNLOAD_INTERRUPT_REASON_NONE;
+}
+
 namespace {
 
 bool EmptyFilter(const GURL& url) {