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..ff8c8a0437a4aded9664c4d2503db3db8edb2e6f 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -4,15 +4,23 @@
 
 #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/download/download_manager.h"
+#include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/download/download_test_util.h"
 #include "chrome/browser/net/url_request_mock_http_job.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.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/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/in_process_browser_test.h"
 #include "chrome/test/ui_test_utils.h"
@@ -47,13 +55,73 @@ class SavePageBrowserTest : public InProcessBrowserTest {
     return *Details<GURL>(observer.details()).ptr();
   }
 
+  // Changes the default folder prefs. This method saves the current folder
+  // for saving HTML, the current folder for saving downloaded files,
+  // the current user's "Downloads" folder and a save type (HTML only or
+  // complete HTML files), and then changes them to |website_save_dir|,
+  // |download_save_dir| and |save_type|, respectively.
+  // If we call this method, we must call RestoreDirectoryPrefs()
+  // after the test to restore the default folder prefs.
+  void ChangeDirectoryPrefs(
+      Profile* profile,
+      const FilePath& website_save_dir,
+      const FilePath& download_save_dir,
+      const SavePackage::SavePackageType save_type) {
+    DCHECK(profile);
+    PrefService* prefs = profile->GetPrefs();
+
+    DCHECK(prefs->FindPreference(prefs::kDownloadDefaultDirectory));
+    prev_download_save_dir_ = prefs->GetFilePath(
+        prefs::kDownloadDefaultDirectory);
+
+    // Check whether the preference has the default folder for saving HTML.
+    // If not, initialize it with the default folder for downloaded files.
+    if (!prefs->FindPreference(prefs::kSaveFileDefaultDirectory)) {
+      prefs->RegisterFilePathPref(prefs::kSaveFileDefaultDirectory,
+                                  prev_download_save_dir_,
+                                  PrefService::UNSYNCABLE_PREF);
+    }
+    prev_website_save_dir_ = prefs->GetFilePath(
+        prefs::kSaveFileDefaultDirectory);
+
+    DownloadPrefs* download_prefs =
+        profile->GetDownloadManager()->download_prefs();
+    prev_save_type_ =
+        static_cast<SavePackage::SavePackageType>
+        (download_prefs->save_file_type());
+
+    prefs->SetFilePath(
+        prefs::kSaveFileDefaultDirectory, website_save_dir);
+    prefs->SetFilePath(
+        prefs::kDownloadDefaultDirectory, download_save_dir);
+    prefs->SetInteger(prefs::kSaveFileType, save_type);
+  }
+
+  // Restores the default folder prefs.
+  void RestoreDirectoryPrefs(Profile* profile) {
+    DCHECK(profile);
+    PrefService* prefs = profile->GetPrefs();
+    prefs->SetFilePath(
+        prefs::kSaveFileDefaultDirectory, prev_website_save_dir_);
+    prefs->SetFilePath(
+        prefs::kDownloadDefaultDirectory, prev_download_save_dir_);
+    prefs->SetInteger(prefs::kSaveFileType, prev_save_type_);
+  }
+
   // Path to directory containing test data.
   FilePath test_dir_;
 
   // Temporary directory we will save pages to.
   ScopedTempDir save_dir_;
+
+  // Temporarily stores the default folder prefs.
+  FilePath prev_website_save_dir_;
+  FilePath prev_download_save_dir_;
+  SavePackage::SavePackageType prev_save_type_;
 };
 
+}  // namespace
+
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveHTMLOnly) {
   FilePath file_name(FILE_PATH_LITERAL("a.htm"));
   GURL url = URLRequestMockHTTPJob::GetMockUrl(
@@ -141,6 +209,244 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveCompleteHTML) {
       dir.AppendASCII("1.css")));
 }
 
