Download later: Plumb download schedule to DownloadTargetInfo.

DownloadSchedule carries application data for download later feature,
this CL plumbs it from DownloadDialogBridge to DownloadTargetInfo.

This is mostly plumbing a base::nullopt through the pipeline. There
will be a few more CLs to plumb it to DownloadItemImpl and the in
progress download db.

Bug: 1078454
Change-Id: I187269c340d775997b6840ba1f16d67815a1b845
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2234129Reviewed-by: Hesen Zhang <hesen@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776366}

chrome/browser/download/android/chrome_duplicate_download_infobar_delegate.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "chrome/browser/download/android/download_controller.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/ui/android/infobars/duplicate_download_infobar.h"
@@ -26,10 +27,14 @@ void CreateNewFileDone(
     download::PathValidationResult result,
     const base::FilePath& target_path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  if (result == download::PathValidationResult::SUCCESS)
-    callback.Run(DownloadConfirmationResult::CONFIRMED, target_path);
-  else
-    callback.Run(DownloadConfirmationResult::FAILED, base::FilePath());
+  if (result == download::PathValidationResult::SUCCESS) {
+    callback.Run(DownloadConfirmationResult::CONFIRMED, target_path,
+                 base::nullopt /*download_schedule*/);
+
+  } else {
+    callback.Run(DownloadConfirmationResult::FAILED, base::FilePath(),
+                 base::nullopt /*download_schedule*/);
+  }
 }
 
 }  // namespace
@@ -101,7 +106,8 @@ bool ChromeDuplicateDownloadInfoBarDelegate::Cancel() {
     return true;
 
   file_selected_callback_.Run(DownloadConfirmationResult::CANCELED,
-                              base::FilePath());
+                              base::FilePath(),
+                              base::nullopt /*download_schedule*/);
   return true;
 }
 

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -254,17 +254,18 @@ void OnDownloadDialogClosed(
   switch (result.location_result) {
     case DownloadLocationDialogResult::USER_CONFIRMED:
       callback.Run(DownloadConfirmationResult::CONFIRMED_WITH_DIALOG,
-                   result.file_path);
+                   result.file_path, std::move(result.download_schedule));
       break;
     case DownloadLocationDialogResult::USER_CANCELED:
-      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                   base::nullopt);
       break;
     case DownloadLocationDialogResult::DUPLICATE_DIALOG:
       // TODO(xingliu): Figure out the dialog behavior on multiple downloads.
       // Currently we just let other downloads continue, which doesn't make
       // sense.
       callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-                   result.file_path);
+                   result.file_path, std::move(result.download_schedule));
       break;
   }
 }
@@ -851,6 +852,8 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!download->IsTransient());
 
+// TODO(xingliu): We should abstract a DownloadFilePicker interface and make all
+// platform use it.
 #if defined(OS_ANDROID)
   content::WebContents* web_contents =
       content::DownloadItemUtils::GetWebContents(download);
@@ -858,20 +861,21 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
     if (reason == DownloadConfirmationReason::SAVE_AS) {
       // If this is a 'Save As' download, just run without confirmation.
       callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-                   suggested_path);
+                   suggested_path, base::nullopt /*download_schedule*/);
     } else if (!web_contents ||
                reason == DownloadConfirmationReason::UNEXPECTED) {
       // If there are no web_contents and there are no errors (ie. location
       // dialog is only being requested because of a user preference), continue.
       if (reason == DownloadConfirmationReason::PREFERENCE) {
         callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-                     suggested_path);
+                     suggested_path, base::nullopt /*download_schedule*/);
         return;
       }
 
       if (reason == DownloadConfirmationReason::TARGET_PATH_NOT_WRITEABLE) {
         OnDownloadCanceled(download, true /* has_no_external_storage */);
-        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                     base::nullopt /*download_schedule*/);
         return;
       }
 
@@ -880,13 +884,15 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
       // gets killed, and user tries to resume a download while another app has
       // created the target file (not the temporary .crdownload file).
       OnDownloadCanceled(download, false /* has_no_external_storage */);
