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 da652e79742199d7a115a112aa9f5fdbca4e8566..6304d0a264ad6e50abb920cf83ee07cffb23e9a0 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_observer.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::DownloadDestinationObserver {

[jam, 2012/08/06 18:16:37]

nit: this is against the style guide:
"All inheritance should be public. If you want to do private inheritance, you
should be including an instance of the base class as a member instead."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

in this case, the convention is to use public inheritance, but put the
overridden methods in private. it's possible to cast to the parent to get to
them, of course, but that's a strong indication that something wrong is being
done.

[Randy Smith (Not in Mondays), 2012/08/10 20:09:00]

I had a moment of wanting to do exactly that, but I really need a weak pointer
to the observer, and I want to do that through a factory rather than
SupportsWeakPtr as per recent chromium-dev discussions, so ....  Done.

  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);
+
   // Called when the target path has been determined. |target_path| is the
   // suggested target path. |disposition| indicates how the target path should
   // be used (see TargetDisposition). |danger_type| is the danger level of
@@ -81,14 +91,6 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   // Set the item's DB handle.
   virtual void SetDbHandle(int64 handle);
 
-  // Cancels the off-thread aspects of the download.
-  // TODO(rdsmith): This should be private and only called from
-  // DownloadItem::Cancel/Interrupt; it isn't now because we can't
-  // call those functions from
-  // DownloadManager::FileSelectionCancelled() without doing some
-  // rewrites of the DownloadManager queues.
-  virtual void OffThreadCancel();
-
   // Called when the downloaded file is removed.
   virtual void OnDownloadedFileRemoved();
 
@@ -110,11 +112,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 DownloadDestinationObserver
+  // for use by download destinations.
+  base::WeakPtr<content::DownloadDestinationObserver>
+      DestinationObserverAsWeakPtr();
+
   // Notify observers that this item is being removed by the user.
   virtual void NotifyRemoved();
 
@@ -203,6 +210,17 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   virtual void SetExternalData(const void* key, ExternalData* data) OVERRIDE;
 
  private:
+  // DownloadDestinationObserver
+  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 downcasting.
+  friend class base::WeakPtr<content::DownloadDestinationObserver>;
+
   // 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
@@ -218,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();
@@ -246,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();
 
@@ -411,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.
+  scoped_ptr<content::DownloadFile> download_file_;
+
   // Net log to use for this download.
   const net::BoundNetLog bound_net_log_;