Added ability to set ChromeDownloadManagerDelegate for testing.

chrome/browser/download/download_browsertest.cc

Index: chrome/browser/download/download_browsertest.cc
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index e17d5b6378958326ec578636b5e0dbb4fcd78b8b..f22ddc73d42798728b117b596c0b8298b3d602f4 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,137 @@ 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 {

[ahendrickson, 2011/09/08 21:24:40]

Clang may not like the OVERRIDE here.

[Randy Smith (Not in Mondays), 2011/09/08 21:27:12]

I'll run the try bot; thanks for the suggestion.

+    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));
+
+    // 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();

[ahendrickson, 2011/09/08 21:24:40]

I've never liked running a message loop inside a constructor . . .

However, it IS a test, so I'm not going to object too strongly $-)

[Randy Smith (Not in Mondays), 2011/09/08 21:27:12]

This wasn't something I changed in this CL (moved yes, but not changed).  I have
no objection to fixing it, but unless you insist, I have an objection to fixing
it in this CL :-}.

[benjhayden, 2011/09/12 16:32:25]

Maybe just a TODO then?

[Randy Smith (Not in Mondays), 2011/09/12 17:52:58]

Sure (I don't find myself caring much, but it probably would be a bit better
outside the constructor).

+  }
+
+  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 +946,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 +1012,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