Only restore downloads since holding space first enabled

Holding space should not show downloads that were created before the
first time it was enabled.
We cache the time it was first enabled in a pref and we use this to set
the time requirement validity only for downloads.
We also moved from comparing the file creation time to comparing the
download end time.

Bug: 1131273
Change-Id: I2b44f4796897aaaf9846a53ea66f9ff36c2392cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2426769Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Ana Salazar <anasalazar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810871}

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.cc

@@ -7,6 +7,7 @@
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "base/barrier_closure.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "content/public/browser/browser_context.h"
 
@@ -76,22 +77,27 @@ void HoldingSpaceDownloadsDelegate::OnManagerInitialized() {
   for (auto* download : downloads) {
     switch (download->GetState()) {
       case download::DownloadItem::COMPLETE: {
-        holding_space_util::ValidityRequirement requirements;
-        requirements.must_be_newer_than = kMaxFileAge;
-        holding_space_util::FilePathValid(
-            profile(), {download->GetFullPath(), requirements},
-            base::BindOnce(
-                [](const base::FilePath& path,
-                   base::RepeatingClosure barrier_closure,
-                   ItemDownloadedCallback callback, bool valid) {
-                  if (valid)
-                    callback.Run(path);
-                  barrier_closure.Run();
-                },
-                download->GetFullPath(), barrier_closure,
-                base::BindRepeating(
-                    &HoldingSpaceDownloadsDelegate::OnDownloadCompleted,
-                    weak_factory_.GetWeakPtr())));
+        base::Time download_end_time = download->GetEndTime();
+        if (download_end_time >=
+                HoldingSpaceKeyedService::GetFirstEnabledTime(profile()) &&
+            download_end_time >= base::Time::Now() - kMaxFileAge) {
+          holding_space_util::FilePathValid(
+              profile(), {download->GetFullPath(), /*requirements=*/{}},
+              base::BindOnce(
+                  [](const base::FilePath& path,
+                     base::RepeatingClosure barrier_closure,
+                     ItemDownloadedCallback callback, bool valid) {
+                    if (valid)
+                      callback.Run(path);
+                    barrier_closure.Run();
+                  },
+                  download->GetFullPath(), barrier_closure,
+                  base::BindRepeating(
+                      &HoldingSpaceDownloadsDelegate::OnDownloadCompleted,
+                      weak_factory_.GetWeakPtr())));
+        } else {
+          barrier_closure.Run();
+        }
       } break;
       case download::DownloadItem::IN_PROGRESS:
         download_item_observer_.Add(download);

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.cc

@@ -19,12 +19,18 @@
 #include "chrome/browser/ui/ash/holding_space/holding_space_persistence_delegate.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "components/account_id/account_id.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/scoped_user_pref_update.h"
 #include "storage/browser/file_system/file_system_url.h"
 
 namespace ash {
 
 namespace {
 
+// Preference path at which we store when holding space was enabled for the
+// first time.
+constexpr char kPrefPathFirstEnabledTime[] = "ash.holding_space.first_enabled";
+
 ProfileManager* GetProfileManager() {
   return g_browser_process->profile_manager();
 }
@@ -62,9 +68,15 @@ HoldingSpaceKeyedService::~HoldingSpaceKeyedService() = default;
 // static
 void HoldingSpaceKeyedService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterTimePref(kPrefPathFirstEnabledTime, base::Time::Now());
   HoldingSpacePersistenceDelegate::RegisterProfilePrefs(registry);
 }
 
+// static
+base::Time HoldingSpaceKeyedService::GetFirstEnabledTime(Profile* profile) {
+  return profile->GetPrefs()->GetTime(kPrefPathFirstEnabledTime);
+}
+
 void HoldingSpaceKeyedService::AddPinnedFile(
     const storage::FileSystemURL& file_system_url) {
   AddItem(HoldingSpaceItem::CreateFileBackedItem(

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h

@@ -51,6 +51,9 @@ class HoldingSpaceKeyedService : public KeyedService,
   // Registers profile preferences for holding space.
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
+  // Get the time when holding space was first enabled.
+  static base::Time GetFirstEnabledTime(Profile* profile);
+
   // Adds a pinned file item identified by the provided file system URL.
   void AddPinnedFile(const storage::FileSystemURL& file_system_url);
 

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc

@@ -723,6 +723,8 @@ TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
 
   content::MockDownloadManager* mock_download_manager = download_manager();
 
+  base::Time initial_testing_time = base::Time::Now();
+
   for (int i = 0; i < 3; ++i) {
     const base::FilePath download_item_virtual_path(
         "Download " + base::NumberToString(i) + ".png");
@@ -745,12 +747,17 @@ TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
     } else {
       EXPECT_CALL(*item, GetState())
           .WillOnce(testing::Return(download::DownloadItem::COMPLETE));
+      EXPECT_CALL(*item, GetEndTime())
+          .WillOnce(testing::Return(initial_testing_time +
+                                    base::TimeDelta::FromHours(1)));
     }
     download_items_mock.push_back(item.release());
   }
   EXPECT_CALL(*mock_download_manager, GetAllDownloads(testing::_))
       .WillOnce(testing::SetArgPointee<0>(download_items_mock));
 
+  holding_space_util::SetNowForTesting(initial_testing_time);
+
   TestingProfile* secondary_profile = CreateSecondaryProfile();
   ActivateSecondaryProfile();
   HoldingSpaceModelAttachedWaiter(secondary_profile).Wait();
@@ -846,41 +853,83 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderDownloads) {
   ScopedDownloadsMountPoint downloads_mount(GetProfile());
   ASSERT_TRUE(downloads_mount.IsValid());
 
-  std::vector<base::FilePath> virtual_paths;
-  std::vector<base::FilePath> full_paths;
   content::DownloadManager::DownloadVector download_items_mock;
+  base::Time initial_testing_time = base::Time::Now();
 
   content::MockDownloadManager* mock_download_manager = download_manager();
 
-  for (int i = 0; i < 3; ++i) {
-    const base::FilePath download_item_virtual_path(
-        "Download " + base::NumberToString(i) + ".png");
-    virtual_paths.push_back(download_item_virtual_path);
-    const base::FilePath download_item_full_path =
-        CreateFile(downloads_mount, download_item_virtual_path,
-                   "download " + base::NumberToString(i));
-    full_paths.push_back(download_item_full_path);
-    std::unique_ptr<download::MockDownloadItem> item(
-        CreateMockDownloadItem(download_item_full_path));
-    EXPECT_CALL(*item, GetState())
-        .WillOnce(testing::Return(download::DownloadItem::COMPLETE));
-    download_items_mock.push_back(item.release());
-  }
+  const base::FilePath download_item_virtual_path("Download.png");
+  const base::FilePath download_item_full_path =
+      CreateFile(downloads_mount, download_item_virtual_path, "download ");
+  std::unique_ptr<download::MockDownloadItem> item(
+      CreateMockDownloadItem(download_item_full_path));
+  EXPECT_CALL(*item, GetState())
+      .WillOnce(testing::Return(download::DownloadItem::COMPLETE));
+
+  // Set an end time one hour from before kMaxFileAge is met, so it is
+  // considered an older download.
+  EXPECT_CALL(*item, GetEndTime())
+      .WillOnce(testing::Return(initial_testing_time - kMaxFileAge -
+                                base::TimeDelta::FromHours(1)));
+  download_items_mock.push_back(item.get());
   EXPECT_CALL(*mock_download_manager, GetAllDownloads(testing::_))
       .WillOnce(testing::SetArgPointee<0>(download_items_mock));
 
-  // Set time to kMaxFileAge from current time, no downloads should be added.
-  holding_space_util::SetNowForTesting(base::Time::Now() + kMaxFileAge);
-
-  TestingProfile* secondary_profile = CreateSecondaryProfile();
+  // Set holding space first enabled time to 1 day before kMaxFileAge is met
+  // from now, so we can make sure downloads are being excluded due to file age
+  // limit, and not due to the holding space first enabled time.
+  TestingProfile* const secondary_profile = CreateSecondaryProfile(
+      base::BindLambdaForTesting([&](TestingPrefStore* pref_store) {
+        base::Time holding_space_start_time =
+            initial_testing_time - kMaxFileAge - base::TimeDelta::FromDays(1);
+        auto time_value = std::make_unique<base::Value>(base::NumberToString(
+            holding_space_start_time.ToDeltaSinceWindowsEpoch()
+                .InMicroseconds()));
+        pref_store->SetValueSilently(
+            "ash.holding_space.first_enabled", std::move(time_value),
+            PersistentPrefStore::DEFAULT_PREF_WRITE_FLAGS);
+      }));
   ActivateSecondaryProfile();
   HoldingSpaceModelAttachedWaiter(secondary_profile).Wait();
 
   HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
   ASSERT_EQ(0u, model->items().size());
+}
 
-  for (int i = 0; i < 3; ++i)
-    delete download_items_mock[i];
+TEST_F(HoldingSpaceKeyedServiceTest,
+       RemoveDownloadsBeforeHoldingSpaceFirstEnabled) {
+  // Create a test downloads mount point.
+  ScopedDownloadsMountPoint downloads_mount(GetProfile());
+  ASSERT_TRUE(downloads_mount.IsValid());
+
+  const base::FilePath download_item_virtual_path("Download.png");
+  const base::FilePath download_item_full_path =
+      CreateFile(downloads_mount, download_item_virtual_path, "download ");
+  std::unique_ptr<download::MockDownloadItem> item(
+      CreateMockDownloadItem(download_item_full_path));
+  EXPECT_CALL(*item, GetState())
+      .WillOnce(testing::Return(download::DownloadItem::COMPLETE));
+
+  // Create a file with a download time set to one hour before current time. The
+  // download will be older than holding space.
+  EXPECT_CALL(*item, GetEndTime())
+      .WillOnce(
+          testing::Return(base::Time::Now() - base::TimeDelta::FromHours(1)));
+          
+  content::DownloadManager::DownloadVector download_items_mock;
+  download_items_mock.push_back(item.get());
+
+  content::MockDownloadManager* mock_download_manager = download_manager();
+
+  EXPECT_CALL(*mock_download_manager, GetAllDownloads(testing::_))
+      .WillOnce(testing::SetArgPointee<0>(download_items_mock));
+
+  TestingProfile* const secondary_profile = CreateSecondaryProfile();
+  ActivateSecondaryProfile();
+  HoldingSpaceModelAttachedWaiter(secondary_profile).Wait();
+
+  HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
+  ASSERT_EQ(0u, model->items().size());
 }
 
 }  // namespace ash