[downloads] Move platform specific code out of DownloadTargetDeterminer.

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index c151f3e44f63f0d7c13715d52176b9115e634039..a443135c69208b518f41e5cb1cb2b74d962e6bbd 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -58,6 +58,8 @@
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
 #include "chrome/browser/android/download/chrome_download_manager_overwrite_infobar_delegate.h"
+#include "chrome/browser/android/download/download_controller.h"
+#include "chrome/browser/android/download/download_manager_service.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #endif
 
@@ -192,6 +194,16 @@ enum DangerousFileReason {
   DANGEROUS_FILE_REASON_MAX
 };
 
+// On Android, Chrome wants to warn the user of file overwrites rather than
+// uniquify.
+#if BUILDFLAG(ANDROID_JAVA_UI)
+const DownloadPathReservationTracker::FilenameConflictAction
+    kDefaultPlatformConflictAction = DownloadPathReservationTracker::PROMPT;
+#else
+const DownloadPathReservationTracker::FilenameConflictAction
+    kDefaultPlatformConflictAction = DownloadPathReservationTracker::UNIQUIFY;
+#endif
+
 }  // namespace
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
@@ -277,8 +289,7 @@ bool ChromeDownloadManagerDelegate::DetermineDownloadTarget(
   DownloadTargetDeterminer::Start(
       download,
       GetPlatformDownloadPath(profile_, download, PLATFORM_TARGET_PATH),
-      download_prefs_.get(),
-      this,
+      kDefaultPlatformConflictAction, download_prefs_.get(), this,
       target_determined_callback);
   return true;
 }
@@ -598,7 +609,7 @@ void ChromeDownloadManagerDelegate::ReserveVirtualPath(
   // TODO(asanka): Handle path reservations for virtual paths as well.
   //               http://crbug.com/151618
   if (drive::util::IsUnderDriveMountPoint(virtual_path)) {
-    callback.Run(virtual_path, true);
+    callback.Run(virtual_path, DownloadTargetResult::SUCCESS);
     return;
   }
 #endif
@@ -611,18 +622,56 @@ void ChromeDownloadManagerDelegate::ReserveVirtualPath(
       callback);
 }
 
-void ChromeDownloadManagerDelegate::PromptUserForDownloadPath(
+void ChromeDownloadManagerDelegate::RequestConfirmation(
     DownloadItem* download,
     const base::FilePath& suggested_path,
+    DownloadConfirmationReason reason,
     const DownloadTargetDeterminerDelegate::FileSelectedCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
 #if BUILDFLAG(ANDROID_JAVA_UI)
-  chrome::android::ChromeDownloadManagerOverwriteInfoBarDelegate::Create(
-      InfoBarService::FromWebContents(download->GetWebContents()), download,
-      suggested_path, callback);
-#else
+  switch (reason) {
+    case DownloadConfirmationReason::NONE:
+      NOTREACHED();
+      return;
+
+    case DownloadConfirmationReason::TARGET_NOT_WRITEABLE:
+      DownloadManagerService::OnDownloadCanceled(
+          download, DownloadController::CANCEL_REASON_NO_EXTERNAL_STORAGE);
+      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+      return;
+
+    case DownloadConfirmationReason::NAME_TOO_LONG:
+    case DownloadConfirmationReason::TARGET_NO_SPACE:

[svaldez, 2016/10/28 17:29:35]

Should this actually continue?

[asanka, 2016/11/07 19:50:15]

This one's a bit odd in that the purpose of continuing is to force the download
back into its interrupted state. I.e. attempting to continue to download will
interrupt it again if there's still no space.

On Android there's [currently] only one download location. Hence the user
doesn't have much of a choice on where to download. Hence we show the download
as interrupted and force a manual resumption, but without the extra prompt that
we do on desktop.

Added a comment to this effect.

+    case DownloadConfirmationReason::SAVE_AS:
+    case DownloadConfirmationReason::PREFERENCE:
+      callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
+                   suggested_path);
+      return;
+
+    case DownloadConfirmationReason::TARGET_CONFLICT:
+      if (download->GetWebContents()) {
+        chrome::android::ChromeDownloadManagerOverwriteInfoBarDelegate::Create(
+            InfoBarService::FromWebContents(download->GetWebContents()),
+            download, suggested_path, callback);
+        return;
+      }
+    // Fallthrough
+
+    // If we cannot reserve the path and the WebContent is already gone, there
+    // is no way to prompt user for an infobar. This could happen after chrome
+    // gets killed, and user tries to resume a download while another app has
+    // created the target file (not the temporary .crdownload file).
+    case DownloadConfirmationReason::UNEXPECTED:
+      DownloadManagerService::OnDownloadCanceled(
+          download,
+          DownloadController::CANCEL_REASON_CANNOT_DETERMINE_DOWNLOAD_TARGET);
+      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+      return;
+  }
+#else   // !ANDROID_JAVA_UI
   DownloadFilePicker::ShowFilePicker(download, suggested_path, callback);
-#endif
+#endif  // !ANDROID_JAVA_UI
 }
 
 void ChromeDownloadManagerDelegate::DetermineLocalPath(
@@ -760,6 +809,32 @@ void ChromeDownloadManagerDelegate::Observe(
 #endif
 }
 
+namespace {
+
+content::DownloadInterruptReason TargetResultToInterruptReasonAfterCompletion(
+    DownloadTargetResult result) {
+  switch (result) {
+    case DownloadTargetResult::SUCCESS:
+      return content::DOWNLOAD_INTERRUPT_REASON_NONE;
+
+    case DownloadTargetResult::USER_CANCELED:
+      return content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED;
+
+    case DownloadTargetResult::NAME_TOO_LONG:
+    case DownloadTargetResult::CONFLICT:
+    case DownloadTargetResult::PATH_NOT_WRITEABLE:
+      NOTREACHED();  // These cases should've caused a confirmation prompt.
+    // fallthrough
+
+    case DownloadTargetResult::UNEXPECTED:
+      return content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED;
+  }
+  NOTREACHED();

[svaldez, 2016/10/28 17:29:35]

Can these lines be removed so that there's a compile warning whenever new
DownloadTargetResult values are added without this switch being updated?

[asanka, 2016/11/07 19:50:15]

This is already the case. I.e. the switch above still needs to be exhaustive for
the compilation to succeed.

The extra lines are here to appease clang which complains that control reaches
end of non-void function if I don't have them.

+  return content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED;
+}
+
+}  // namespace
+
 void ChromeDownloadManagerDelegate::OnDownloadTargetDetermined(
     int32_t download_id,
     const content::DownloadTargetCallback& callback,
@@ -779,10 +854,10 @@ void ChromeDownloadManagerDelegate::OnDownloadTargetDetermined(
 
     DownloadItemModel(item).SetDangerLevel(target_info->danger_level);
   }
-  callback.Run(target_info->target_path,
-               target_info->target_disposition,
-               target_info->danger_type,
-               target_info->intermediate_path);
+  callback.Run(
+      target_info->target_path, target_info->target_disposition,
+      target_info->danger_type, target_info->intermediate_path,
+      TargetResultToInterruptReasonAfterCompletion(target_info->result));
 }
 
 bool ChromeDownloadManagerDelegate::IsOpenInBrowserPreferreredForFile(