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

chrome/browser/download/download_util.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.
 //
 // Download utility implementation
 
 #include "chrome/browser/download/download_util.h"
 
 #if defined(OS_WIN)
 #include <shobjidl.h>
@@ skipping 185 matching lines @@
 
   DCHECK(!generated_name->empty());
 
   GenerateSafeFileName(mime_type, generated_name);
 }
 
 }  // namespace
 
 // Download temporary file creation --------------------------------------------
 
-class DefaultDownloadDirectory {
- public:
-  const FilePath& path() const { return path_; }
- private:
-  DefaultDownloadDirectory() {
-    if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &path_)) {
-      NOTREACHED();

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

This may generate disagreement from you or Pawel, but I'd rather leave in the
NOTREACHED()'s and not return success/failure (i.e. assert that we'll always
succeed).  Otherwise we have to figure out ways to propagate an error up for an
event we believe will never happen.

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

I'm fine with the NOTREACHED in that case.

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

Done. I left NOTREACHED() here and changed the signature of "bool
DefaultDownloadDirectory::Get(FilePath* path)" to "const FilePath
DefaultDownloadDirectory::Get()".

-    }
-    if (DownloadPathIsDangerous(path_)) {
-      if (!PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS_SAFE, &path_)) {
-        NOTREACHED();
-      }
-    }
+FilePath DefaultDownloadDirectory::override_path_;
+
+bool DefaultDownloadDirectory::Get(FilePath* path) {
+  if (!override_path_.empty()) {
+    *path = override_path_;
+    return true;
   }
-  friend struct base::DefaultLazyInstanceTraits<DefaultDownloadDirectory>;
-  FilePath path_;
-};
+  bool result = PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, path);
+  if (DownloadPathIsDangerous(*path)) {
+    result = PathService::Get(
+        chrome::DIR_DEFAULT_DOWNLOADS_SAFE, path);
+  }
+  return result;
+}
 
-static base::LazyInstance<DefaultDownloadDirectory>
-    g_default_download_directory(base::LINKER_INITIALIZED);
+void DefaultDownloadDirectory::Override(const FilePath& override_path) {

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

Now the problem is that we have prefs::kDownloadDefaultDirectory that should be
updated too. How about moving this entire logic to DownloadPrefs? I that we
generally avoid adding new code to download_util.

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

I agree about not adding more logic to download_util.cc, but DownloadPrefs isn't
right--DownloadPrefs is just a cache/layer on top of the preferences service,
and adding stuff unrelated to the preferences service to it would just pollute
the interface.  Maybe just move DefaultDownloadDirectory to its own file?

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

I added DownloadPrefs with that kind of code in mind too. Having a thin wrapper
is not ideal, and having the tiniest pieces in their own files is not ideal
either.

My idea is that DownloadPrefs is for everything that deals with the prefs, and
this _will_ deal with the prefs.

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

I am sorry but I am a bit confused. First of all, my understanding is that
user's preferences and PathService are entirely different things. So, in the
beginning, I cannot see why we have to update prefs::kDownloadDefaultDirectory
when we want to update the user's "Downloads" folder provided by PathService. I
guess that an overriding function for user's preferences and an overriding
function for PathService should be prepared separately. Currently, the former is
PrefService::SetFilePath() and the latter is
DefaultDownloadDirectory::Override(). Would you please tell me your idea more in
detail?

(I made no modification yet for now. )

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

Ok, I'm willing to accept this.  It's not ideal, but it's ok, and I don't see a
better plan (sorry about the tepid endorsement :-}).

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

I moved the functionality of DefaultDownloadDirectory into DownloadPrefs as
follows:

- DownloadPrefs::GetDefaultDownloadDirectory() : Returns the user's default
"Downloads" folder that may be being overridden. 
- DownloadPrefs::OverrideDefaultDownloadDirectory() : Overrides the user's
default "Downloads" folder.
- DownloadPrefs::UnOverrideDefaultDownloadDirectory() : Invalidates the
overriding.

In particular, DownloadPrefs::OverrideDefaultDownloadDirectory() updates not
only the path provided by PathService but also the path managed by PrefService
and |download_path_|.

+  override_path_ = override_path;
+}
 
-const FilePath& GetDefaultDownloadDirectory() {
-  return g_default_download_directory.Get().path();
+void DefaultDownloadDirectory::UnOverride() {
+  override_path_.clear();
 }
 
 bool CreateTemporaryFileForDownload(FilePath* temp_file) {
-  if (file_util::CreateTemporaryFileInDir(GetDefaultDownloadDirectory(),
-                                          temp_file))
+  FilePath path;
+  if (!DefaultDownloadDirectory::Get(&path))
+    return false;
+  if (file_util::CreateTemporaryFileInDir(path, temp_file))
     return true;
   return file_util::CreateTemporaryFile(temp_file);
 }
 
 bool DownloadPathIsDangerous(const FilePath& download_path) {
   FilePath desktop_dir;
   if (!PathService::Get(chrome::DIR_USER_DESKTOP, &desktop_dir)) {
     NOTREACHED();
     return false;
   }
@@ skipping 679 matching lines @@
 FilePath GetCrDownloadPath(const FilePath& suggested_path) {
   FilePath::StringType file_name;
   base::SStringPrintf(
       &file_name,
       PRFilePathLiteral FILE_PATH_LITERAL(".crdownload"),
       suggested_path.value().c_str());
   return FilePath(file_name);
 }
 
 }  // namespace download_util