Added ability to set ChromeDownloadManagerDelegate for testing.

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

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

9 files changed, 216 insertions, 132 deletions

diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h
index d371fb4..b006e53 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.h
+++ b/chrome/browser/download/chrome_download_manager_delegate.h
@@ -83,9 +83,16 @@ class ChromeDownloadManagerDelegate
   DownloadPrefs* download_prefs() { return download_prefs_.get(); }
   DownloadHistory* download_history() { return download_history_.get(); }
 
+ protected:
+  // So that test classes can inherit from this for override purposes.
+  virtual ~ChromeDownloadManagerDelegate();
+
+  // So that test classes that inherit from this for override purposes
+  // can call back into the DownloadManager.
+  scoped_refptr<DownloadManager> download_manager_;
+
  private:
   friend class base::RefCountedThreadSafe<ChromeDownloadManagerDelegate>;
-  virtual ~ChromeDownloadManagerDelegate();
 
   // NotificationObserver implementation.
   virtual void Observe(int type,
@@ -128,7 +135,6 @@ class ChromeDownloadManagerDelegate
   void CheckDownloadHashDone(int32 download_id, bool is_dangerous_hash);
 
   Profile* profile_;
-  scoped_refptr<DownloadManager> download_manager_;
   scoped_ptr<DownloadPrefs> download_prefs_;
   scoped_ptr<DownloadHistory> download_history_;
 
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 20a355c..060e756 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/test/test_file_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_prefs.h"
@@ -490,6 +491,138 @@ class CancelTestDataCollector
   DISALLOW_COPY_AND_ASSIGN(CancelTestDataCollector);
 };
 
