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

chrome/browser/download/save_page_browsertest.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 "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"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/download/download_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/in_process_browser_test.h"
 #include "chrome/test/ui_test_utils.h"
@@ skipping 115 matching lines @@
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("b.saved1.htm"),
       full_file_name));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.png"),
       dir.AppendASCII("1.png")));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.css"),
       dir.AppendASCII("1.css")));
 }
 
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SavedDirectory) {

[Randy Smith (Not in Mondays), 2011/05/31 23:03:10]

I know it's not consistent with the rest of the file, but could you put a
sentence or two comment before this test describing what it's testing?

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

I added a detailed comment for the test. Also, for clarity, I split this test
into three tests, SaveToDirectoryForHTMLPage, SaveToDirectoryForDownloadedFiles
and SaveToUsersDownloadsFolder.

+  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);
+
+  ScopedTempDir website_save_temp_dir, download_save_temp_dir;
+  // The default folder for saving HTML.
+  ASSERT_TRUE(website_save_temp_dir.CreateUniqueTempDir());
+  // The default folder for saving downloaded files.
+  ASSERT_TRUE(download_save_temp_dir.CreateUniqueTempDir());
+  FilePath website_save_dir = website_save_temp_dir.path();
+  FilePath download_save_dir = download_save_temp_dir.path();
+  FilePath default_save_dir;
+  PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &default_save_dir);

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

Check the return value.

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

Done.

+  ASSERT_TRUE(file_util::PathExists(default_save_dir));
+  FilePath full_file_name;
+  std::string title;
+
+  title = UTF16ToASCII(

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

nit: Why not initialize title when declaring it then?

Also, please put blank lines in places that results in logical blocks.
full_file_name and title are more related to this block rather than the block
above.

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

Done.

+      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.

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

This is weird. Why don't we just pass a nonexistent folder at the beginning?

[Randy Smith (Not in Mondays), 2011/05/31 23:03:10]

Is there a way to create a unique temp dir that doesn't exist?  I'd think that'd
require creating a new version of CreateUniqueTempDir() that (like that
function) figured out where it could put a temp dir that wasn't already being
used, but (unlike that function) didn't create it.

[Paweł Hajdan Jr., 2011/06/01 07:08:09]

I'd say just pass some dir name like
/this/dir/does/not/exist/cyuwvuvcudrandomness and assert that it indeed doesn't
exist.

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

Done.

+  ASSERT_TRUE(website_save_temp_dir.Delete());
+
+  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.
+  ASSERT_TRUE(download_save_temp_dir.Delete());

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

Why do we explicitly do that here?

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

I removed it.

+
+  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));
+
+  // Delete the file that we created in a user's "Downloads" directory.
+  ASSERT_TRUE(file_util::Delete(full_file_name, false));

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

Why do we explicitly do that here?

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

Done.

+}
+
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, NoSave) {
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kAboutBlankURL));
   ASSERT_TRUE(browser()->command_updater()->SupportsCommand(IDC_SAVE_PAGE));
   EXPECT_FALSE(browser()->command_updater()->IsCommandEnabled(IDC_SAVE_PAGE));
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
   FilePath file_name(FILE_PATH_LITERAL("b.htm"));
 
   GURL url = URLRequestMockHTTPJob::GetMockUrl(
@@ skipping 23 matching lines @@
       full_file_name));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.png"),
       dir.AppendASCII("1.png")));
   EXPECT_TRUE(file_util::ContentsEqual(
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.css"),
       dir.AppendASCII("1.css")));
 }
 
 }  // namespace