Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/browser_main_loop.cc

Index: content/browser/browser_main_loop.cc
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
index 7d2eaa5755a987b141094ab619d33f4f241b8996..e2cd7c163658cf4b481c576726fa6242c70a6468 100644
--- a/content/browser/browser_main_loop.cc
+++ b/content/browser/browser_main_loop.cc
@@ -45,6 +45,7 @@
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/device_sensors/device_sensor_service.h"
 #include "content/browser/dom_storage/dom_storage_area.h"
+#include "content/browser/download/download_manager_impl.h"
 #include "content/browser/download/save_file_manager.h"
 #include "content/browser/gamepad/gamepad_service.h"
 #include "content/browser/gpu/browser_gpu_channel_host_factory.h"
@@ -1288,6 +1289,12 @@ int BrowserMainLoop::BrowserThreadsStarted() {
       "BrowserMainLoop::BrowserThreadsStarted:InitResourceDispatcherHost");
     resource_dispatcher_host_.reset(new ResourceDispatcherHostImpl());
     GetContentClient()->browser()->ResourceDispatcherHostCreated();
+    // TODO(ananta)
+    // We register an interceptor on the ResourceDispatcherHostImpl instance to
+    // intercept requests to create handlers for download requests. We need to
+    // find a better way to achieve this. Ideally we don't want knowledge of
+    // downloads in ResourceDispatcherHostImpl.
+    DownloadManagerImpl::ResourceDispatcherHostCreated();

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

I have a slight preference for this reference being to DownloadResourceHandler
instead of DownloadManager{Impl}; DRH is an IO thread object, and DMI a UI
thread object, and the RDH is much closer to DRH architecturally than it is to
DMI.  DMI is what represents the download system to the higher levels, and RDH
is pretty low level.

Having said that, I don't feel strongly about it--if you really want the
reference to be to DMI, I'll accept it (and maybe circle back with Asanka and
jam@ after Asanka gets back).  

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

Why not change the RDHI constructor rather than register an interceptor?  We're
putting in a DCHECK that basically says that the interceptor must always be
there, and passing it into the constructor seems a slightly better way to
enforce that than a DCHECK.  It means this code needs to know the name and
signature of the function used to create the resource handler, but that's
basically |ResourceHandler *func(URLRequest*)|, which doesn't seem too bad to
make visible here.   

I'll note that I've convinced myself in reading this CL that a)
content/browser/loader *does* need to have knowledge of downloads, and that b)
while we could find a way to restrict that knowledge to the resource handlers in
c/b/l and keep it out of RDHI, RDHI is the only obvious place to store any
information long-term; the other objects are not persistent.  So I think RDHI is
likely to need to have knowledge of downloads long-term.  I.e. something in this
space is likely to be what we do long-term, and we may as well make it as clean
as possible.  

Please let me know if you see flaws in this logic; it's not directly relevant to
this CL, but it will affect how I review it (in the whole "I want to see where
we're going to make sure the individual steps move us in that direction" sense).

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

This is a temporary change to get us to the end goal of moving the loader code
out. I think your points about having the DownloadResourceHandler live in the
loader folder may be what we end up doing anyways once we are at that point. We
can leave this as is for now?

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

I'm not sure we're communicating; would you maybe catch me on IM?

Here's a summary of my perspective, but as I say, I'm not sure we're
communicating, so interactive communication would probably be better.
* I understand that the short term goal is moving the loading code out into its
own component.

* I'm a big believer in "temporary solutions aren't", and as a result I'm rarely
willing to allow landing code in the code base that I wouldn't be comfortable
being there forever.  Sometimes I'll do it, but I need a good argument as to why
it's worth doing.  I haven't heard such an argument yet; I totally get that we
have a goal of moving loader code out into a component, but I don't have any
understanding of why that should change my code review evaluations.

* I'm not trying to create major work for you, I'm trying to explore what the
best options are given the short-term goal.  My perception (which may be
incorrect--please let me know if you think it is) is that the changes I'm
suggesting are fairly small ones (+).  So I keep being confused as to why you
push back with "but this is only temporary".  What am I missing?

* More specifically, because I've convinced myself that something like an
interceptor will be necessary long term (in mojo terms, a reference to an
external service) I want to make the interceptor addition as clean as possible
(it's much more likely we'll go in and rip out something that'll need to be
changed than that we'll tweak something that doesn't need to be changed).  Is
there some reason why the interceptor shouldn't be passed to the constructor
instead of doing through a separate setter?

* Is there some reason why we can't use a static function on the
DownloadResourceHandler instead of a static function on the DownloadManagerImpl?
 Again, that seems like a fairly small change?


(+) The exception here is moving the tests.  I understand that that's real work,
and I'm sorry about that, but what I don't understand is why, if we agree that
work needs to be done at some point, it shouldn't be done now to keep the
results of this CL more cohesive.  Again, it feels like there's some context I'm
missing.  Do you have a sense what it is?

 
     loader_delegate_.reset(new LoaderDelegateImpl());
     resource_dispatcher_host_->SetLoaderDelegate(loader_delegate_.get());