+class PickSuggestedFileDelegate : public ChromeDownloadManagerDelegate {
+ public:
+  explicit PickSuggestedFileDelegate(Profile* profile)
+      : ChromeDownloadManagerDelegate(profile) {
+    SetDownloadManager(profile->GetDownloadManager());
+  }
+
+  virtual void ChooseDownloadPath(TabContents* tab_contents,
+                                  const FilePath& suggested_path,
+                                  void* data) OVERRIDE {
+    if (download_manager_)
+      download_manager_->FileSelected(suggested_path, data);
+  }
+};
+
+// Get History Information.
+class DownloadsHistoryDataCollector {
+ public:
+  DownloadsHistoryDataCollector(int64 download_db_handle,
+                                DownloadManager* manager)
+      : result_valid_(false),
+        download_db_handle_(download_db_handle) {
+    HistoryService* hs =
+        Profile::FromBrowserContext(manager->browser_context())->
+            GetHistoryService(Profile::EXPLICIT_ACCESS);
+    DCHECK(hs);
+    hs->QueryDownloads(
+        &callback_consumer_,
+        NewCallback(this,
+                    &DownloadsHistoryDataCollector::OnQueryDownloadsComplete));
+
+    // TODO(rdsmith): Move message loop out of constructor.
+    // Cannot complete immediately because the history backend runs on a
+    // separate thread, so we can assume that the RunMessageLoop below will
+    // be exited by the Quit in OnQueryDownloadsComplete.
+    ui_test_utils::RunMessageLoop();
+  }
+
+  bool GetDownloadsHistoryEntry(DownloadPersistentStoreInfo* result) {
+    DCHECK(result);
+    *result = result_;
+    return result_valid_;
+  }
+
+ private:
+  void OnQueryDownloadsComplete(
+      std::vector<DownloadPersistentStoreInfo>* entries) {
+    result_valid_ = false;
+    for (std::vector<DownloadPersistentStoreInfo>::const_iterator it =
+             entries->begin();
+         it != entries->end(); ++it) {
+      if (it->db_handle == download_db_handle_) {
+        result_ = *it;
+        result_valid_ = true;
+      }
+    }
+    MessageLoopForUI::current()->Quit();
+  }
+
+  DownloadPersistentStoreInfo result_;
+  bool result_valid_;
+  int64 download_db_handle_;
+  CancelableRequestConsumer callback_consumer_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsHistoryDataCollector);
+};
+
+// Mock that simulates a permissions dialog where the user denies
+// permission to install.  TODO(skerner): This could be shared with
+// extensions tests.  Find a common place for this class.
+class MockAbortExtensionInstallUI : public ExtensionInstallUI {
+ public:
+  MockAbortExtensionInstallUI() : ExtensionInstallUI(NULL) {}
+
+  // Simulate a user abort on an extension installation.
+  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
+    delegate->InstallUIAbort(true);
+    MessageLoopForUI::current()->Quit();
+  }
+
+  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
+  virtual void OnInstallFailure(const std::string& error) {}
+};
+
+// Mock that simulates a permissions dialog where the user allows
+// installation.
+class MockAutoConfirmExtensionInstallUI : public ExtensionInstallUI {
+ public:
+  explicit MockAutoConfirmExtensionInstallUI(Profile* profile)
+      : ExtensionInstallUI(profile) {}
+
+  // Proceed without confirmation prompt.
+  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
+    delegate->InstallUIProceed();
+  }
+
+  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
+  virtual void OnInstallFailure(const std::string& error) {}
+};
+
+}  // namespace
+
+// While an object of this class exists, it will mock out download
+// opening for all downloads created on the specified download manager.
+class MockDownloadOpeningObserver : public DownloadManager::Observer {
+ public:
+  explicit MockDownloadOpeningObserver(DownloadManager* manager)
+      : download_manager_(manager) {
+    download_manager_->AddObserver(this);
+  }
+
+  ~MockDownloadOpeningObserver() {
+    download_manager_->RemoveObserver(this);
+  }
+
+  // DownloadManager::Observer
+  virtual void ModelChanged() {
+    std::vector<DownloadItem*> downloads;
+    download_manager_->SearchDownloads(string16(), &downloads);
+
+    for (std::vector<DownloadItem*>::iterator it = downloads.begin();
+         it != downloads.end(); ++it) {
+      (*it)->TestMockDownloadOpen();
+    }
+  }
+
+ private:
+  DownloadManager* download_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver);
+};
+
 class DownloadTest : public InProcessBrowserTest {
  public:
   enum SelectExpectation {
@@ -814,128 +947,27 @@ class DownloadTest : public InProcessBrowserTest {
     EXPECT_EQ(expected, BrowserList::size());
   }
 
- private:
-  // Location of the test data.
-  FilePath test_dir_;
+  // Arrange for select file calls on the given browser from the
+  // download manager to always choose the suggested file.
+  void NullSelectFile(Browser* browser) {
+    PickSuggestedFileDelegate* new_delegate =
+        new PickSuggestedFileDelegate(browser->profile());
 
-  // Location of the downloads directory for these tests
-  ScopedTempDir downloads_directory_;
-};
-
-// Get History Information.
-class DownloadsHistoryDataCollector {
- public:
-  DownloadsHistoryDataCollector(int64 download_db_handle,
-                                DownloadManager* manager)
-      : result_valid_(false),
-        download_db_handle_(download_db_handle) {
-    HistoryService* hs =
-        Profile::FromBrowserContext(manager->browser_context())->
-            GetHistoryService(Profile::EXPLICIT_ACCESS);
-    DCHECK(hs);
-    hs->QueryDownloads(
-        &callback_consumer_,
-        NewCallback(this,
-                    &DownloadsHistoryDataCollector::OnQueryDownloadsComplete));
-
-    // Cannot complete immediately because the history backend runs on a
-    // separate thread, so we can assume that the RunMessageLoop below will
-    // be exited by the Quit in OnQueryDownloadsComplete.
-    ui_test_utils::RunMessageLoop();
-  }
-
-  bool GetDownloadsHistoryEntry(DownloadPersistentStoreInfo* result) {
-    DCHECK(result);
-    *result = result_;
-    return result_valid_;
-  }
-
- private:
-  void OnQueryDownloadsComplete(
-      std::vector<DownloadPersistentStoreInfo>* entries) {
-    result_valid_ = false;
-    for (std::vector<DownloadPersistentStoreInfo>::const_iterator it =
-             entries->begin();
-         it != entries->end(); ++it) {
-      if (it->db_handle == download_db_handle_) {
-        result_ = *it;
-        result_valid_ = true;
-      }
-    }
-    MessageLoopForUI::current()->Quit();
-  }
-
-  DownloadPersistentStoreInfo result_;
-  bool result_valid_;
-  int64 download_db_handle_;
-  CancelableRequestConsumer callback_consumer_;
-
-  DISALLOW_COPY_AND_ASSIGN(DownloadsHistoryDataCollector);
-};
-
-// Mock that simulates a permissions dialog where the user denies
-// permission to install.  TODO(skerner): This could be shared with
-// extensions tests.  Find a common place for this class.
-class MockAbortExtensionInstallUI : public ExtensionInstallUI {
- public:
-  MockAbortExtensionInstallUI() : ExtensionInstallUI(NULL) {}
-
-  // Simulate a user abort on an extension installation.
-  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
-    delegate->InstallUIAbort(true);
-    MessageLoopForUI::current()->Quit();
-  }
-
-  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
-  virtual void OnInstallFailure(const std::string& error) {}
-};
-
-// Mock that simulates a permissions dialog where the user allows
-// installation.
-class MockAutoConfirmExtensionInstallUI : public ExtensionInstallUI {
- public:
-  explicit MockAutoConfirmExtensionInstallUI(Profile* profile)
-      : ExtensionInstallUI(profile) {}
-
-  // Proceed without confirmation prompt.
-  virtual void ConfirmInstall(Delegate* delegate, const Extension* extension) {
-    delegate->InstallUIProceed();
-  }
-
-  virtual void OnInstallSuccess(const Extension* extension, SkBitmap* icon) {}
-  virtual void OnInstallFailure(const std::string& error) {}
-};
-
-}  // namespace
-
-// While an object of this class exists, it will mock out download
-// opening for all downloads created on the specified download manager.
-class MockDownloadOpeningObserver : public DownloadManager::Observer {
- public:
-  explicit MockDownloadOpeningObserver(DownloadManager* manager)
-      : download_manager_(manager) {
-    download_manager_->AddObserver(this);
-  }
-
-  ~MockDownloadOpeningObserver() {
-    download_manager_->RemoveObserver(this);
-  }
+    DownloadManager* manager = browser->profile()->GetDownloadManager();
 
-  // DownloadManager::Observer
-  virtual void ModelChanged() {
-    std::vector<DownloadItem*> downloads;
-    download_manager_->SearchDownloads(string16(), &downloads);
+    new_delegate->SetDownloadManager(manager);
+    manager->set_delegate(new_delegate);
 
-    for (std::vector<DownloadItem*>::iterator it = downloads.begin();
-         it != downloads.end(); ++it) {
-      (*it)->TestMockDownloadOpen();
-    }
+    // Gives ownership to Profile.
+    browser->profile()->SetDownloadManagerDelegate(new_delegate);
   }
 
  private:
-  DownloadManager* download_manager_;
+  // Location of the test data.
+  FilePath test_dir_;
 
-  DISALLOW_COPY_AND_ASSIGN(MockDownloadOpeningObserver);
+  // Location of the downloads directory for these tests
+  ScopedTempDir downloads_directory_;
 };
 
 // NOTES:
