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

chrome/browser/download/download_util.h

 // 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.
 //
 // Download utilities.
 
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_UTIL_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_UTIL_H_
 #pragma once
 
@@ skipping 33 matching lines @@
 
 namespace gfx {
 class Canvas;
 class Image;
 }
 
 namespace download_util {
 
 // Download temporary file creation --------------------------------------------
 
-// Return the default download directory.
-const FilePath& GetDefaultDownloadDirectory();
+// Overrides the path of the default download folder returned by
+// DefaultDownloadDirectory::Get().
+// WARNING: We can generate an object of this class only on testing context
+// (e.g. download tests) for temporarily switching the default download folder.
+// WARNING: In order to override the default download folder, we must use

[Paweł Hajdan Jr., 2011/06/09 19:10:38]

nit: Please remove the part about PathService::Override, it's really weird to
list things we shouldn't be using here. There are lots of them.

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

Done.

+// this class instead of using PathService::Override(),
+// since PathService::Override() can cause many problems:
+// http://codereview.chromium.org/2805100
+class ScopedDefaultDownloadDirectory {

[Paweł Hajdan Jr., 2011/06/09 19:10:38]

Could you move this class to a special file only for testing?

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

I moved it to download_test_util.h, download_test_util.cc and
download_test_util_unittest.cc.

+ public:
+  ScopedDefaultDownloadDirectory();
+
+  // If the path is overridden, invalidates the overriding.
+  ~ScopedDefaultDownloadDirectory();
+
+  // Overrides the path.
+  void Override(const FilePath& override_path);

[Paweł Hajdan Jr., 2011/06/09 19:10:38]

I think the ctor should take the FilePath parameter, and that there should be no
other public methods. Much harder to misuse.

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

Done. Now, ScopedDefaultDownloadDirectory has only a constructor and a
destructor. 

+
+  // Invalidates the overriding.
+  void UnOverride();

[Randy Smith (Not in Mondays), 2011/06/10 20:58:53]

I'd get rid of Override and UnOverride and just have the constructor and
destructor do work--having the *Overrides seems like a more complicated
interface than you need.

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

Done.

+};

[Paweł Hajdan Jr., 2011/06/09 19:10:38]

And DISALLOW_COPY_AND_ASSIGN please.

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

Done.

+
+class DefaultDownloadDirectory {

[Randy Smith (Not in Mondays), 2011/06/10 20:58:53]

This can be used from multiple threads *unless* ScopedDefaultDownloadDirectory
is being used, in which case it's only valid to use it on the same thread as
ScopedDefaultDownloadDirectory (or otherwise be certain that you're not racing).
 That's a fairly complex and non-obvious thread safety restriction.  Could you
either document it here or (preferred) make sure that this function is only
called on one thread and document that that's it's thread?

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

- I wrote "DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))" at the head of
DownloadPrefs::OverrideDefaultDownloadDirectory() and
DownloadPrefs::UnOverrideDefaultDownloadDirectory().
- I added a comment in download_prefs.h that says those methods can be used only
through ScopedDefaultDownloadDirectory. 
- I added a comment in download_test_util.h that says
ScopedDefaultDownloadDirectory can be used only from the UI thread.

+ public:
+  // Returns the path of the default download folder.
+  // If the path is being overridden, returns the overridden path.
+  static const FilePath Get();
+
+ private:
+  friend class ScopedDefaultDownloadDirectory;
+  static FilePath override_path_;
+};
+
+// Chooses a writable directory from |website_save_dir|, |download_save_dir|
+// and |default_downloads_dir| in this order of priority.
+// If none of them is writable, creates |download_save_dir| and
+// chooses |download_save_dir|. The choosed directory is stored to |save_dir|.
+// Returns true if neither |website_save_dir| nor |download_save_dir|
+// is writable, which indicates the select file dialog should be displayed.
+bool ChooseSavableDirectory(const FilePath& website_save_dir,
+                            const FilePath& download_save_dir,
+                            const FilePath& default_downloads_dir,
+                            FilePath* save_dir);

[Randy Smith (Not in Mondays), 2011/06/10 20:58:53]

I'm not sure I see a reason for this to be in download_util, and if we can have
it somewhere else, that's my preference.  I'd suggest making it a static method
on DownloadManager if it's needed outside of DownloadManager, and a static
function in download_manager.cc if it's not.

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

Done. I made it a static method of DownloadManager, since
SavePackage::CreateDirectoryOnFileThread requires the method. 

 
 // Create a temporary file for a download in the user's default download
 // directory and return true if was successful in creating the file.
 bool CreateTemporaryFileForDownload(FilePath* path);
 
 // Return true if the |download_path| is dangerous path.
 bool DownloadPathIsDangerous(const FilePath& download_path);
 
 // Create an extension based on the file name and mime type.
 void GenerateExtension(const FilePath& file_name,
@@ skipping 223 matching lines @@
 // Erases all downloaded files with the specified path and name prefix.
 // Used by download UI tests to clean up the download directory.
 void EraseUniqueDownloadFiles(const FilePath& path_prefix);
 
 // Returns a .crdownload intermediate path for the |suggested_path|.
 FilePath GetCrDownloadPath(const FilePath& suggested_path);
 
 }  // namespace download_util
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_UTIL_H_