Download later: Implement core DownloadItemImpl logic.

The download later check is after the intermediate file rename, right
before MaybeComplete. If any other interrupt reason happens, we use
the existing logic to handle.

When Resume() is called, we clear the DownloadSchedule struct. Since
only auto resumption handler or the user will trigger it.

Bug: 1078454
Change-Id: I0f442b662e94fb55991796a99fb226d2c8cc7998
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2249039Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780861}

components/download/internal/common/download_item_impl.cc

@@ -618,6 +618,7 @@ void DownloadItemImpl::Resume(bool user_resume) {
       paused_ = false;
       if (auto_resume_count_ >= kMaxAutoResumeAttempts) {
         RecordAutoResumeCountLimitReached(GetLastReason());
+        UpdateObservers();
         return;
       }
 
@@ -640,6 +641,7 @@ void DownloadItemImpl::UpdateResumptionInfo(bool user_resume) {
   }
 
   auto_resume_count_ = user_resume ? 0 : ++auto_resume_count_;
+  download_schedule_ = base::nullopt;
 }
 
 void DownloadItemImpl::Cancel(bool user_cancel) {
@@ -1670,6 +1672,15 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
     return;
   }
 
+  if (base::FeatureList::IsEnabled(features::kDownloadLater)) {
+    // If the user select to download on wifi only, use |allow_metered_| to
+    // enforce it. If the user select to download later, allow download on
+    // metered network.
+    download_schedule_ = std::move(download_schedule);
+    if (download_schedule.has_value())
+      allow_metered_ = !download_schedule->only_on_wifi();
+  }
+
   // There were no other pending errors, and we just failed to determined the
   // download target. The target path, if it is non-empty, should be considered
   // suspect. The safe option here is to interrupt the download without doing an
@@ -1807,6 +1818,12 @@ void DownloadItemImpl::OnTargetResolved() {
     return;
   }
 
+  // The download will be started later, interrupt it for now.
+  if (MaybeDownloadLater()) {
+    UpdateObservers();
+    return;
+  }
+
   TransitionTo(IN_PROGRESS_INTERNAL);
   // TODO(asanka): Calling UpdateObservers() prior to MaybeCompleteDownload() is
   // not safe. The download could be in an underminate state after invoking
@@ -1815,6 +1832,31 @@ void DownloadItemImpl::OnTargetResolved() {
   MaybeCompleteDownload();
 }
 
+bool DownloadItemImpl::MaybeDownloadLater() {
+  if (!base::FeatureList::IsEnabled(features::kDownloadLater) ||
+      !download_schedule_.has_value()) {
+    return false;
+  }
+
+  bool network_type_ok = !download_schedule_->only_on_wifi() ||
+                         !delegate_->IsActiveNetworkMetered();
+  bool should_start_later = false;
+  if (download_schedule_->start_time().has_value() &&
+      download_schedule_->start_time() > base::Time::Now()) {
+    should_start_later = true;
+  }
+
+  if (!network_type_ok || should_start_later) {
+    // TODO(xingliu): Maybe add a new interrupt reason for download later
+    // feature.
+    InterruptWithPartialState(GetReceivedBytes(), std::move(hash_state_),
+                              DOWNLOAD_INTERRUPT_REASON_CRASH);
+    return true;
+  }
+
+  return false;
+}
+
 // When SavePackage downloads MHTML to GData (see
 // SavePackageFilePickerChromeOS), GData calls MaybeCompleteDownload() like it
 // does for non-SavePackage downloads, but SavePackage downloads never satisfy
