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

[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,
                    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_