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

chrome/browser/download/download_manager.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 "chrome/browser/download/download_manager.h"
 
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/logging.h"
-#include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/stl_util-inl.h"
 #include "base/stringprintf.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_create_info.h"
 #include "chrome/browser/download/download_extensions.h"
@@ skipping 339 matching lines @@
           download->id(),
           state,
           download_prefs()->download_path()));
 }
 
 void DownloadManager::CheckIfSuggestedPathExists(int32 download_id,
                                                  DownloadStateInfo state,
                                                  const FilePath& default_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  // Make sure the default download directory exists.
-  // TODO(phajdan.jr): only create the directory when we're sure the user
-  // is going to save there and not to another directory of his choice.
-  file_util::CreateDirectory(default_path);
-
   // Check writability of the suggested path. If we can't write to it, default
-  // to the user's "My Documents" directory. We'll prompt them in this case.
+  // to the user's "Downloads" directory. We'll prompt them in this case.
   FilePath dir = state.suggested_path.DirName();
   FilePath filename = state.suggested_path.BaseName();
   if (!file_util::PathIsWritable(dir)) {
     VLOG(1) << "Unable to write to directory \"" << dir.value() << "\"";
     state.prompt_user_for_save_location = true;
-    PathService::Get(chrome::DIR_USER_DOCUMENTS, &state.suggested_path);
-    state.suggested_path = state.suggested_path.Append(filename);
+    if (!download_util::DefaultDownloadDirectory::Get(&state.suggested_path) ||
+        !file_util::PathIsWritable(state.suggested_path)) {
+      VLOG(1) << "Cannot find the user's writable \"Downloads\" folder.";
+      // If the user's writable "Downloads" folder does not exist, use the
+      // originally suggested path even if the path does not exist
+      // (This is a rare case).
+      state.suggested_path = dir.Append(filename);
+    } else {
+      state.suggested_path = state.suggested_path.Append(filename);
+    }
+    // Make sure that the folder does exist.
+    if (!file_util::CreateDirectory(state.suggested_path.DirName()))

[Paweł Hajdan Jr., 2011/06/08 09:50:23]

nit: Please use braces {} for multi-line "if" body.

[haraken1, 2011/06/09 10:16:56]

Done.

+      LOG(ERROR) << "Failed to create " <<

[Paweł Hajdan Jr., 2011/06/08 09:50:23]

Logging the error is fine, but shouldn't we exit at this point and propagate the
error? I'd like to avoid downloads mysteriously failing with the only error
message being in the logs.

[Randy Smith (Not in Mondays), 2011/06/08 22:31:18]

I'm inclined to think that the right behavior is to make sure that the user is
prompted for the file location and not worry about it beyond that.  Ideally (and
maybe actually, based on the file selection dialog on the different systems) the
file selection dialog won't allow the user to pick a location that can't be
written to.  This is a corner case, and seeding the UI with a bad location
doesn't strike me as a horrible way to deal, given that we tried really hard and
couldn't find a good location.

[haraken1, 2011/06/09 10:16:56]

I would like to agree with Randy's idea, also considering the fact that we
cannot anyway avoid the "downloads mysteriously failing" situation, since even
if the download folder exists here, the download folder may be removed when a
file is actually downloaded to the folder. 

In conclusion, I made no modification here for now. 

+          state.suggested_path.DirName().value();
   }
 
   // If the download is deemed dangerous, we'll use a temporary name for it.
   if (state.IsDangerous()) {
     state.target_name = FilePath(state.suggested_path).BaseName();
     // Create a temporary file to hold the file until the user approves its
     // download.
     FilePath::StringType file_name;
     FilePath path;
 #if defined(OS_WIN)
     string16 unconfirmed_prefix =
         l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
 #else
     std::string unconfirmed_prefix =
         l10n_util::GetStringUTF8(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
 #endif
 
     while (path.empty()) {
       base::SStringPrintf(
           &file_name,
           unconfirmed_prefix.append(
               FILE_PATH_LITERAL(" %d.crdownload")).c_str(),
           base::RandInt(0, 100000));
       path = dir.Append(file_name);

[haraken1, 2011/06/09 10:16:56]

I changed this dir.Append(file_name) to
state.suggested_path.DirName().Append(file_name) in the latest patch. I think it
is correct, but would you please look at it?

       if (file_util::PathExists(path))
         path = FilePath();
     }
     state.suggested_path = path;
   } else {
     // Do not add the path uniquifier if we are saving to a specific path as in
     // the drag-out case.
     if (state.force_file_name.empty()) {
       state.path_uniquifier = download_util::GetUniquePathNumberWithCrDownload(
           state.suggested_path);
@@ skipping 69 matching lines @@
     // FileSelectionCancelled().
     int32* id_ptr = new int32;
     *id_ptr = download_id;
     select_file_dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
                                     string16(),
                                     suggested_path,
                                     &file_type_info, 0, FILE_PATH_LITERAL(""),
                                     contents, owning_window,
                                     reinterpret_cast<void*>(id_ptr));
     FOR_EACH_OBSERVER(Observer, observers_,
-                      SelectFileDialogDisplayed(download_id));
+                      SelectFileDialogDisplayed(download_id, suggested_path));
   } else {
     // No prompting for download, just continue with the suggested name.
     ContinueDownloadWithPath(download, suggested_path);
   }
 }
 
 void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DownloadItem* download = new DownloadItem(this, *info,
@@ skipping 768 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }