DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

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 e19080170e19f46e57e3fc72682bda4bbf667565..624ea0c1cc63216a8fe051a60c3f9ac982739560 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -24,29 +24,68 @@
 #include "net/base/net_errors.h"
 
 class DownloadFileManager;
-class DownloadId;
-class DownloadManager;
 class TabContents;
 
 struct DownloadCreateInfo;
 struct DownloadPersistentStoreInfo;
 
+namespace content {
+class BrowserContext;
+}
+
 // See download_item.h for usage.
 class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
  public:
+  // Interface to delegate to which DownloadItemImpl requests operations
+  // it doesn't know how to perform.
+  class Delegate {

[cbentzel, 2011/11/29 21:30:44]

There's no documentation about lifetime expectations of the delegate relative to
the DownloadItemImpl.

It looks like it's expected to outlive the DownloadItemImpl since it's passed
into the constructor, retained as a raw pointer, and no set overrides. Would be
good to mention that though.

Should you do this, or should you have a way for the delegate to unregister?

[ahendrickson, 2011/11/29 21:40:15]

Should this be part of DownloadItem instead of DownloadItemImpl?

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

My intent was to specify that the delegate must outlive the item; I'll put that
in.  I'm going to put the documentation at the constructor, which IMO is the
interface that the contract applies to.

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

Nope.  It's irrelevant to the DownloadItem interface.  Specifically, if someone
wants to construct something that exports the DownloadItem interface (e.g. a
mock), there's no need for a delegate.  The currently implementation of
DownloadItem (i.e. DownloadItemImpl) is what's dependent on the delegate, so the
delegate definition belongs in the impl.

+   public:
+    virtual ~Delegate() {}
+
+    // Tests if a file type should be opened automatically.
+    virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) = 0;
+
+    // Allows the delegate to override the opening of a download. If it returns
+    // true then it's reponsible for opening the item.
+    virtual bool ShouldOpenDownload(DownloadItem* download) = 0;

[cbentzel, 2011/11/29 21:30:44]

It's a little strange that the DownloadItemImpl::Delegate passes a DownloadItem*
in instead of a DownloadItemImpl* to many of the arguments. Does that indicate
that the delegate belongs in DownloadItem instead?

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

As I indicated to Andy above, I don't think so.  I agree that passing a
DownloadItem is weird, but I think (as we discussed offline) that where I'm
ending up is: the Delegate should be on the DownloadItemImpl since it's relevant
to the implementation of DownloadItemImpl, not to its interface (which is
DownloadItem), but I will leave the arguments to the delegate to DownloadItem's
so that classes that inherit from this interface may be used with other classes
(e.g. mocks) inherited from DownloadItem.  I'll put in a comment to that effect.

+
+    // Checks whether a downloaded file still exists and updates the
+    // file's state if the file is already removed. The check runs in
+    // the background and may finish asynchronously after this method returns.
+    virtual void CheckForFileRemoval(DownloadItem* download_item) = 0;
+
+    // If all pre-requisites have been met, complete download processing.
+    // TODO(rdsmith): Move into DownloadItem.
+    virtual void MaybeCompleteDownload(DownloadItem* download) = 0;
+
+    // For contextual issues like language and prefs.
+    virtual content::BrowserContext* BrowserContext() const = 0;
+
+    // Handle any delegate portions of a state change operation on the
+    // DownloadItem.
+    virtual void DownloadCancelled(DownloadItem* download) = 0;
+    virtual void DownloadCompleted(DownloadItem* download) = 0;
+    virtual void DownloadOpened(DownloadItem* download) = 0;
+    virtual void DownloadRemoved(DownloadItem* download) = 0;
+
+    // Assert consistent state for delgate object at various transitions.
+    virtual void AssertStateConsistent(DownloadItem* download) const = 0;
+  };
+
   // Constructing from persistent store:
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
+                   DownloadId download_id,
                    const DownloadPersistentStoreInfo& info);
 
   // Constructing for a regular download.
   // Takes ownership of the object pointed to by |request_handle|.
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
                    const DownloadCreateInfo& info,
                    DownloadRequestHandleInterface* request_handle,
                    bool is_otr);
 
   // Constructing for the "Save Page As..." feature:
-  DownloadItemImpl(DownloadManager* download_manager,
+  DownloadItemImpl(Delegate* delegate,
                    const FilePath& path,
                    const GURL& url,
                    bool is_otr,
@@ -112,7 +151,6 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual base::Time GetEndTime() const OVERRIDE;
   virtual void SetDbHandle(int64 handle) OVERRIDE;
   virtual int64 GetDbHandle() const OVERRIDE;
-  virtual DownloadManager* GetDownloadManager() OVERRIDE;
   virtual bool IsPaused() const OVERRIDE;
   virtual bool GetOpenWhenComplete() const OVERRIDE;
   virtual void SetOpenWhenComplete(bool open) OVERRIDE;
@@ -134,6 +172,7 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   virtual InterruptReason GetLastReason() const OVERRIDE;
   virtual DownloadPersistentStoreInfo GetPersistentStoreInfo() const OVERRIDE;
   virtual DownloadStateInfo GetStateInfo() const OVERRIDE;
+  virtual content::BrowserContext* BrowserContext() const OVERRIDE;
   virtual TabContents* GetTabContents() const OVERRIDE;
   virtual FilePath GetTargetFilePath() const OVERRIDE;
   virtual FilePath GetFileNameToReportUser() const OVERRIDE;
@@ -242,8 +281,8 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   // Our persistent store handle
   int64 db_handle_;
 
-  // Our owning object
-  DownloadManager* download_manager_;
+  // Our delegate.
+  Delegate* delegate_;
 
   // In progress downloads may be paused by the user, we note it here
   bool is_paused_;