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

chrome/browser/download/download_util.h

Index: chrome/browser/download/download_util.h
diff --git a/chrome/browser/download/download_util.h b/chrome/browser/download/download_util.h
index e7726a9d28ac646f4aa640c022bee14a006139b2..52fd4292a043e9a4df70f4052edc44e4c7c331a2 100644
--- a/chrome/browser/download/download_util.h
+++ b/chrome/browser/download/download_util.h
@@ -51,8 +51,49 @@ 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.