-      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+      callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                   base::nullopt /*download_schedule*/);
     } else if (reason == DownloadConfirmationReason::TARGET_CONFLICT) {
       // If there is a file that already has the same name, try to generate a
       // unique name for the new download (ie. "image (1).png" vs "image.png").
       base::FilePath download_dir;
       if (!base::android::GetDownloadsDirectory(&download_dir)) {
-        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                     base::nullopt /*download_schedule*/);
         return;
       }
 
@@ -941,7 +947,8 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
 
       case DownloadConfirmationReason::TARGET_PATH_NOT_WRITEABLE:
         OnDownloadCanceled(download, true /* has_no_external_storage */);
-        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                     base::nullopt /*download_schedule*/);
         return;
 
       case DownloadConfirmationReason::PREFERENCE:
@@ -957,7 +964,7 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
 
       case DownloadConfirmationReason::SAVE_AS:
         callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-                     suggested_path);
+                     suggested_path, base::nullopt /*download_schedule*/);
         return;
 
       case DownloadConfirmationReason::TARGET_CONFLICT:
@@ -975,7 +982,8 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
       // created the target file (not the temporary .crdownload file).
       case DownloadConfirmationReason::UNEXPECTED:
         OnDownloadCanceled(download, false /* has_no_external_storage */);
-        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath());
+        callback.Run(DownloadConfirmationResult::CANCELED, base::FilePath(),
+                     base::nullopt /*download_schedule*/);
         return;
     }
   }
@@ -999,7 +1007,7 @@ void ChromeDownloadManagerDelegate::OnConfirmationCallbackComplete(
     const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback,
     DownloadConfirmationResult result,
     const base::FilePath& virtual_path) {
-  callback.Run(result, virtual_path);
+  callback.Run(result, virtual_path, base::nullopt /*download_schedule*/);
   if (!file_picker_callbacks_.empty()) {
     base::OnceClosure callback = std::move(file_picker_callbacks_.front());
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -1055,10 +1063,11 @@ void ChromeDownloadManagerDelegate::GenerateUniqueFileNameDone(
     // If user chose not to show download location dialog, uses current unique
     // target path.
     callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-                 target_path);
+                 target_path, base::nullopt /*download_schedule*/);
   } else {
     // If the name generation failed, fail the download.
-    callback.Run(DownloadConfirmationResult::FAILED, base::FilePath());
+    callback.Run(DownloadConfirmationResult::FAILED, base::FilePath(),
+                 base::nullopt /*download_schedule*/);
   }
 }
 

chrome/browser/download/download_target_determiner.cc

