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

chrome/browser/download/download_manager_unittest.cc

 // 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.
 
 #include <string>
 #include <set>
 
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util-inl.h"
@@ skipping 106 matching lines @@
   const char* url;
   const char* mime_type;
   bool save_as;
   bool prompt_for_download;
   bool expected_save_as;
 } kStartDownloadCases[] = {
   { "http://www.foo.com/dont-open.html",
     "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,
     false,
     true, },
   { "http://www.foo.com/always-prompt.html",
     "text/html",
     false,
     true,
     true, },
   { "http://www.foo.com/user-script-text-html-mimetype.user.js",
     "text/html",
     false,
     false,
-    false, },
+    true, },
   { "http://www.foo.com/extensionless-extension",
     "application/x-chrome-extension",
     true,
     false,
     true, },
   { "http://www.foo.com/save-as.pdf",
     "application/pdf",
     true,
     false,
     true, },
   { "http://www.foo.com/sometimes_prompt.pdf",
     "application/pdf",
     false,
     true,
-    false, },
+    true, },
   { "http://www.foo.com/always_prompt.jar",
     "application/jar",
     false,
     true,
     true, },
 };
 
 const struct {
   FilePath::StringType suggested_path;
   bool is_dangerous_file;
@@ skipping 52 matching lines @@
     download_manager_->AddObserver(this);
   }
 
   ~SelectFileObserver() {
     download_manager_->RemoveObserver(this);
   }
 
   // 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);
   }
 
   bool ShowedFileDialogForId(int32 id) {
     return file_dialog_ids_.find(id) != file_dialog_ids_.end();
   }
 
  private:
   std::set<int32> file_dialog_ids_;
   scoped_refptr<DownloadManager> download_manager_;
@@ skipping 59 matching lines @@
     scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
     info->download_id = static_cast<int>(i);
     info->prompt_user_for_save_location = kStartDownloadCases[i].save_as;
     info->url_chain.push_back(GURL(kStartDownloadCases[i].url));
     info->mime_type = kStartDownloadCases[i].mime_type;
     download_manager_->CreateDownloadItem(info.get());
 
     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();
 
     // SelectFileObserver will have recorded any attempt to open the
     // select file dialog.
     // Note that DownloadManager::FileSelectionCanceled() is never called.
     EXPECT_EQ(kStartDownloadCases[i].expected_save_as,
               observer.ShowedFileDialogForId(i));
   }
 }
@@ skipping 228 matching lines @@
   scoped_ptr<ItemObserver> observer(new ItemObserver(download));
 
   // Create and initialize the download file.  We're bypassing the first part
   // of the download process and skipping to the part after the final file
   // name has been chosen, so we need to initialize the download file
   // properly.
   DownloadFile* download_file(
       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);
 
   ContinueDownloadWithPath(download, new_path);
   message_loop_.RunAllPending();
   EXPECT_TRUE(GetActiveDownloadItem(0) != NULL);
 
   download_file->AppendDataToFile(kTestData, kTestDataLen);
 
   // Finish the download.
@@ skipping 11 matching lines @@
   EXPECT_FALSE(observer->was_opened());
   EXPECT_EQ(DownloadItem::COMPLETE, download->state());
 
   EXPECT_TRUE(file_util::PathExists(new_path));
   EXPECT_FALSE(file_util::PathExists(cr_path));
   EXPECT_FALSE(file_util::PathExists(unique_new_path));
   std::string file_contents;
   EXPECT_TRUE(file_util::ReadFileToString(new_path, &file_contents));
   EXPECT_EQ(std::string(kTestData), file_contents);
 }