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

chrome/browser/download/save_page_browsertest.cc

Index: chrome/browser/download/save_page_browsertest.cc
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index c2420ba5eb08c07de08f4fedb52d396cbf998351..b612173df2a0d234efb7fa2ac7e1b415dcea054b 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -4,8 +4,10 @@
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/i18n/file_util_icu.h"
 #include "base/path_service.h"
 #include "base/scoped_temp_dir.h"
+#include "base/string_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/net/url_request_mock_http_job.h"
 #include "chrome/browser/ui/browser.h"
@@ -141,6 +143,94 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
       dir.AppendASCII("1.css")));
 }
 
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SavedDirectory) {
+  FilePath file_name(FILE_PATH_LITERAL("a.htm"));
+  GURL url = URLRequestMockHTTPJob::GetMockUrl(
+      FilePath(kTestDir).Append(file_name));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  TabContentsWrapper* current_tab = browser()->GetSelectedTabContentsWrapper();
+  ASSERT_TRUE(current_tab);
+
+  // The default folder for saving HTML.
+  ScopedTempDir *website_save_temp_dir = new ScopedTempDir();

[Paweł Hajdan Jr., 2011/05/30 14:15:49]

Why are all of those pointers? Can't you just stack-allocate all of them? It
seems it doesn't matter much to delete them before the end of the test. If
that's not possible, switch to scoped_ptr.

Also, the star is on the wrong side. The right style is always "ScopedTempDir*
".

[haraken1, 2011/05/31 12:41:31]

Done.

+  // The default folder for saving downloaded files.
+  ScopedTempDir *download_save_temp_dir = new ScopedTempDir();
+  // The user's "Downloads" folder.
+  ScopedTempDir *default_save_temp_dir = new ScopedTempDir();
+  ASSERT_TRUE(website_save_temp_dir->CreateUniqueTempDir());
+  ASSERT_TRUE(download_save_temp_dir->CreateUniqueTempDir());
+  ASSERT_TRUE(default_save_temp_dir->CreateUniqueTempDir());
+  FilePath website_save_dir = website_save_temp_dir->path();

[Paweł Hajdan Jr., 2011/05/30 14:15:49]

nit: Does it make sense to have temporary variables for those?

[haraken1, 2011/05/31 12:41:31]

I feel that having these variables will make the code clearer. (Note: these
variables are not literally "temporary" variables but the variables for temporal
directories.) 

[Paweł Hajdan Jr., 2011/05/31 17:15:53]

I don't see how they help. The length of the expression is roughly the same, and
a value of those variables might change, while ScopedTempDir generally stays the
same.

[haraken1, 2011/06/02 09:13:22]

Done.

+  FilePath download_save_dir = download_save_temp_dir->path();
+  FilePath default_save_dir = default_save_temp_dir->path();
+  std::string title;
+  FilePath full_file_name;
+
+  // Temporarily change the user's "Downloads" folder to |default_save_dir|.
+  FilePath prev_default_save_dir;
+  PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &prev_default_save_dir);
+  PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir);

[Paweł Hajdan Jr., 2011/05/30 14:15:49]

PathService::Override is _extremely_ error-prone. Could you find a solution that
doesn't use it?

[haraken1, 2011/05/31 12:41:31]

I see. I removed PathService::Override and instead I used the real user's
"Downloads" directory for the test. But is it OK to use the real user's
"Downloads" directory for tests?

[Paweł Hajdan Jr., 2011/05/31 17:15:53]

I think some existing tests use the real Downloads directory, but in this
case... why don't you just Set the pref instead of overriding it? You should be
able to set it to a non-existent directory, and that'd make the tests much
simpler.

[haraken1, 2011/06/02 09:13:22]

I added a new method PathService::Set(int key, const FilePath& path), and used
it here. 

[Randy Smith (Not in Mondays), 2011/06/02 19:13:57]

Pawel: I'll go along with this if you really think it's the better choice, but
why is this better than using Override?  I can't offhand think of any gotchas
for Override that won't apply to Set, and Set involves creating a new method on
a very central class.  Enlighten me as to why this is the better path?

[Paweł Hajdan Jr., 2011/06/02 19:31:19]

No, Set is not the right solution. Please don't touch PathService at all. Just
change the pref's value in the user's profile. I'm pretty sure the downloads
directory is a preference.

+
+  title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageToProperDirectory(
+          website_save_dir, download_save_dir, SavePackage::SAVE_AS_ONLY_HTML));
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  full_file_name = website_save_dir.Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  // Delete the default folder for saving HTML.
+  delete website_save_temp_dir;
+
+  title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageToProperDirectory(
+          website_save_dir, download_save_dir, SavePackage::SAVE_AS_ONLY_HTML));
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  full_file_name = download_save_dir.Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  // Delete the default folder for downloaded files.
+  delete download_save_temp_dir;
+
+  title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageToProperDirectory(
+          website_save_dir, download_save_dir, SavePackage::SAVE_AS_ONLY_HTML));
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  full_file_name = default_save_dir.Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  // Restore the user's "Downloads" folder.
+  delete default_save_temp_dir;
+  PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, prev_default_save_dir);
+}
+
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, NoSave) {
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kAboutBlankURL));
   ASSERT_TRUE(browser()->command_updater()->SupportsCommand(IDC_SAVE_PAGE));