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 04326fccffc96dc17b320b2b9004ea0295abf123..7d991b1d82f46d2d3b907d165a399201832ba57f 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -24,29 +24,80 @@
 #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

[cbentzel, 2011/12/02 18:00:52]

"Interface to delegate to which" is confusing.

Perhaps this whole paragraph is unneeded since Delegate is a well-known term
throughout the Chrome code base. You could just adjust the next paragraph to be 

"Delegat is defined in DownloadItemImpl...."

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

Done.

+  // it doesn't know how to perform.
+  //
+  // Placed 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:
+    virtual ~Delegate() {}
+
+    // Tests if a file type should be opened automatically.
+    virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) = 0;

[cbentzel, 2011/12/02 18:00:52]

Probably for another CL - but seems like this should just pass the extension in
rather than the full path. 

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

Noted for another CL (this signature already existed and I'm just extending it
into a new area).

+
+    // 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 runs in
+    // the background and may finish asynchronously after this method returns.
+    virtual void CheckForFileRemoval(DownloadItem* download_item) = 0;

[cbentzel, 2011/12/02 18:00:52]

Is it expected that this will call back into OnDownloadFileRemoved
asynchronously? Should that be documented? 

[cbentzel, 2011/12/02 18:00:52]

Could this move to a const DownloadItem* argument? I looked at
DownloadManagerImpl's implementation and even though it takes a non-const it
looks like it could take a const easily.

It looks like const DownloadItem* may be reasonable for other members here. I
understand if you want to table that to another CL, as it may require a lot of
plumbing changes.

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

Done.

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

Yeah; I think I want to put this one of into another CL; doing a non-const to
const transformation often has a *lot* of dominos falling before it's done.

+
+    // 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.

[cbentzel, 2011/12/02 18:00:52]

Should these move to DownloadItem::Observer?

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

Architecturally, definitely.  Practically, I'm wary of doing so just because
that code is right at the center of the timing/race issues that we've run into,
and thus I consider the change higher risk than anything else I've done here. 
But I'd definitely like to see it happen.

+    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;
+  };
+
+  // Note that it is the responsibility of the caller to ensure that a
+  // DownloadItemImpl::Delegate passed to a DownloadItemImpl constructor
+  // outlive the DownloadItemImpl.

[cbentzel, 2011/12/02 18:00:52]

Nit: outlives? 

Should you put some debug checks in to make sure this is actually upheld?

For example:

class Delegate {
 public:
  Delegate() : count_(0) {}
  void Attach() { ++count_; }
  void Detach() { DCHECK(count_ > 0); --count_; }
  virtual ~Delegate() { DCHECK(count_ == 0); } 
 private:
  int count_;  
};

class DownloadItemImpl {
 public:
  DownloadItemImpl(Delegate* delegate) {
    delegate_ = delegate;
    delegate_->Attach();
  }
  virtual ~DownloadItemImpl() {
    delegate_->Detach();
  }
}

If the test coverage is good this may not be worth it because the memory bots
should catch the use-after-free problems. But may be worth considering.

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

Done.
I thought about this for a while, and flip-flopped since I didn't see a way to
just keep it in the delegate class, but eventually did it.  Do you think it's ok
to have the extra variable in the class in release, or should I throw in a whole
lot of (ugly :-}) if !defined(NDEBUG)s?

+
   // 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,
@@ -71,6 +122,7 @@ class CONTENT_EXPORT DownloadItemImpl : public DownloadItem {
   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;
@@ -112,7 +164,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 +185,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;
@@ -245,8 +297,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_;