DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_item_impl.h

 // Copyright (c) 2011 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_
 #pragma once
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_state_info.h"
 #include "content/browser/download/interrupt_reasons.h"
 #include "content/common/content_export.h"
 #include "googleurl/src/gurl.h"
 #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,
                    DownloadId download_id);
 
   virtual ~DownloadItemImpl();
 
   // Overridden from DownloadItem.
   virtual void AddObserver(DownloadItem::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(DownloadItem::Observer* observer) OVERRIDE;
@@ skipping 45 matching lines @@
   virtual int64 GetTotalBytes() const OVERRIDE;
   virtual void SetTotalBytes(int64 total_bytes) OVERRIDE;
   virtual const std::string& GetHash() const OVERRIDE;
   virtual int64 GetReceivedBytes() const OVERRIDE;
   virtual int32 GetId() const OVERRIDE;
   virtual DownloadId GetGlobalId() const OVERRIDE;
   virtual base::Time GetStartTime() const OVERRIDE;
   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;
   virtual bool GetFileExternallyRemoved() const OVERRIDE;
   virtual SafetyState GetSafetyState() const OVERRIDE;
   virtual DownloadStateInfo::DangerType GetDangerType() const OVERRIDE;
   virtual bool IsDangerous() const OVERRIDE;
   virtual void MarkFileDangerous() OVERRIDE;
   virtual void MarkUrlDangerous() OVERRIDE;
   virtual void MarkContentDangerous() OVERRIDE;
   virtual bool GetAutoOpened() OVERRIDE;
   virtual const FilePath& GetTargetName() const OVERRIDE;
   virtual bool PromptUserForSaveLocation() const OVERRIDE;
   virtual bool IsOtr() const OVERRIDE;
   virtual const FilePath& GetSuggestedPath() const OVERRIDE;
   virtual bool IsTemporary() const OVERRIDE;
   virtual void SetOpened(bool opened) OVERRIDE;
   virtual bool GetOpened() const OVERRIDE;
   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;
   virtual FilePath GetUserVerifiedFilePath() const OVERRIDE;
   virtual bool NeedsRename() const OVERRIDE;
   virtual void OffThreadCancel(DownloadFileManager* file_manager) OVERRIDE;
   virtual std::string DebugString(bool verbose) const OVERRIDE;
   virtual void MockDownloadOpenForTesting() OVERRIDE;
 
  private:
@@ skipping 88 matching lines @@
 
   // Time the download was started
   base::Time start_time_;
 
   // Time the download completed
   base::Time end_time_;
 
   // 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_;
 
   // A flag for indicating if the download should be opened at completion.
   bool open_when_complete_;
 
   // A flag for indicating if the downloaded file is externally removed.
   bool file_externally_removed_;
 
@@ skipping 25 matching lines @@
   // only.
   bool open_enabled_;
 
   // Did the delegate delay calling Complete on this download?
   bool delegate_delayed_complete_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl);
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_IMPL_H_