@@ -524,12 +524,14 @@ DownloadTargetDeterminer::DoRequestConfirmation() {
 
 void DownloadTargetDeterminer::RequestConfirmationDone(
     DownloadConfirmationResult result,
-    const base::FilePath& virtual_path) {
+    const base::FilePath& virtual_path,
+    base::Optional<download::DownloadSchedule> download_schedule) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(!download_->IsTransient());
   DVLOG(20) << "User selected path:" << virtual_path.AsUTF8Unsafe();
 #if defined(OS_ANDROID)
   is_checking_dialog_confirmed_path_ = false;
+  download_schedule_ = std::move(download_schedule);
 #endif
   if (result == DownloadConfirmationResult::CANCELED) {
     RecordDownloadCancelReason(DownloadCancelReason::kTargetConfirmationResult);
@@ -980,6 +982,7 @@ void DownloadTargetDeterminer::ScheduleCallbackAndDeleteSelf(
   target_info->mime_type = mime_type_;
   target_info->is_filetype_handled_safely = is_filetype_handled_safely_;
   target_info->mixed_content_status = mixed_content_status_;
+  target_info->download_schedule = std::move(download_schedule_);
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,

chrome/browser/download/download_target_determiner.h

@@ -206,8 +206,10 @@ class DownloadTargetDeterminer : public download::DownloadItem::Observer {
 
   // Callback invoked after the file picker completes. Cancels the download if
   // the user cancels the file picker.
-  void RequestConfirmationDone(DownloadConfirmationResult result,
-                               const base::FilePath& virtual_path);
+  void RequestConfirmationDone(
+      DownloadConfirmationResult result,
+      const base::FilePath& virtual_path,
+      base::Optional<download::DownloadSchedule> download_schedule);
 
   // Up until this point, the path that was used is considered to be a virtual
   // path. This step determines the local file system path corresponding to this
@@ -356,6 +358,7 @@ class DownloadTargetDeterminer : public download::DownloadItem::Observer {
   DownloadTargetDeterminerDelegate* delegate_;
   CompletionCallback completion_callback_;
   base::CancelableTaskTracker history_tracker_;
+  base::Optional<download::DownloadSchedule> download_schedule_;
 
   base::WeakPtrFactory<DownloadTargetDeterminer> weak_ptr_factory_{this};
 

chrome/browser/download/download_target_determiner_delegate.h

@@ -8,11 +8,13 @@
 #include <string>
 
 #include "base/callback_forward.h"
+#include "base/optional.h"
 #include "chrome/browser/download/download_confirmation_reason.h"
 #include "chrome/browser/download/download_confirmation_result.h"
 #include "components/download/public/common/download_danger_type.h"
 #include "components/download/public/common/download_item.h"
 #include "components/download/public/common/download_path_reservation_tracker.h"
+#include "components/download/public/common/download_schedule.h"
 
 namespace base {
 class FilePath;
@@ -48,8 +50,10 @@ class DownloadTargetDeterminerDelegate {
   //    selection, then this parameter will be the empty path. On Chrome OS,
   //    this path may contain virtual mount points if the user chose a virtual
   //    path (e.g. Google Drive).
-  typedef base::Callback<void(DownloadConfirmationResult,
-                              const base::FilePath& virtual_path)>
+  typedef base::Callback<void(
+      DownloadConfirmationResult,
+      const base::FilePath& virtual_path,
+      base::Optional<download::DownloadSchedule> download_schedule)>
       ConfirmationCallback;
 
   // Callback to be invoked when DetermineLocalPath() completes. The argument

chrome/browser/download/download_target_determiner_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
@@ -118,12 +119,16 @@ ACTION_P(ScheduleCallback, result0) {
       FROM_HERE, base::BindOnce(std::move(arg0), result0));
 }
 
-// Similar to ScheduleCallback, but binds 2 arguments.
 ACTION_P2(ScheduleCallback2, result0, result1) {
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(arg0), result0, result1));
 }
 
+ACTION_P3(ScheduleCallback3, result0, result1, result2) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(arg0), result0, result1, result2));
+}
+
 // Used with DownloadTestCase. Indicates the type of test case. The expectations
 // for the test is set based on the type.
 enum TestCaseType {
@@ -572,7 +577,8 @@ void MockDownloadTargetDeterminerDelegate::NullPromptUser(
     const base::FilePath& suggested_path,
     DownloadConfirmationReason reason,
     const ConfirmationCallback& callback) {
-  callback.Run(DownloadConfirmationResult::CONFIRMED, suggested_path);
+  callback.Run(DownloadConfirmationResult::CONFIRMED, suggested_path,
+               base::nullopt);
 }
 
 // static
@@ -659,8 +665,9 @@ TEST_F(DownloadTargetDeterminerTest, CancelSaveAs) {
 
        EXPECT_LOCAL_PATH}};
   ON_CALL(*delegate(), RequestConfirmation(_, _, _, _))
-      .WillByDefault(WithArg<3>(ScheduleCallback2(
-          DownloadConfirmationResult::CANCELED, base::FilePath())));
+      .WillByDefault(
+          WithArg<3>(ScheduleCallback3(DownloadConfirmationResult::CANCELED,
+                                       base::FilePath(), base::nullopt)));
   RunTestCasesWithActiveItem(kCancelSaveAsTestCases,
                              base::size(kCancelSaveAsTestCases));
 }
@@ -931,9 +938,9 @@ TEST_F(DownloadTargetDeterminerTest, DefaultVirtual) {
     EXPECT_CALL(*delegate(), RequestConfirmation(
                                  _, test_virtual_dir().AppendASCII("bar.txt"),
                                  DownloadConfirmationReason::SAVE_AS, _))
-        .WillOnce(WithArg<3>(
-            ScheduleCallback2(DownloadConfirmationResult::CONFIRMED,
-                              test_virtual_dir().AppendASCII("prompted.txt"))));
+        .WillOnce(WithArg<3>(ScheduleCallback3(
+            DownloadConfirmationResult::CONFIRMED,
+            test_virtual_dir().AppendASCII("prompted.txt"), base::nullopt)));
     RunTestCasesWithActiveItem(&kSaveAsToVirtualDir, 1);
   }
 