@@ -2396,8 +2438,11 @@ void DownloadItemImpl::SetFullPath(const base::FilePath& new_path) {
 void DownloadItemImpl::AutoResumeIfValid() {
   DVLOG(20) << __func__ << "() " << DebugString(true);
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  ResumeMode mode = GetResumeMode();
 
+  if (download_schedule_.has_value())
+    return;
+
+  ResumeMode mode = GetResumeMode();
   if (mode != ResumeMode::IMMEDIATE_RESTART &&
       mode != ResumeMode::IMMEDIATE_CONTINUE) {
     return;

components/download/internal/common/download_item_impl_unittest.cc

@@ -21,10 +21,12 @@
 #include "base/memory/ptr_util.h"
 #include "base/test/gmock_move_support.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread.h"
 #include "components/download/public/common/download_create_info.h"
 #include "components/download/public/common/download_destination_observer.h"
+#include "components/download/public/common/download_features.h"
 #include "components/download/public/common/download_file_factory.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_item_impl_delegate.h"
@@ -55,7 +57,6 @@ const base::FilePath::CharType kDummyIntermediatePath[] =
     FILE_PATH_LITERAL("/testpathx");
 
 namespace download {
-
 namespace {
 
 template <typename T>
@@ -100,6 +101,7 @@ class MockDelegate : public DownloadItemImplDelegate {
   MOCK_METHOD1(DownloadOpened, void(DownloadItemImpl*));
   MOCK_METHOD1(DownloadRemoved, void(DownloadItemImpl*));
   MOCK_CONST_METHOD0(IsOffTheRecord, bool());
+  MOCK_METHOD(bool, IsActiveNetworkMetered, (), (const override));
 
   void VerifyAndClearExpectations() {
     ::testing::Mock::VerifyAndClearExpectations(this);
@@ -112,6 +114,7 @@ class MockDelegate : public DownloadItemImplDelegate {
         .WillRepeatedly(Return(false));
     EXPECT_CALL(*this, ShouldOpenDownload_(_, _)).WillRepeatedly(Return(true));
     EXPECT_CALL(*this, IsOffTheRecord()).WillRepeatedly(Return(false));
+    EXPECT_CALL(*this, IsActiveNetworkMetered).WillRepeatedly(Return(false));
   }
 };
 
@@ -212,8 +215,6 @@ const uint8_t kHashOfTestData1[] = {
     0xef, 0x9c, 0x92, 0x33, 0x36, 0xe1, 0x06, 0x53, 0xfe, 0xc1, 0x95,
     0xf4, 0x93, 0x45, 0x8b, 0x3b, 0x21, 0x89, 0x0e, 0x1b, 0x97};
 
-}  // namespace
-
 class DownloadItemTest : public testing::Test {
  public:
   DownloadItemTest()
@@ -298,7 +299,18 @@ class DownloadItemTest : public testing::Test {
     EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
     EXPECT_TRUE(item->GetTargetFilePath().empty());
     DownloadItemImplDelegate::DownloadTargetCallback callback;
-    MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+    MockDownloadFile* file = CallDownloadItemStart(item, &callback);
+    DoRenameAndRunTargetCallback(item, file, std::move(callback), danger_type,
+                                 base::nullopt);
+    return file;
+  }
+
+  void DoRenameAndRunTargetCallback(
+      DownloadItemImpl* item,
+      MockDownloadFile* download_file,
+      DownloadItemImplDelegate::DownloadTargetCallback callback,
+      DownloadDangerType danger_type,
+      base::Optional<DownloadSchedule> download_schedule) {
     base::FilePath target_path(kDummyTargetPath);
     base::FilePath intermediate_path(kDummyIntermediatePath);
     auto task_runner = base::ThreadTaskRunnerHandle::Get();
@@ -314,9 +326,8 @@ class DownloadItemTest : public testing::Test {
     std::move(callback).Run(
         target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE, danger_type,
         DownloadItem::MixedContentStatus::UNKNOWN, intermediate_path,
-        base::nullopt /*download_schedule*/, DOWNLOAD_INTERRUPT_REASON_NONE);
+        download_schedule, DOWNLOAD_INTERRUPT_REASON_NONE);
     task_environment_.RunUntilIdle();
-    return download_file;
   }
 
   void DoDestinationComplete(DownloadItemImpl* item,
@@ -2180,8 +2191,6 @@ TEST_F(DownloadItemTest, AnnotationWithEmptyURLInIncognito) {
   task_environment_.RunUntilIdle();
 }
 
-namespace {
-
 // The DownloadItemDestinationUpdateRaceTest fixture (defined below) is used to
 // test for race conditions between download destination events received via the
 // DownloadDestinationObserver interface, and the target determination logic.
@@ -2406,8 +2415,6 @@ INSTANTIATE_TEST_SUITE_P(Failure,
                          DownloadItemDestinationUpdateRaceTest,
                          ::testing::ValuesIn(GenerateFailingEventLists()));
 
-}  // namespace
-
 // Run through the DII workflow but the embedder cancels the download at target
 // determination.
 TEST_P(DownloadItemDestinationUpdateRaceTest, DownloadCancelledByUser) {
@@ -2600,4 +2607,83 @@ TEST_P(DownloadItemDestinationUpdateRaceTest, IntermediateRenameSucceeds) {
   task_environment_.RunUntilIdle();
 }
 
+// --------------------------------------------------------------
+// Download later feature test.
+struct DownloadLaterTestParam {
+  // Input to build DownloadSchedule and config network type.
+  bool only_on_wifi;
+  base::Optional<base::Time> start_time;
+  bool is_active_network_metered;
+
+  // Output and expectation.
+  DownloadItem::DownloadState state;
+  bool allow_metered;
+};
+
+std::vector<DownloadLaterTestParam> DownloadLaterTestParams() {
+  std::vector<DownloadLaterTestParam> params;
+  // Required wifi, and currently on wifi, the download won't stop.
+  params.push_back(
+      {true, base::nullopt, false, DownloadItem::IN_PROGRESS, false});
+  // Don't require wifi, and currently not on wifi, the download won't stop.
+  params.push_back(
+      {false, base::nullopt, true, DownloadItem::IN_PROGRESS, true});
+  // Download later, will be interrupted.
+  auto future_time = base::Time::Now() + base::TimeDelta::FromDays(10);
+  params.push_back({false, future_time, true, DownloadItem::INTERRUPTED, true});
+  return params;
+}
+
+class DownloadLaterTest
+    : public DownloadItemTest,
+      public ::testing::WithParamInterface<DownloadLaterTestParam> {
+ public:
+  DownloadLaterTest() = default;
+  ~DownloadLaterTest() override = default;
+};
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         DownloadLaterTest,
+                         testing::ValuesIn(DownloadLaterTestParams()));
+
+TEST_P(DownloadLaterTest, TestAll) {
+  const auto& param = GetParam();
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kDownloadLater);
+
+  DownloadItemImpl* item = CreateDownloadItem();
+
+  // Setup network type and download schedule.
+  EXPECT_CALL(*mock_delegate(), IsActiveNetworkMetered)
+      .WillRepeatedly(Return(param.is_active_network_metered));
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_TRUE(item->GetTargetFilePath().empty());
+  DownloadItemImplDelegate::DownloadTargetCallback callback;
+  MockDownloadFile* download_file = CallDownloadItemStart(item, &callback);
+
+  if (param.state == DownloadItem::INTERRUPTED) {
+    EXPECT_CALL(*download_file, Detach());
+  }
+
+  base::Optional<DownloadSchedule> download_schedule =
+      base::make_optional<DownloadSchedule>(param.only_on_wifi,
+                                            param.start_time);
+
+  DoRenameAndRunTargetCallback(item, download_file, std::move(callback),
+                               DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                               std::move(download_schedule));
+
+  // Verify final states.
+  ASSERT_EQ(param.allow_metered, item->AllowMetered());
+  ASSERT_EQ(param.state, item->GetState());
+  ASSERT_EQ(0, item->GetAutoResumeCount())
+      << "Download should not be auto resumed with DownloadSchedule.";
+  if (param.state == DownloadItem::INTERRUPTED) {
+    ASSERT_EQ(DOWNLOAD_INTERRUPT_REASON_CRASH, item->GetLastReason());
+  }
+
+  CleanupItem(item, download_file, param.state);
+}
+
+}  // namespace
 }  // namespace download

components/download/public/common/download_features.cc

@@ -30,10 +30,10 @@ const base::Feature kParallelDownloading {
 #endif
 };
 
-#if defined(OS_ANDROID)
 const base::Feature kDownloadLater{"DownloadLater",
                                    base::FEATURE_DISABLED_BY_DEFAULT};
 
+#if defined(OS_ANDROID)
 const base::Feature kRefreshExpirationDate{"RefreshExpirationDate",
                                            base::FEATURE_ENABLED_BY_DEFAULT};
 #endif

components/download/public/common/download_features.h

@@ -23,10 +23,10 @@ COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature
 // Whether a download can be handled by parallel jobs.
 COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature kParallelDownloading;
 
-#if defined(OS_ANDROID)
 // Whether to enable download later feature.
 COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature kDownloadLater;
 
+#if defined(OS_ANDROID)
 // Whether download expiration date will be refreshed on resumption.
 COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature kRefreshExpirationDate;
 #endif

components/download/public/common/download_item_impl.h

@@ -570,6 +570,10 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItemImpl
 
   void OnTargetResolved();
 
+  // If |download_schedule_| presents, maybe interrupt the download and start
+  // later. Returns whether the download should be started later.
+  bool MaybeDownloadLater();
+
   // If all pre-requisites have been met, complete download processing, i.e. do
   // internal cleanup, file rename, and potentially auto-open.  (Dangerous
   // downloads still may block on user acceptance after this point.)