@@ -981,27 +1013,34 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CheckInternetZone) {
 }
 #endif
 
-// Put up a Select File dialog when the file is downloaded, due to its MIME
-// type.
-//
-// This test runs correctly, but leaves behind turds in the test user's
-// download directory because of http://crbug.com/62099.  No big loss; it
-// was primarily confirming DownloadsObserver wait on select file dialog
-// functionality anyway.
-IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeTypeSelect) {
+// Put up a Select File dialog when the file is downloaded, due to
+// downloads preferences settings.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) {
   ASSERT_TRUE(InitialSetup(true));
   FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
   GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
 
+  NullSelectFile(browser());
+
   // Download the file and wait.  We expect the Select File dialog to appear
-  // due to the MIME type.
-  DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
+  // due to the MIME type, but we still wait until the download completes.
+  scoped_ptr<DownloadsObserver> observer(
+      new DownloadsObserver(
+          browser()->profile()->GetDownloadManager(),
+          1,
+          DownloadItem::COMPLETE,  // Really done
+          false,                   // Continue on select file.
+          ON_DANGEROUS_DOWNLOAD_FAIL));
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), url, CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  observer->WaitForFinished();
+  EXPECT_TRUE(observer->select_file_dialog_seen());
 
   // Check state.
   EXPECT_EQ(1, browser()->tab_count());
