Use a DownloadRequestHandle pointer in construction to allow mocking for tests.

BUG=101214


Review URL: http://codereview.chromium.org/8399001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108149 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/download/download_manager_unittest.cc

@@ -183,7 +183,7 @@ DownloadFileWithMockStream::DownloadFileWithMockStream(
     DownloadCreateInfo* info,
     DownloadManager* manager,
     net::testing::MockFileStream* stream)
-    : DownloadFile(info, DownloadRequestHandle(), manager) {
+    : DownloadFile(info, new DownloadRequestHandle(), manager) {
   DCHECK(file_stream_ == NULL);
   file_stream_.reset(stream);
 }
@@ -281,7 +281,7 @@ const struct {
 class MockDownloadFile : public DownloadFile {
  public:
   MockDownloadFile(DownloadCreateInfo* info, DownloadManager* manager)
-      : DownloadFile(info, DownloadRequestHandle(), manager),
+      : DownloadFile(info, new DownloadRequestHandle(), manager),
         renamed_count_(0) { }
   virtual ~MockDownloadFile() { Destructed(); }
   MOCK_METHOD1(Rename, net::Error(const FilePath&));
@@ -397,7 +397,7 @@ TEST_F(DownloadManagerTest, StartDownload) {
     download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
 
     DownloadFile* download_file(
-        new DownloadFile(info.get(), DownloadRequestHandle(),
+        new DownloadFile(info.get(), new DownloadRequestHandle(),
                          download_manager_));
     AddDownloadToFileManager(info->download_id.local(), download_file);
     download_file->Initialize(false);
@@ -758,7 +758,8 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   // name has been chosen, so we need to initialize the download file
   // properly.
   DownloadFile* download_file(
-      new DownloadFile(info.get(), DownloadRequestHandle(), download_manager_));
+      new DownloadFile(info.get(), new DownloadRequestHandle(),
+                       download_manager_));
   download_file->Rename(cr_path);
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);
@@ -834,7 +835,8 @@ TEST_F(DownloadManagerTest, DownloadRemoveTest) {
   // name has been chosen, so we need to initialize the download file
   // properly.
   DownloadFile* download_file(
-      new DownloadFile(info.get(), DownloadRequestHandle(), download_manager_));
+      new DownloadFile(info.get(), new DownloadRequestHandle(),
+                       download_manager_));
   download_file->Rename(cr_path);
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);

content/browser/download/download_file.cc

@@ -24,7 +24,7 @@ static const int kMaxUniqueFiles = 100;
 }
 
 DownloadFile::DownloadFile(const DownloadCreateInfo* info,
-                           const DownloadRequestHandle& request_handle,
+                           DownloadRequestHandleInterface* request_handle,
                            DownloadManager* download_manager)
     : BaseFile(info->save_info.file_path,
                info->url(),
@@ -43,7 +43,7 @@ DownloadFile::~DownloadFile() {
 
 void DownloadFile::CancelDownloadRequest() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  request_handle_.CancelRequest();
+  request_handle_->CancelRequest();
 }
 
 DownloadManager* DownloadFile::GetDownloadManager() {
@@ -58,7 +58,7 @@ std::string DownloadFile::DebugString() const {
                             " Base File = %s"
                             " }",
                             id_.local(),
-                            request_handle_.DebugString().c_str(),
+                            request_handle_->DebugString().c_str(),
                             BaseFile::DebugString().c_str());
 }
 

content/browser/download/download_file.h

@@ -26,8 +26,9 @@ class ResourceDispatcherHost;
 // cancelled, the DownloadFile is destroyed.
 class CONTENT_EXPORT DownloadFile : public BaseFile {
  public:
+  // Takes ownership of the object pointed to by |request_handle|.
   DownloadFile(const DownloadCreateInfo* info,
-               const DownloadRequestHandle& request_handle,
+               DownloadRequestHandleInterface* request_handle,
                DownloadManager* download_manager);
   virtual ~DownloadFile();
 
@@ -67,7 +68,7 @@ class CONTENT_EXPORT DownloadFile : public BaseFile {
 
   // The handle to the request information.  Used for operations outside the
   // download system, specifically canceling a download.
-  DownloadRequestHandle request_handle_;
+  scoped_ptr<DownloadRequestHandleInterface> request_handle_;
 
   // DownloadManager this download belongs to.
   scoped_refptr<DownloadManager> download_manager_;

content/browser/download/download_file_manager.cc

@@ -64,7 +64,9 @@ void DownloadFileManager::CreateDownloadFile(
   scoped_ptr<DownloadCreateInfo> infop(info);
 
   scoped_ptr<DownloadFile>
-      download_file(new DownloadFile(info, request_handle, download_manager));
+      download_file(new DownloadFile(info,
+                                     new DownloadRequestHandle(request_handle),
+                                     download_manager));
   if (net::OK != download_file->Initialize(get_hash)) {
     request_handle.CancelRequest();
     return;

content/browser/download/download_file_unittest.cc

@@ -71,7 +71,8 @@ class DownloadFileTest : public testing::Test {
     // info.request_handle default constructed to null.
     info.save_info.file_stream = file_stream_;
     file->reset(
-        new DownloadFile(&info, DownloadRequestHandle(), download_manager_));
+        new DownloadFile(&info, new DownloadRequestHandle(),
+                         download_manager_));
   }
 
   virtual void DestroyDownloadFile(scoped_ptr<DownloadFile>* file, int offset) {

content/browser/download/download_item.cc

@@ -157,7 +157,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
 // Constructing for a regular download:
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const DownloadCreateInfo& info,
-                           const DownloadRequestHandle& request_handle,
+                           DownloadRequestHandleInterface* request_handle,
                            bool is_otr)
     : state_info_(info.original_name, info.save_info.file_path,
                   info.has_user_gesture, info.transition_type,
@@ -574,9 +574,9 @@ void DownloadItem::TogglePause() {
 
   DCHECK(IsInProgress());
   if (is_paused_)
-    request_handle_.ResumeRequest();
+    request_handle_->ResumeRequest();
   else
-    request_handle_.PauseRequest();
+    request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
@@ -641,7 +641,7 @@ bool DownloadItem::MatchesQuery(const string16& query) const {
   //   L"/\x4f60\x597d\x4f60\x597d",
   //   "/%E4%BD%A0%E5%A5%BD%E4%BD%A0%E5%A5%BD"
   std::string languages;
-  TabContents* tab = request_handle_.GetTabContents();
+  TabContents* tab = request_handle_->GetTabContents();
   if (tab)
     languages = content::GetContentClient()->browser()->GetAcceptLangs(tab);
   string16 url_formatted(net::FormatUrl(GetURL(), languages));
@@ -701,6 +701,12 @@ DownloadPersistentStoreInfo DownloadItem::GetPersistentStoreInfo() const {
                                      opened());
 }
 
+TabContents* DownloadItem::GetTabContents() const {
+  if (request_handle_.get())
+    return request_handle_->GetTabContents();
+  return NULL;
+}
+
 FilePath DownloadItem::GetTargetFilePath() const {
   return full_path_.DirName().Append(state_info_.target_name);
 }
@@ -721,7 +727,7 @@ FilePath DownloadItem::GetUserVerifiedFilePath() const {
 
 void DownloadItem::OffThreadCancel(DownloadFileManager* file_manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle_.CancelRequest();
+  request_handle_->CancelRequest();
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&DownloadFileManager::CancelDownload,

content/browser/download/download_item.h

@@ -22,6 +22,7 @@
 
 #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"
@@ -36,6 +37,8 @@
 class DownloadFileManager;
 class DownloadId;
 class DownloadManager;
+class TabContents;
+
 struct DownloadCreateInfo;
 struct DownloadPersistentStoreInfo;
 
@@ -117,10 +120,11 @@ class CONTENT_EXPORT DownloadItem {
   DownloadItem(DownloadManager* download_manager,
                const DownloadPersistentStoreInfo& info);
 
-  // Constructing for a regular download:
+  // Constructing for a regular download.
+  // Takes ownership of the object pointed to by |request_handle|.
   DownloadItem(DownloadManager* download_manager,
                const DownloadCreateInfo& info,
-               const DownloadRequestHandle& request_handle,
+               DownloadRequestHandleInterface* request_handle,
                bool is_otr);
 
   // Constructing for the "Save Page As..." feature:
@@ -311,9 +315,8 @@ class CONTENT_EXPORT DownloadItem {
 
   DownloadPersistentStoreInfo GetPersistentStoreInfo() const;
   DownloadStateInfo state_info() const { return state_info_; }
-  const DownloadRequestHandle& request_handle() const {
-    return request_handle_;
-  }
+
+  TabContents* GetTabContents() const;
 
   // Returns the final target file path for the download.
   FilePath GetTargetFilePath() const;
@@ -377,7 +380,7 @@ class CONTENT_EXPORT DownloadItem {
 
   // The handle to the request information.  Used for operations outside the
   // download system.
-  DownloadRequestHandle request_handle_;
+  scoped_ptr<DownloadRequestHandleInterface> request_handle_;
 
   // Download ID assigned by DownloadResourceHandler.
   DownloadId download_id_;

content/browser/download/download_manager.cc

@@ -294,8 +294,7 @@ void DownloadManager::RestartDownload(
 
   if (download->prompt_user_for_save_location()) {
     // We must ask the user for the place to put the download.
-    DownloadRequestHandle request_handle = download->request_handle();
-    TabContents* contents = request_handle.GetTabContents();
+    TabContents* contents = download->GetTabContents();
 
     // |id_ptr| will be deleted in either FileSelected() or
     // FileSelectionCancelled().
@@ -317,8 +316,8 @@ void DownloadManager::CreateDownloadItem(
     DownloadCreateInfo* info, const DownloadRequestHandle& request_handle) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DownloadItem* download = new DownloadItem(this, *info,
-                                            request_handle,
+  DownloadItem* download = new DownloadItem(
+      this, *info, new DownloadRequestHandle(request_handle),
       browser_context_->IsOffTheRecord());
   int32 download_id = info->download_id.local();
   DCHECK(!ContainsKey(in_progress_, download_id));
@@ -920,8 +919,7 @@ void DownloadManager::OnDownloadItemAddedToPersistentStore(int32 download_id,
 void DownloadManager::ShowDownloadInBrowser(DownloadItem* download) {
   // The 'contents' may no longer exist if the user closed the tab before we
   // get this start completion event.
-  DownloadRequestHandle request_handle = download->request_handle();
-  TabContents* content = request_handle.GetTabContents();
+  TabContents* content = download->GetTabContents();
 
   // If the contents no longer exists, we ask the embedder to suggest another
   // tab.

content/browser/download/download_request_handle.h

@@ -8,6 +8,7 @@
 
 #include <string>
 
+#include "base/compiler_specific.h"
 #include "content/common/content_export.h"
 
 class DownloadManager;
@@ -18,7 +19,30 @@ class TabContents;
 // or objects conditional on it (e.g. TabContents).
 // This class needs to be copyable, so we can pass it across threads and not
 // worry about lifetime or const-ness.
-class CONTENT_EXPORT DownloadRequestHandle {
+//
+// DownloadRequestHandleInterface is defined for mocking purposes.
+class CONTENT_EXPORT DownloadRequestHandleInterface {
+ public:
+  virtual ~DownloadRequestHandleInterface() {}
+
+  // These functions must be called on the UI thread.
+  virtual TabContents* GetTabContents() const = 0;
+  virtual DownloadManager* GetDownloadManager() const = 0;
+
+  // Pause or resume the matching URL request.
+  virtual void PauseRequest() const = 0;
+  virtual void ResumeRequest() const = 0;
+
+  // Cancel the request.
+  virtual void CancelRequest() const = 0;
+
+  // Describe the object.
+  virtual std::string DebugString() const = 0;
+};
+
+
+class CONTENT_EXPORT DownloadRequestHandle
+    : public DownloadRequestHandleInterface {
  public:
   // Create a null DownloadRequestHandle: getters will return null, and
   // all actions are no-ops.
@@ -35,18 +59,13 @@ class CONTENT_EXPORT DownloadRequestHandle {
                         int render_view_id,
                         int request_id);
 
-  // These functions must be called on the UI thread.
-  TabContents* GetTabContents() const;
-  DownloadManager* GetDownloadManager() const;
-
-  // Pause or resume the matching URL request.
-  void PauseRequest() const;
-  void ResumeRequest() const;
-
-  // Cancel the request
-  void CancelRequest() const;
-
-  std::string DebugString() const;
+  // Implement DownloadRequestHandleInterface interface.
+  virtual TabContents* GetTabContents() const OVERRIDE;
+  virtual DownloadManager* GetDownloadManager() const OVERRIDE;
+  virtual void PauseRequest() const OVERRIDE;
+  virtual void ResumeRequest() const OVERRIDE;
+  virtual void CancelRequest() const OVERRIDE;
+  virtual std::string DebugString() const OVERRIDE;
 
  private:
   // The resource dispatcher host.