Merge SavePackageFilePicker{,ChromeOS}

chrome/browser/download/save_package_file_picker.cc

Index: chrome/browser/download/save_package_file_picker.cc
diff --git a/chrome/browser/download/save_package_file_picker.cc b/chrome/browser/download/save_package_file_picker.cc
index fe76d68f70d942175817fc8033de403a12939027..8383365e4f0bf5bce901423d5988902b12a297cb 100644
--- a/chrome/browser/download/save_package_file_picker.cc
+++ b/chrome/browser/download/save_package_file_picker.cc
@@ -4,18 +4,24 @@
 
 #include "chrome/browser/download/save_package_file_picker.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/command_line.h"
+#include "base/i18n/file_util_icu.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_member.h"
 #include "base/prefs/pref_service.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/save_page_type.h"
@@ -23,6 +29,12 @@
 #include "content/public/browser/web_contents_view.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/shell_dialogs/selected_file_info.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/drive/download_handler.h"
+#include "chrome/browser/chromeos/drive/file_system_util.h"
+#endif
 
 using content::RenderProcessHost;
 using content::SavePageType;
@@ -60,17 +72,43 @@ const int kIndexToIDS[] = {
   0, IDS_SAVE_PAGE_DESC_HTML_ONLY, IDS_SAVE_PAGE_DESC_COMPLETE,
 };
 
