Replace the DownloadFileManager with direct ownership

content/browser/download/download_item_impl.h

Index: content/browser/download/download_item_impl.h
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 0a64132e6733822860c7eb4a40479a44ece8183b..2b86d2372321a06d0ed91033af2dcd55d271ad4c 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -17,6 +17,7 @@
 #include "content/browser/download/download_net_log_parameters.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/download_destination_controller.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_item.h"
 #include "googleurl/src/gurl.h"
@@ -25,8 +26,14 @@
 
 class DownloadItemImplDelegate;
 
+namespace content {
+class DownloadFile;
+}
+
 // See download_item.h for usage.
-class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
+class CONTENT_EXPORT DownloadItemImpl
+    : public content::DownloadItem,
+      private content::DownloadDestinationController {
  public:
   // Note that it is the responsibility of the caller to ensure that a
   // DownloadItemImplDelegate passed to a DownloadItemImpl constructor
@@ -61,6 +68,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
 
   // Implementation functions (not part of the DownloadItem interface).
 
+  // Start the download
+  virtual void Start(scoped_ptr<content::DownloadFile> download_file);
+
   // Indicate that an error has occurred on the download.
   virtual void Interrupt(content::DownloadInterruptReason reason);
 
@@ -76,7 +86,7 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // call those functions from
   // DownloadManager::FileSelectionCancelled() without doing some
   // rewrites of the DownloadManager queues.
-  virtual void OffThreadCancel(DownloadFileManager* file_manager);
+  virtual void OffThreadCancel();
 
   // Called when the downloaded file is removed.
   virtual void OnDownloadedFileRemoved();
@@ -84,7 +94,7 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
   // DownloadItem::Completed().
-  virtual void OnDownloadCompleting(DownloadFileManager* file_manager);
+  virtual void OnDownloadCompleting();
 
   // Called periodically from the download thread, or from the UI thread
   // for saving packages.
@@ -99,11 +109,16 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   virtual void MarkAsComplete();
 
   // Called when all data has been saved. Only has display effects.
-  virtual void OnAllDataSaved(int64 size, const std::string& final_hash);
+  virtual void OnAllDataSaved(const std::string& final_hash);
 
   // Called by SavePackage to set the total number of bytes on the item.
   virtual void SetTotalBytes(int64 total_bytes);
 
+  // Provide a weak pointer reference to a DownloadDestinationController
+  // for use by download destinations.
+  base::WeakPtr<content::DownloadDestinationController>
+      DestinationControllerAsWeakPtr();
+
   // Overridden from DownloadItem.
   virtual void AddObserver(DownloadItem::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(DownloadItem::Observer* observer) OVERRIDE;
@@ -140,8 +155,7 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   virtual void OnTargetPathSelected(const FilePath& target_path) OVERRIDE;
   virtual void OnContentCheckCompleted(
       content::DownloadDangerType danger_type) OVERRIDE;
-  virtual void OnIntermediatePathDetermined(DownloadFileManager* file_manager,
-                                            const FilePath& path) OVERRIDE;
+  virtual void OnIntermediatePathDetermined(const FilePath& path) OVERRIDE;
   virtual const GURL& GetURL() const OVERRIDE;
   virtual const std::vector<GURL>& GetUrlChain() const OVERRIDE;
   virtual const GURL& GetOriginalUrl() const OVERRIDE;
@@ -196,6 +210,17 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   virtual void SetExternalData(const void* key, ExternalData* data) OVERRIDE;
 
  private:
+  // DownloadDestinationController
+  virtual void DestinationUpdate(int64 bytes_so_far,
+                                 int64 bytes_per_sec,
+                                 const std::string& hash_state) OVERRIDE;
+  virtual void DestinationError(
+      content::DownloadInterruptReason reason) OVERRIDE;
+  virtual void DestinationCompleted(const std::string& final_hash) OVERRIDE;
+
+  // For weak pointer downlcasting.

[benjhayden, 2012/07/25 15:19:16]

I have the same problem! You start typing some word that happens to start with
"down", and before you know it you've already finished typing "load".

We need foot pedals that type "Download" and "download" so that we never have to
type them and our brains can start to heal some of the damage.

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

I couldn't figure out what you were talking about for a moment (I mean, I know
exactly the experience you're talking about, but couldn't figure out how it
related to this line), but you got me LOLing when I did.  Fixed.

+  friend class base::WeakPtr<content::DownloadDestinationController>;
+
   // Construction common to all constructors. |active| should be true for new
   // downloads and false for downloads from the history.
   // |download_type| indicates to the net log system what kind of download
@@ -211,12 +236,6 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // downloads still may block on user acceptance after this point.)
   void MaybeCompleteDownload();
 
-  // Internal helper for maintaining consistent received and total sizes, and
-  // setting the final hash.
-  // Should only be called from |OnAllDataSaved|.
-  void ProgressComplete(int64 bytes_so_far,
-                        const std::string& final_hash);
-
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
@@ -231,8 +250,7 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   void SetFullPath(const FilePath& new_path);
 
   // Callback invoked when the download has been renamed to its final name.
-  void OnDownloadRenamedToFinalName(DownloadFileManager* file_manager,
-                                    content::DownloadInterruptReason reason,
+  void OnDownloadRenamedToFinalName(content::DownloadInterruptReason reason,
                                     const FilePath& full_path);
 
   // Callback invoked when the download has been renamed to its intermediate
@@ -240,6 +258,10 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   void OnDownloadRenamedToIntermediateName(
       content::DownloadInterruptReason reason, const FilePath& full_path);
 
+  // Callback from file thread when we initialize the DownloadFile.
+  void OnDownloadFileInitialized(
+      content::DownloadInterruptReason result);
+
   // Callback from file thread when we release the DownloadFile.
   void OnDownloadFileReleased();
 
@@ -405,6 +427,10 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // are owned by the DownloadItemImpl.
   std::map<const void*, ExternalData*> external_data_map_;
 
+  // DownloadFile associated with this download.  Note that this
+  // pointer may only be used or destroyed on the FILE thread.

[benjhayden, 2012/07/25 15:19:16]

Then maybe it shouldn't be scoped_ptr? scoped_ptr suggests that this member will
be deleted when the owner is deleted.
Is there a generalization of scoped_ptr that affords deletion-on-another-thread?

[Randy Smith (Not in Mondays), 2012/07/30 01:07:23]

Well, I want to strongly indicate the ownership relationship.  I don't think
there's a generalization of scoped_ptr<>, but I do have a DCHECK() in the
destructor of DownloadItem that the scoped_ptr is null at that point.  That good
enough for expressing ownership but warding off wrong-thread destruction?

+  scoped_ptr<content::DownloadFile> download_file_;
+
   // Net log to use for this download.
   const net::BoundNetLog bound_net_log_;