-  // Since we exited while the Select File dialog was visible, there should not
-  // be anything in the download shelf and so it should not be visible.
-  CheckDownloadUI(browser(), false, false, FilePath());
+  CheckDownload(browser(), file, file);
+  CheckDownloadUI(browser(), true, true, file);
 }
 
 // Access a file with a viewable mime-type, verify that a download
diff --git a/chrome/browser/profiles/off_the_record_profile_impl.cc b/chrome/browser/profiles/off_the_record_profile_impl.cc
index a608da2..b9c9f05 100644
--- a/chrome/browser/profiles/off_the_record_profile_impl.cc
+++ b/chrome/browser/profiles/off_the_record_profile_impl.cc
@@ -326,7 +326,10 @@ TemplateURLFetcher* OffTheRecordProfileImpl::GetTemplateURLFetcher() {
 
 DownloadManager* OffTheRecordProfileImpl::GetDownloadManager() {
   if (!download_manager_.get()) {
-    download_manager_delegate_ = new ChromeDownloadManagerDelegate(this);
+    // In case the delegate has already been set by
+    // SetDownloadManagerDelegate.
+    if (!download_manager_delegate_.get())
+      download_manager_delegate_ = new ChromeDownloadManagerDelegate(this);
     scoped_refptr<DownloadManager> dlm(
         new DownloadManager(download_manager_delegate_,
                             g_browser_process->download_status_updater()));
@@ -615,6 +618,11 @@ void OffTheRecordProfileImpl::Observe(int type,
   }
 }
 
+void OffTheRecordProfileImpl::SetDownloadManagerDelegate(
+    ChromeDownloadManagerDelegate* delegate) {
+  download_manager_delegate_ = delegate;
+}
+
 void OffTheRecordProfileImpl::CreateQuotaManagerAndClients() {
   if (quota_manager_.get()) {
     DCHECK(file_system_context_.get());
diff --git a/chrome/browser/profiles/off_the_record_profile_impl.h b/chrome/browser/profiles/off_the_record_profile_impl.h
index fbef5ab..f37e6b2 100644
--- a/chrome/browser/profiles/off_the_record_profile_impl.h
+++ b/chrome/browser/profiles/off_the_record_profile_impl.h
@@ -137,6 +137,9 @@ class OffTheRecordProfileImpl : public Profile,
                        const NotificationDetails& details) OVERRIDE;
 
  private:
+  virtual void SetDownloadManagerDelegate(
+      ChromeDownloadManagerDelegate* delegate) OVERRIDE;
+
   void CreateQuotaManagerAndClients();
 
   NotificationRegistrar registrar_;
diff --git a/chrome/browser/profiles/profile.h b/chrome/browser/profiles/profile.h
index 2cd49e4..70171ac 100644
--- a/chrome/browser/profiles/profile.h
+++ b/chrome/browser/profiles/profile.h
@@ -59,6 +59,7 @@ class Predictor;
 class AutocompleteClassifier;
 class BookmarkModel;
 class ChromeAppCacheService;
+class ChromeDownloadManagerDelegate;
 class ChromeURLDataManager;
 class Extension;
 class ExtensionDevToolsManager;
@@ -555,6 +556,13 @@ class Profile : public content::BrowserContext {
   static net::URLRequestContextGetter* default_request_context_;
 
  private:
+  friend class DownloadTest;
+
+  // Allow replacement of the download manager delegate, for interposition
+  // on browser tests.  Note that this takes ownership of |delegate|.
+  virtual void SetDownloadManagerDelegate(
+      ChromeDownloadManagerDelegate* delegate) = 0;
+
   // ***DEPRECATED**: You should be passing in the specific profile's
   // URLRequestContextGetter or using the system URLRequestContextGetter.
   //
diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
index e698177..94ab40b 100644
--- a/chrome/browser/profiles/profile_impl.cc
+++ b/chrome/browser/profiles/profile_impl.cc
@@ -1349,7 +1349,9 @@ void ProfileImpl::CreatePasswordStore() {
 
 DownloadManager* ProfileImpl::GetDownloadManager() {
   if (!created_download_manager_) {
-    download_manager_delegate_= new ChromeDownloadManagerDelegate(this);
+    // In case the delegate has already been set by SetDownloadManagerDelegate.
+    if (!download_manager_delegate_.get())
+      download_manager_delegate_= new ChromeDownloadManagerDelegate(this);
     scoped_refptr<DownloadManager> dlm(
         new DownloadManager(download_manager_delegate_,
                             g_browser_process->download_status_updater()));
@@ -1824,3 +1826,8 @@ SpellCheckProfile* ProfileImpl::GetSpellCheckProfile() {
     spellcheck_profile_.reset(new SpellCheckProfile());
   return spellcheck_profile_.get();
 }
+
+void ProfileImpl::SetDownloadManagerDelegate(
+    ChromeDownloadManagerDelegate* delegate) {
+  download_manager_delegate_ = delegate;
+}
diff --git a/chrome/browser/profiles/profile_impl.h b/chrome/browser/profiles/profile_impl.h
index df45efb..d03f50e 100644
--- a/chrome/browser/profiles/profile_impl.h
+++ b/chrome/browser/profiles/profile_impl.h
@@ -179,6 +179,9 @@ class ProfileImpl : public Profile,
 
   SpellCheckProfile* GetSpellCheckProfile();
 
+  virtual void SetDownloadManagerDelegate(
+      ChromeDownloadManagerDelegate* delegate);
+
   NotificationRegistrar registrar_;
   PrefChangeRegistrar pref_change_registrar_;
 
diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
index dcb57ef..5d035a4 100644
--- a/chrome/test/base/testing_profile.cc
+++ b/chrome/test/base/testing_profile.cc
@@ -786,6 +786,13 @@ prerender::PrerenderManager* TestingProfile::GetPrerenderManager() {
   return prerender_manager_.get();
 }
 
+void TestingProfile::SetDownloadManagerDelegate(
+    ChromeDownloadManagerDelegate* delegate) {
+  // Specially marked so errors from use will occur near to the site
+  // of the error.
+  NOTIMPLEMENTED();
+}
+
 PrefService* TestingProfile::GetOffTheRecordPrefs() {
   return NULL;
 }
diff --git a/chrome/test/base/testing_profile.h b/chrome/test/base/testing_profile.h
index 5057da5..5bae730 100644
--- a/chrome/test/base/testing_profile.h
+++ b/chrome/test/base/testing_profile.h
@@ -294,6 +294,9 @@ class TestingProfile : public Profile {
   TestingPrefService* testing_prefs_;
 
  private:
+  virtual void SetDownloadManagerDelegate(
+      ChromeDownloadManagerDelegate* delegate);
+
   // Common initialization between the two constructors.
   void Init();