+// Checks if an HTML page is saved to the default folder for saving HTML
+// in the following situation:
+// The default folder for saving HTML exists.
+// The default folder for downloaded files exists.
+// The user's "Downloads" folder exists.
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder1) {
+  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);
+  ASSERT_TRUE(current_tab->tab_contents());
+  ASSERT_TRUE(current_tab->tab_contents()->profile());
+
+  ScopedTempDir website_save_dir, download_save_dir, default_download_dir;
+  // Prepare the default folder for saving HTML.
+  ASSERT_TRUE(website_save_dir.CreateUniqueTempDir());
+  // Prepare the default folder for downloaded files.
+  ASSERT_TRUE(download_save_dir.CreateUniqueTempDir());
+  // Prepare the user's "Downloads" folder.
+  ASSERT_TRUE(default_download_dir.CreateUniqueTempDir());
+
+  // Changes the default prefs.
+  ChangeDirectoryPrefs(
+      current_tab->tab_contents()->profile(),
+      website_save_dir.path(),
+      download_save_dir.path(),
+      SavePackage::SAVE_AS_ONLY_HTML);
+  // Overrides the user's "Downloads" folder.
+  download_test_util::ScopedDefaultDownloadDirectory
+      override_default_download(
+          default_download_dir.path(),
+          current_tab->tab_contents()->profile()->
+          GetDownloadManager()->download_prefs());
+
+  std::string title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageBasedOnDefaultPrefs());
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  FilePath full_file_name =
+      website_save_dir.path().Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Is the file downloaded to the default folder for saving HTML?
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  RestoreDirectoryPrefs(current_tab->tab_contents()->profile());
+}
+
+// Checks if an HTML page is saved to the default folder for downloaded files
+// in the following situation:
+// The default folder for saving HTML does not exist.
+// The default folder for downloaded files exists.
+// The user's "Downloads" folder exists.
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder2) {
+  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);
+  ASSERT_TRUE(current_tab->tab_contents());
+  ASSERT_TRUE(current_tab->tab_contents()->profile());
+
+  ScopedTempDir download_save_dir, default_download_dir;
+  // Prepare the default folder for saving downloaded files.
+  ASSERT_TRUE(download_save_dir.CreateUniqueTempDir());
+  // Prepare the user's "Downloads" folder.
+  ASSERT_TRUE(default_download_dir.CreateUniqueTempDir());
+  // Prepare non-existent folder.
+  FilePath nonexistent_path("/tmp/koakuma_mikity_moemoe_nyannyan");
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path));
+
+  // Changes the default prefs.
+  ChangeDirectoryPrefs(
+      current_tab->tab_contents()->profile(),
+      nonexistent_path,
+      download_save_dir.path(),
+      SavePackage::SAVE_AS_ONLY_HTML);
+  // Overrides the user's "Downloads" folder.
+  download_test_util::ScopedDefaultDownloadDirectory
+      override_default_download(
+          default_download_dir.path(),
+          current_tab->tab_contents()->profile()->
+          GetDownloadManager()->download_prefs());
+
+  std::string title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageBasedOnDefaultPrefs());
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  FilePath full_file_name =
+      download_save_dir.path().Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Is the file downloaded to the default folder for downloaded files?
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_FALSE(file_util::PathExists(nonexistent_path));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  RestoreDirectoryPrefs(current_tab->tab_contents()->profile());
+}
+
+// Checks if an HTML page is saved to the user's "Downloads" directory
+// in the following situation:
+// The default folder for saving HTML does not exist.
+// The default folder for downloaded files does not exist.
+// The user's "Downloads" folder exists.
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder3) {
+  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);
+  ASSERT_TRUE(current_tab->tab_contents());
+  ASSERT_TRUE(current_tab->tab_contents()->profile());
+
+  ScopedTempDir default_download_dir;
+  // Prepare the user's "Downloads" folder.
+  ASSERT_TRUE(default_download_dir.CreateUniqueTempDir());
+  // Prepare non-existent folder.
+  FilePath nonexistent_path1("/tmp/koakuma_mikity_moemoe_nyannyan");
+  FilePath nonexistent_path2("/tmp/koakuma_mikity_moemoe_pyonpyon");
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path1));
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path2));
+
+  // Changes the default prefs.
+  ChangeDirectoryPrefs(
+      current_tab->tab_contents()->profile(),
+      nonexistent_path1,
+      nonexistent_path2,
+      SavePackage::SAVE_AS_ONLY_HTML);
+  // Overrides the user's "Downloads" folder.
+  download_test_util::ScopedDefaultDownloadDirectory
+      override_default_download(
+          default_download_dir.path(),
+          current_tab->tab_contents()->profile()->
+          GetDownloadManager()->download_prefs());
+
+  std::string title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageBasedOnDefaultPrefs());
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  FilePath full_file_name =
+      default_download_dir.path().Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Is the file downloaded to the user's "Downloads" directory?
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_FALSE(file_util::PathExists(nonexistent_path1));
+  EXPECT_FALSE(file_util::PathExists(nonexistent_path2));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  RestoreDirectoryPrefs(current_tab->tab_contents()->profile());
+}
+
+// Checks if the default folder for downloaded files is created
+// and an HTML page is saved to the folder in the following situation:
+// The default folder for saving HTML does not exist.
+// The default folder for downloaded files does not exist.
+// The user's "Downloads" folder does not exist.
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder4) {

[haraken1, 2011/06/14 11:10:05]

IMPORTANT: This test fails in this patch by the following reason. 

A downloaded file is renamed .org.chromium.xxxx -> yyyy.crdownload -> yyyy, in
this order. The problem here is where .org.chromium.xxxx is created. In the
previous patch, .org.chromium.xxxx is created on the user's "Downloads" folder
provided by PathService. Specifically, SaveFileManager::StartSave() calls
BaseFile::Initialize(), and  then BaseFile::Initialize() creates
.org.chromium.xxxx on the "Downloads" folder provided by PathService, not on the
"Downloads" folder being overridden by ScopedDefaultDownloadDirectory. Thus,
this creation definitely succeeds even if the overridden "Downloads" folder does
not exist (this is the thing that we are going to test in this test). 

However, in this patch, I modified so that SaveFileManager::StartSave() uses the
folder provided by  DownloadPrefs::GetDefaultDownloadDirectory(), which returns
the overridden "Downloads" folder (if it is being overridden). In this way,
SaveFileManager::StartSave() tries to create .org.chromium.xxxx on the
non-existent overriden "Downloads" folder and thus it fails... I think that the
modification that SaveFileManager::StartSave() uses the folder provided by 
DownloadPrefs::GetDefaultDownloadDirectory() is definitely right, because If we
use the folder provided by PathService as before, we cannot test what we want to
test here (i.e. what happens if the "Downloads" folder does not exist). 

Anyway, what is important is to decide what *should* happen if the "Downloads"
folder does not exist. I guess that creating the "Downloads" folder and trying
to create .org.chromium.xxxx on the created folder may be a good approach, but
what do you think? The same problem happens in DownloadTest.DownloadFolder2,
which also fails with this patch. 

(I feel that this patch becomes too large... So if possible, I would like to
just remove SavePageBrowserTest.SaveFolder4 and DownloadTest.DownloadFolder2
from this patch, and then tackle this problem in detail as an another patch.)

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

I'm torn.  That's fine behavior, but there will be circumstances under which we
cannot create that directory, so we'll need to handle that case as well.  And I
think the case where the default downloads directory doesn't exist is rare
enough that I don't want to add yet another conditional in the code to try to
create it and then surface the error if we don't.  So my inclination is to say
that we should fail the download/save page in this situation.  Ideally we'd
report that error back to the user, but if there's not an easy way to do that
I'd be ok with filing a bug indicating that we need to find a way to report the
error back to the user and fail the download/save page in a not very informative
way.

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

I removed SavePageBrowserTest.SaveFolder4 and DownloadTest.DownloadFolder2. 

+  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);
+  ASSERT_TRUE(current_tab->tab_contents());
+  ASSERT_TRUE(current_tab->tab_contents()->profile());
+
+  // Prepare non-existent folder.
+  FilePath nonexistent_path1("/tmp/koakuma_mikity_moemoe_nyannyan");
+  FilePath nonexistent_path2("/tmp/koakuma_mikity_moemoe_pyonpyon");
+  FilePath nonexistent_path3("/tmp/koakuma_mikity_moemoe_kyunkyun");
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path1));
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path2));
+  ASSERT_FALSE(file_util::PathExists(nonexistent_path3));
+
+  // Changes the default prefs.
+  ChangeDirectoryPrefs(
+      current_tab->tab_contents()->profile(),
+      nonexistent_path1,
+      nonexistent_path2,
+      SavePackage::SAVE_AS_ONLY_HTML);
+  // Overrides the user's "Downloads" folder.
+  download_test_util::ScopedDefaultDownloadDirectory
+      override_default_download(
+          nonexistent_path3,
+          current_tab->tab_contents()->profile()->
+          GetDownloadManager()->download_prefs());
+
+  std::string title = UTF16ToASCII(
+      current_tab->download_tab_helper()->SavePageBasedOnDefaultPrefs());
+  file_util::ReplaceIllegalCharactersInPath(&title, ' ');
+  FilePath full_file_name =
+      nonexistent_path2.Append(FilePath(title + ".html"));
+
+  EXPECT_EQ(url, WaitForSavePackageToFinish());
+
+  if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF))
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Is the file downloaded to the default folder for downloaded files?
+  EXPECT_TRUE(file_util::PathExists(full_file_name));
+  EXPECT_FALSE(file_util::PathExists(nonexistent_path1));
+  EXPECT_TRUE(file_util::PathExists(nonexistent_path2));
+  EXPECT_FALSE(file_util::PathExists(nonexistent_path3));
+  EXPECT_TRUE(file_util::ContentsEqual(
+      test_dir_.Append(FilePath(kTestDir)).Append(file_name),
+      full_file_name));
+
+  ASSERT_TRUE(file_util::Delete(nonexistent_path2, true));
+
+  RestoreDirectoryPrefs(current_tab->tab_contents()->profile());
+}
+
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, NoSave) {
   ui_test_utils::NavigateToURL(browser(), GURL(chrome::kAboutBlankURL));
   ASSERT_TRUE(browser()->command_updater()->SupportsCommand(IDC_SAVE_PAGE));
@@ -182,5 +488,3 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, FileNameFromPageTitle) {
       test_dir_.Append(FilePath(kTestDir)).AppendASCII("1.css"),
       dir.AppendASCII("1.css")));
 }
-
-}  // namespace