Replace the DownloadFileManager with direct ownership of DownloadFileImpl

content/browser/download/download_item_impl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "base/timer.h"
 #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"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 
 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,
+      public content::DownloadDestinationObserver {
  public:
   // Note that it is the responsibility of the caller to ensure that a
   // DownloadItemImplDelegate passed to a DownloadItemImpl constructor
   // outlives the DownloadItemImpl.
 
   // Constructing from persistent store:
   // |bound_net_log| is constructed externally for our use.
   DownloadItemImpl(DownloadItemImplDelegate* delegate,
                    content::DownloadId download_id,
                    const content::DownloadPersistentStoreInfo& info,
@@ skipping 97 matching lines @@
 
   // All remaining public interfaces virtual to allow for DownloadItemImpl
   // mocks.
 
   // Main entry points for regular downloads, in order -------------------------
 
   // TODO(rdsmith): Fold the process that uses these fully into
   // DownloadItemImpl and pass callbacks to the delegate so that all of
   // these can be made private
 
+  // 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
   // |target_path| as determined by the caller. |intermediate_path| is the path
   // to use to store the download until OnDownloadCompleting() is called.
   virtual void OnDownloadTargetDetermined(
       const FilePath& target_path,
       TargetDisposition disposition,
       content::DownloadDangerType danger_type,
       const FilePath& intermediate_path);
 
   // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
   // DownloadItem::Completed().
   virtual void OnDownloadCompleting();
 
   // Needed because of interwining with DownloadManagerImpl --------------------
 
   // TODO(rdsmith): Unwind DownloadManagerImpl and DownloadItemImpl,
   // removing these from the public interface.
 
   // Notify observers that this item is being removed by the user.
   virtual void NotifyRemoved();
 
   virtual void OnDownloadedFileRemoved();
 
-  // Cancels the off-thread aspects of the download (such as
-  // the corresponding URLRequest).
-  // 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();
-
   // Indicate that an error has occurred on the download.
   virtual void Interrupt(content::DownloadInterruptReason reason);
 
+  // Provide a weak pointer reference to a DownloadDestinationObserver
+  // for use by download destinations.
+  base::WeakPtr<content::DownloadDestinationObserver>
+      DestinationObserverAsWeakPtr();
+
+  // For dispatching on whether we're dealing with a SavePackage download.
+  virtual bool IsSavePackageDownload() const;
+
   // DownloadItemImpl routines only needed by SavePackage ----------------------
 
   // Called by SavePackage to set the total number of bytes on the item.
   virtual void SetTotalBytes(int64 total_bytes);
 
   // Indicate progress in saving data to its destination.
   // |bytes_so_far| is the number of bytes received so far.
   // |hash_state| is the current hash state.
   virtual void UpdateProgress(int64 bytes_so_far,
                               int64 bytes_per_sec,
                               const std::string& hash_state);
 
-  virtual void OnAllDataSaved(int64 size, const std::string& final_hash);
+  virtual void OnAllDataSaved(const std::string& final_hash);
 
   // Called by SavePackage to display progress when the DownloadItem
   // should be considered complete.
   virtual void MarkAsComplete();
 
   // Interactions with persistence system --------------------------------------
 
   // TODO(benjhayden): Remove when DownloadHistory becomes an observer.
   virtual void SetIsPersisted();
   virtual void SetDbHandle(int64 handle);
@@ skipping 20 matching lines @@
     // Only incoming transition IN_PROGRESS->
     CANCELLED_INTERNAL,
 
     // An error has interrupted the download.
     // Only incoming transition IN_PROGRESS->
     INTERRUPTED_INTERNAL,
 
     MAX_DOWNLOAD_INTERNAL_STATE,
   };
 
+  // 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;
+
   // Normal progression of a download ------------------------------------------
 
   // These are listed in approximately chronological order.  There are also
   // public methods involved in normal download progression; see
   // the implementation ordering in download_item_impl.cc.
 
   // 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
   // this is.
   void Init(bool active, download_net_logs::DownloadType download_type);
 
+  // Callback from file thread when we initialize the DownloadFile.
+  void OnDownloadFileInitialized(
+      content::DownloadInterruptReason result);
+
   void OnDownloadRenamedToIntermediateName(
       content::DownloadInterruptReason reason, const FilePath& full_path);
 
   // If all pre-requisites have been met, complete download processing, i.e. do
   // internal cleanup, file rename, and potentially auto-open.  (Dangerous
   // downloads still may block on user acceptance after this point.)
   void MaybeCompleteDownload();
 
   void OnDownloadRenamedToFinalName(content::DownloadInterruptReason reason,
                                     const FilePath& full_path);
 
+  void ReleaseDownloadFile();
+
   void OnDownloadFileReleased();
 
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
 
   // Helper routines -----------------------------------------------------------
 
   // Returns true if the download still needs to be renamed to
   // GetTargetFilePath().
   bool NeedsRename() const;
 
-  // 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);
-
   // Call to transition state; all state transitions should go through this.
   void TransitionTo(DownloadInternalState new_state);
 
   // Set the |danger_type_| and invoke obserers if necessary.
   void SetDangerType(content::DownloadDangerType danger_type);
 
   void SetFullPath(const FilePath& new_path);
 
   // Mapping between internal and external states.
   static DownloadState InternalToExternalState(
       DownloadInternalState internal_state);
   static DownloadInternalState ExternalToInternalState(
       DownloadState external_state);
 
   // Debugging routines --------------------------------------------------------
   static const char* DebugDownloadStateString(DownloadInternalState state);
 
+  // Will be false for save package downloads retrieved from the history.
+  // TODO(rdsmith): Replace with a generalized enum for "download source".
+  const bool is_save_package_download_;
+
   // The handle to the request information.  Used for operations outside the
   // download system.
   scoped_ptr<DownloadRequestHandleInterface> request_handle_;
 
   // Download ID assigned by DownloadResourceHandler.
   content::DownloadId download_id_;
 
   // Display name for the download. If this is empty, then the display name is
   // considered to be |target_path_.BaseName()|.
   FilePath display_name_;
@@ skipping 135 matching lines @@
   // when the user closes the shelf before the item has been opened but should
   // be treated as though the user opened it.
   bool opened_;
 
   // Do we actually open downloads when requested?  For testing purposes only.
   bool open_enabled_;
 
   // Did the delegate delay calling Complete on this download?
   bool delegate_delayed_complete_;
 
+  // DownloadFile associated with this download.  Note that this
+  // pointer may only be used or destroyed on the FILE thread.

[benjhayden, 2012/09/24 17:43:27]

So, the scoped_ptr will never delete the DownloadFile when the DownloadItemImpl
goes out of scope.
Should this really be a scoped_ptr?
Would a slightly different kind of magic pointer or a raw pointer be more
appropriate? It looks like you've already built a custom deletion management
mechanism.
If it can't be de-ref'd on the UI thread at all, then perhaps a shim would be
more appropriate?

[Randy Smith (Not in Mondays), 2012/09/26 21:01:05]

Hmmm.  I'm not certain what the right thing to do is here.  What I was looking
for was a way to say "THIS OBJECT IS OWNED BY THE DOWNLOAD ITEM", and
scoped_ptr<> is a generic way to do that.  And yes, the basic "will delete
object when pointer is destroyed" logic isn't very useful here.  

Having said that, I think I'd like to keep it, though I'm open to you pushing
back.  This requires users of the pointer to keep track of ownership; they have
to do a Pass(), or a release(), and you can glance at the code and see the
passing of ownership (to the file thread for deletion in this case).  I think
that has maintainability value even though we're not actually using the
destruction semantics.  WDYT?

+  // This pointer will be non-null only while the DownloadItem is in
+  // the IN_PROGRESS state.
+  scoped_ptr<content::DownloadFile> download_file_;
+
   // Net log to use for this download.
   const net::BoundNetLog bound_net_log_;
 
   base::WeakPtrFactory<DownloadItemImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl);
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_