-void OnSavePackageDownloadCreated(
-    content::DownloadItem* download) {
+void OnSavePackageDownloadCreated(content::DownloadItem* download) {
   ChromeDownloadManagerDelegate::DisableSafeBrowsing(download);

[Randy Smith (Not in Mondays), 2013/05/01 19:18:43]

If I'm reading the code correctly, this hasn't historically been done under
ChromeOS.  Why start now?  (I don't object, I'm just asking for my own
education.)

[benjhayden, 2013/05/06 20:05:19]

Code unification.
It will continue to do nothing if safe browsing is not enabled, and if/when safe
browsing is enabled on cros, then fewer things will need to be updated.

 }
 
+#if defined(OS_CHROMEOS)
+void OnSavePackageDownloadCreatedChromeOS(
+    Profile* profile,
+    const base::FilePath& drive_path,
+    content::DownloadItem* download) {
+  drive::DownloadHandler::GetForProfile(profile)->SetDownloadParams(
+      drive_path, download);
+  OnSavePackageDownloadCreated(download);
+}
+
+// Trampoline callback between SubstituteDriveDownloadPath() and |callback|.
+void ContinueSettingUpDriveDownload(
+    const content::SavePackagePathPickedCallback& callback,
+    content::SavePageType save_type,
+    Profile* profile,
+    const base::FilePath& drive_path,
+    const base::FilePath& drive_tmp_download_path) {
+  if (drive_tmp_download_path.empty())  // Substitution failed.
+    return;
+  callback.Run(drive_tmp_download_path, save_type, base::Bind(
+      &OnSavePackageDownloadCreatedChromeOS, profile, drive_path));
+}
+#endif
+
 }  // anonymous namespace
 
 bool SavePackageFilePicker::ShouldSaveAsMHTML() const {
-  return can_save_as_complete_ &&
-         CommandLine::ForCurrentProcess()->HasSwitch(
-             switches::kSavePageAsMHTML);
+#if !defined(OS_CHROMEOS)
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kSavePageAsMHTML))
+    return false;
+#endif
+  return can_save_as_complete_;
 }
 
 SavePackageFilePicker::SavePackageFilePicker(
@@ -82,22 +120,30 @@ SavePackageFilePicker::SavePackageFilePicker(
     const content::SavePackagePathPickedCallback& callback)
     : render_process_id_(web_contents->GetRenderProcessHost()->GetID()),
       can_save_as_complete_(can_save_as_complete),
+      download_prefs_(download_prefs),
       callback_(callback) {
   base::FilePath suggested_path = suggested_path_const;
   base::FilePath::StringType default_extension = default_extension_const;
-  int file_type_index = SavePackageTypeToIndex(
-      static_cast<SavePageType>(download_prefs->save_file_type()));
-  DCHECK_NE(-1, file_type_index);
-
+  int file_type_index = 0;
   ui::SelectFileDialog::FileTypeInfo file_type_info;
 
+#if defined(OS_CHROMEOS)
+  file_type_info.support_drive = true;
+#else
+  file_type_index = SavePackageTypeToIndex(
+      static_cast<SavePageType>(download_prefs_->save_file_type()));
+  DCHECK_NE(-1, file_type_index);
+#endif
+
   // TODO(benjhayden): Merge the first branch with the second when all of the
   // platform-specific file selection dialog implementations fully support
   // switching save-as file formats, and remove the flag/switch.
   if (ShouldSaveAsMHTML()) {
     default_extension = FILE_PATH_LITERAL("mhtml");
     suggested_path = suggested_path.ReplaceExtension(default_extension);
-  } else if (can_save_as_complete) {
+  } else if (can_save_as_complete_) {
+    // NOTE: this branch will never run on chromeos because ShouldSaveAsHTML()
+    // == can_save_as_complete_ on chromeos.
     bool add_extra_extension = false;
     base::FilePath::StringType extra_extension;
     if (!suggested_path.Extension().empty() &&
@@ -183,46 +229,59 @@ void SavePackageFilePicker::SetShouldPromptUser(bool should_prompt) {
   g_should_prompt_for_filename = should_prompt;
 }
 
-void SavePackageFilePicker::FileSelected(const base::FilePath& path,
-                                         int index,
-                                         void* unused_params) {
+void SavePackageFilePicker::FileSelected(
+    const base::FilePath& path_const, int index, void* unused_params) {
   RenderProcessHost* process = RenderProcessHost::FromID(render_process_id_);
-  if (process) {
-    SavePageType save_type = content::SAVE_PAGE_TYPE_UNKNOWN;
-    PrefService* prefs = Profile::FromBrowserContext(
-        process->GetBrowserContext())->GetPrefs();
-    if (ShouldSaveAsMHTML()) {
-      save_type = content::SAVE_PAGE_TYPE_AS_MHTML;
-    } else {
-      // The option index is not zero-based.
-      DCHECK(index >= kSelectFileHtmlOnlyIndex &&
-             index <= kSelectFileCompleteIndex);
-      save_type = kIndexToSaveType[index];
-      if (select_file_dialog_ &&
-          select_file_dialog_->HasMultipleFileTypeChoices())
-        prefs->SetInteger(prefs::kSaveFileType, save_type);
-    }
-
-    UMA_HISTOGRAM_ENUMERATION("Download.SavePageType",
-                              save_type,
-                              content::SAVE_PAGE_TYPE_MAX);
+  if (!process) {
+    delete this;

[Randy Smith (Not in Mondays), 2013/05/01 19:18:43]

Suggestion: I'm a bit uncomfortable with the number of different exit points
from this routine, all of them with a "delete this" in them.  Seems like an
invitation for drift leading to leaks.  It's not an idiom I've seen before, but
maybe setup a scoped_ptr<SavePackageFilePicker> delete_on_exit(this); at
function entry?

[benjhayden, 2013/05/06 20:05:19]

Done.

+    return;
+  }
+  Profile* profile = Profile::FromBrowserContext(process->GetBrowserContext());
+  SavePageType save_type = content::SAVE_PAGE_TYPE_UNKNOWN;
 
-    StringPrefMember save_file_path;
-    save_file_path.Init(prefs::kSaveFileDefaultDirectory, prefs);
-#if defined(OS_POSIX)
-    std::string path_string = path.DirName().value();
-#elif defined(OS_WIN)
-    std::string path_string = WideToUTF8(path.DirName().value());
+  if (ShouldSaveAsMHTML()) {
+    save_type = content::SAVE_PAGE_TYPE_AS_MHTML;
+  } else {
+#if defined(OS_CHROMEOS)
+    save_type = content::SAVE_PAGE_TYPE_AS_ONLY_HTML;
+#else
+    // The option index is not zero-based.
+    DCHECK(index >= kSelectFileHtmlOnlyIndex &&
+           index <= kSelectFileCompleteIndex);
+    save_type = kIndexToSaveType[index];
+    if (select_file_dialog_ &&
+        select_file_dialog_->HasMultipleFileTypeChoices())
+      download_prefs_->SetSaveFileType(save_type);
 #endif
-    // If user change the default saving directory, we will remember it just
-    // like IE and FireFox.
-    if (!process->GetBrowserContext()->IsOffTheRecord() &&
-        save_file_path.GetValue() != path_string)
-      save_file_path.SetValue(path_string);
+  }
+
+  UMA_HISTOGRAM_ENUMERATION("Download.SavePageType",
+                            save_type,
+                            content::SAVE_PAGE_TYPE_MAX);
 
-    callback_.Run(path, save_type, base::Bind(&OnSavePackageDownloadCreated));
+  base::FilePath path(path_const);
+  file_util::NormalizeFileNameEncoding(&path);
+
+  if (!profile->IsOffTheRecord())
+    download_prefs_->SetSaveFilePath(path.DirName());

[Randy Smith (Not in Mondays), 2013/05/01 19:18:43]

This doesn't look right to me, though I'm not sure of the right pattern.  I
think the *behavior* we want for incognito is for the path preference to sticky
inside the profile, and to disappear when the profile does.  If I was addressing
this from scratch, the way I'd do that is make sure that the download_prefs_
associated with an incognito profile were initialized from the corresponding
non-incognito profile but not synced out to disk when the profile was destroyed.
 I haven't checked, but I think it's worth at least a bit of code tracing to
find out if this is what's happening or not; if it is, you can skip the
conditional.  If it's not, I'd put at least a bit of thought into how you might
achieve the desired behavior before making it non-sticky within incognito
profiles.

[benjhayden, 2013/05/06 20:05:19]

This might be a can of worms.
Up until now, DownloadPrefs for incognito profiles leaked everything to the
on-record pref store.
I can see the privacy issue if directory names from incognito sessions were
leaked to the on-record pref store, so I added 'base::FilePath
DownloadPrefs::incognito_save_file_path_'. I can't quite see the same privacy
issue for the other fields there, so I only did this for save_file_path.
PTAL at download_prefs.cc.

+
+#if defined(OS_CHROMEOS)
+  if (drive::util::IsUnderDriveMountPoint(path)) {
+    // Here's a map to the callback chain:
+    // SubstituteDriveDownloadPath ->
+    //   ContinueSettingUpDriveDownload ->
+    //     callback_ = SavePackage::OnPathPicked ->
+    //       download_created_callback = OnSavePackageDownloadCreatedChromeOS
+    drive::DownloadHandler* drive_download_handler =
+        drive::DownloadHandler::GetForProfile(profile);
+    drive_download_handler->SubstituteDriveDownloadPath(path, NULL, base::Bind(
+        &ContinueSettingUpDriveDownload, callback_, save_type, profile, path));
+    delete this;
+    return;
   }
+#endif
 
+  callback_.Run(path, save_type, base::Bind(&OnSavePackageDownloadCreated));
   delete this;
 }