When the download folder does not exist, change the download folder to a user's "Downloads"

chrome/browser/download/download_manager_unittest.cc

Index: chrome/browser/download/download_manager_unittest.cc
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index 0e947d1325432e5c7ebf5a7dc6cfca356b075ee4..65bcd8c23db391f2d563d76488368dc30c08fe86 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -124,7 +124,7 @@ const struct {
     "text/html",
     false,
     false,
-    false, },
+    true, },

[Randy Smith (Not in Mondays), 2011/06/15 19:48:59]

It looks like we no longer have any cases in which we're not expecting a save_as
dialog; why is that?  I'm sure it's something obvious I'm not getting, but I
wouldn't have thought this patch would change that behavior, and the fact that
we have argues that there's a code path we used to test that we're not testing
anymore.  Can you enlighten me?

[haraken1, 2011/06/22 18:01:58]

This change is required in this patch (if we need to test |expected_save_as|).
This patch changes the download behavior so that the select file dialog appears
if the default download folder does not exist. In
DownloadManagerTest.StartDownload, since we set the default download folder to a
non-existent folder by 'prefs->SetFilePath(prefs::kDownloadDefaultDirectory,
FilePath())', the select file dialog always appears, whether |save_as| and
|prompt_for_download| is true or false. On the other hand, in the previous code,
we silently created the default download folder if it does not exist, and then
determined whether we should show the select file dialog or not, depending on
the value of |save_as| and |prompt_for_download|.

However, anyway I agree that it is no longer useful to test |expected_save_as|
because it must be always true. I would like to remove |expected_save_as| after
hearing your opinion.

[Randy Smith (Not in Mondays), 2011/06/23 20:24:42]

I still feel like we're not testing code that we used to test that's important
to test (the logic for when to prompt or not when we have a directory to write
to), and I'd like to go back to testing it.  Based on your description, I think
the issue is that we silently create non-existent directories for the default
download directory.  Could you change the call to set the default download
directory to a ScopedTempDir and see if you can revert the expectations back to
their original?

[haraken1, 2011/06/24 01:57:24]

Ah, it make a great sense to me. I set the default download folder to
ScopedTempDir and then reverted this test back to the original. 

   { "http://www.foo.com/save-as.html",
     "text/html",
     true,
@@ -139,7 +139,7 @@ const struct {
     "text/html",
     false,
     false,
-    false, },
+    true, },
   { "http://www.foo.com/extensionless-extension",
     "application/x-chrome-extension",
     true,
@@ -154,7 +154,7 @@ const struct {
     "application/pdf",
     false,
     true,
-    false, },
+    true, },
   { "http://www.foo.com/always_prompt.jar",
     "application/jar",
     false,
@@ -227,7 +227,8 @@ class SelectFileObserver : public DownloadManager::Observer {
   // Downloadmanager::Observer functions.
   virtual void ModelChanged() {}
   virtual void ManagerGoingDown() {}
-  virtual void SelectFileDialogDisplayed(int32 id) {
+  virtual void SelectFileDialogDisplayed(int32 id,
+                                         const FilePath& suggested_path) {
     file_dialog_ids_.insert(id);
   }
 
@@ -307,7 +308,10 @@ TEST_F(DownloadManagerTest, StartDownload) {
     DownloadFile* download_file(
         new DownloadFile(info.get(), download_manager_));
     AddDownloadToFileManager(info->download_id, download_file);
-    download_file->Initialize(false);
+
+    ScopedTempDir temp_dir;
+    ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+    download_file->Initialize(false, temp_dir.path());
     download_manager_->StartDownload(info->download_id);
     message_loop_.RunAllPending();
 
@@ -556,7 +560,9 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
       new DownloadFile(info.get(), download_manager_));
   download_file->Rename(cr_path);
   // This creates the .crdownload version of the file.
-  download_file->Initialize(false);
+  ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  download_file->Initialize(false, temp_dir.path());
   // |download_file| is owned by DownloadFileManager.
   AddDownloadToFileManager(info->download_id, download_file);