@@ -955,9 +962,10 @@ TEST_F(DownloadTargetDeterminerTest, DefaultVirtual) {
     EXPECT_CALL(*delegate(), RequestConfirmation(
                                  _, test_virtual_dir().AppendASCII("bar.txt"),
                                  DownloadConfirmationReason::SAVE_AS, _))
-        .WillOnce(WithArg<3>(ScheduleCallback2(
+        .WillOnce(WithArg<3>(ScheduleCallback3(
             DownloadConfirmationResult::CONFIRMED,
-            GetPathInDownloadDir(FILE_PATH_LITERAL("foo-x.txt")))));
+            GetPathInDownloadDir(FILE_PATH_LITERAL("foo-x.txt")),
+            base::nullopt)));
     RunTestCasesWithActiveItem(&kSaveAsToLocalDir, 1);
   }
 
@@ -1399,9 +1407,10 @@ TEST_F(DownloadTargetDeterminerTest, ContinueWithoutConfirmation_SaveAs) {
       RequestConfirmation(
           _, GetPathInDownloadDir(FILE_PATH_LITERAL("save-as.kindabad")),
           DownloadConfirmationReason::SAVE_AS, _))
-      .WillOnce(WithArg<3>(ScheduleCallback2(
+      .WillOnce(WithArg<3>(ScheduleCallback3(
           DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION,
-          GetPathInDownloadDir(FILE_PATH_LITERAL("foo.kindabad")))));
+          GetPathInDownloadDir(FILE_PATH_LITERAL("foo.kindabad")),
+          base::nullopt)));
   RunTestCasesWithActiveItem(&kTestCase, 1);
 }
 
@@ -1425,9 +1434,10 @@ TEST_F(DownloadTargetDeterminerTest, ContinueWithConfirmation_SaveAs) {
       RequestConfirmation(
           _, GetPathInDownloadDir(FILE_PATH_LITERAL("save-as.kindabad")),
           DownloadConfirmationReason::SAVE_AS, _))
-      .WillOnce(WithArg<3>(ScheduleCallback2(
+      .WillOnce(WithArg<3>(ScheduleCallback3(
           DownloadConfirmationResult::CONFIRMED,
-          GetPathInDownloadDir(FILE_PATH_LITERAL("foo.kindabad")))));
+          GetPathInDownloadDir(FILE_PATH_LITERAL("foo.kindabad")),
+          base::nullopt)));
   RunTestCasesWithActiveItem(&kTestCase, 1);
 }
 

chrome/browser/download/download_target_info.h

@@ -8,9 +8,11 @@
 #include <string>
 
 #include "base/files/file_path.h"
+#include "base/optional.h"
 #include "components/download/public/common/download_danger_type.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_item.h"
+#include "components/download/public/common/download_schedule.h"
 #include "components/safe_browsing/core/proto/download_file_types.pb.h"
 
 struct DownloadTargetInfo {
@@ -80,6 +82,9 @@ struct DownloadTargetInfo {
 
   // What sort of blocking should be used if the download is of mixed content.
   download::DownloadItem::MixedContentStatus mixed_content_status;
+
+  // Defines when to start the download, used by download later feature.
+  base::Optional<download::DownloadSchedule> download_schedule;
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_TARGET_INFO_H_

components/download/public/common/download_schedule.cc

@@ -18,4 +18,9 @@ DownloadSchedule::DownloadSchedule(const DownloadSchedule&) = default;
 
 DownloadSchedule::~DownloadSchedule() = default;
 
+bool DownloadSchedule::operator==(const DownloadSchedule& other) const {
+  return only_on_wifi_ == other.only_on_wifi() &&
+         start_time_ == other.start_time();
+}
+
 }  // namespace download

components/download/public/common/download_schedule.h

@@ -20,6 +20,8 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadSchedule {
   DownloadSchedule(const DownloadSchedule&);
   ~DownloadSchedule();
 
+  bool operator==(const DownloadSchedule&) const;
+
   bool only_on_wifi() const { return only_on_wifi_; }
 
   const base::Optional<base::Time>& start_time() const { return start_time_; }