DownloadUrlParameters

content/public/browser/download_manager.h

Index: content/public/browser/download_manager.h
diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h
index d67a385a4141f8f2e691a78154debff61ae81479..fc3521baf6f06c6b9de32342475f95b205179cc8 100644
--- a/content/public/browser/download_manager.h
+++ b/content/public/browser/download_manager.h
@@ -35,34 +35,36 @@
 #include "base/callback.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop_helpers.h"
 #include "base/time.h"
+#include "base/values.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
-#include "content/public/browser/browser_thread.h"
-#include "net/base/net_log.h"
+#include "content/public/browser/download_save_info.h"
+#include "googleurl/src/gurl.h"
 #include "net/base/net_errors.h"

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

Why are there so many include files (and other stuff) being added to this file
for the removal of a lot of things and a parameter that can be forward declared?

[benjhayden, 2012/05/02 15:11:54]

Done.

+#include "net/base/net_log.h"
 
 class DownloadRequestHandle;
-class GURL;
 struct DownloadCreateInfo;
 struct DownloadRetrieveInfo;
 
 namespace content {
+
 class BrowserContext;
 class DownloadManagerDelegate;
 class DownloadQuery;
-class WebContents;
-struct DownloadSaveInfo;
+class DownloadUrlParameters;
+class ResourceContext;
+class ResourceDispatcherHostImpl;
 
 // Browser's download manager: manages all downloads and destination view.
 class CONTENT_EXPORT DownloadManager
     : public base::RefCountedThreadSafe<DownloadManager> {
  public:
-  // NOTE: If there is an error, the DownloadId will be invalid.
-  typedef base::Callback<void(DownloadId, net::Error)> OnStartedCallback;

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

Suggestion: Several of the files in this CL have only the change of getting
OnStartedCallback from DownloadUrlParameters instead of DownloadManager.  That's
suggestive that it's at least worth asking the question whether we should leave
the callback defined on DownloadManager (even if used in DownloadUrlParamters);
if there are several locations that only need to know about the callback, should
we be making them include the whole include file?

(It's up to you, but for what it's worth when I ask the question I lean towards
leaving it on DownloadManager, but not strongly.  Looking over the locations
that need it without DownloadUrlParameters, there are two categories: Test files
(which don't count), and download system implementation (which sorta count and
sorta don't).  On the other hand, I have no problem imagining a wider range of
consumers needing the callback than need all the nitty-gritty of the
parameters.)

[benjhayden, 2012/05/02 15:11:54]

It seems strange to define a callback in an interface that doesn't use it. It
seems like that might encourage throwing other stuff that doesn't belong
anywhere in DownloadManager.

-
   virtual ~DownloadManager() {}
 
   static DownloadManager* Create(
@@ -172,26 +174,8 @@ class CONTENT_EXPORT DownloadManager
   // deleted is returned back to the caller.
   virtual int RemoveAllDownloads() = 0;
 
-  // Downloads the content at |url|. |referrer| and |referrer_encoding| are the
-  // referrer for the download, and may be empty. If |prefer_cache| is true,
-  // then if the response to |url| is in the HTTP cache it will be used without
-  // revalidation. If |post_id| is non-negative, then it identifies the post
-  // transaction used to originally retrieve the |url| resource - it also
-  // requires |prefer_cache| to be |true| since re-post'ing is not done.
-  // |save_info| specifies where the downloaded file should be
-  // saved, and whether the user should be prompted about the download.
-  // |web_contents| is the web page that the download is done in context of,
-  // and must be non-NULL.
-  // |callback| will be called when the download starts, or if an error
-  // occurs that prevents a download item from being created.
-  virtual void DownloadUrl(const GURL& url,
-                           const GURL& referrer,
-                           const std::string& referrer_encoding,
-                           bool prefer_cache,
-                           int64 post_id,
-                           const DownloadSaveInfo& save_info,
-                           WebContents* web_contents,
-                           const OnStartedCallback& callback) = 0;
+  // Takes ownership of |parameters|.

[Randy Smith (Not in Mondays), 2012/04/30 19:44:48]

nit, suggestion: "See DownloadUrlParameters for details about controlling the
download."?

[benjhayden, 2012/05/02 15:11:54]

Done.

+  virtual void DownloadUrl(DownloadUrlParameters* parameters) = 0;
 
   // Allow objects to observe the download creation process.
   virtual void AddObserver(Observer* observer) = 0;