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:
+  // Delegate is defined in DownloadItemImpl (rather than DownloadItem)
+  // as it's relevant to the class implementation (class methods need to
+  // call into it) and doesn't have anything to do with its interface.
+  // Despite this, the delegate methods take DownloadItems as arguments
+  // (rather than DownloadItemImpls) so that classes that inherit from it
+  // can be used with DownloadItem mocks rather than being tied to
+  // DownloadItemImpls.
+  class Delegate {
+   public:
+    Delegate();
+    virtual ~Delegate();
+
+    // Used for catching use-after-free errors.
+    void Attach();

[cbentzel, 2011/12/02 20:12:56]

I wonder if we want to keep these around for the long-term or just for now to
see if any new problems.

[Randy Smith (Not in Mondays), 2011/12/02 20:26:29]

Just as much on the fence as I was about putting them in :-}.  We'll see what
happens; if they don't show anything up in the next couple of weeks, they're
probably not worth it, and I'll probably take them out.

+    void Detach();
+
+    // 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;
+
+    // Checks whether a downloaded file still exists and updates the
+    // file's state if the file is already removed.
+    // The check may or may not result in a later asynchronous call
+    // to OnDownloadedFileRemoved().
+    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;
+   private:

[cbentzel, 2011/12/02 20:12:56]

Nit: add an extra newline between AssertStateConsistent and private. 

[Randy Smith (Not in Mondays), 2011/12/02 20:26:29]

Done.

+    // For "Outlives attached DownloadItemImpl" invariant assertion.
+    int count_;
+  };
+
+  // Note that it is the responsibility of the caller to ensure that a
+  // DownloadItemImpl::Delegate passed to a DownloadItemImpl constructor
+  // outlives the DownloadItemImpl.
+
   // 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;
   virtual void UpdateObservers() OVERRIDE;
   virtual bool CanShowInFolder() OVERRIDE;
   virtual bool CanOpenDownload() OVERRIDE;
   virtual bool ShouldOpenFileBasedOnExtension() OVERRIDE;
   virtual void OpenDownload() OVERRIDE;
   virtual void ShowDownloadInShell() OVERRIDE;
   virtual void DangerousDownloadValidated() OVERRIDE;
   virtual void UpdateProgress(int64 bytes_so_far, int64 bytes_per_sec) OVERRIDE;
   virtual void Cancel(bool user_cancel) OVERRIDE;
   virtual void MarkAsComplete() OVERRIDE;
   virtual void DelayedDownloadOpened() OVERRIDE;
   virtual void OnAllDataSaved(
       int64 size, const std::string& final_hash) OVERRIDE;
   virtual void OnDownloadedFileRemoved() OVERRIDE;
+  virtual void MaybeCompleteDownload() OVERRIDE;
   virtual void Interrupted(int64 size, InterruptReason reason) OVERRIDE;
   virtual void Delete(DeleteReason reason) OVERRIDE;
   virtual void Remove() OVERRIDE;
   virtual bool TimeRemaining(base::TimeDelta* remaining) const OVERRIDE;
   virtual int64 CurrentSpeed() const OVERRIDE;
   virtual int PercentComplete() const OVERRIDE;
   virtual void OnPathDetermined(const FilePath& path) OVERRIDE;
   virtual bool AllDataSaved() const OVERRIDE;
   virtual void SetFileCheckResults(const DownloadStateInfo& state) OVERRIDE;
   virtual void Rename(const FilePath& full_path) OVERRIDE;
@@ skipping 21 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 91 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_