Skip to content

GitLab

T
tangled
Commit 182706e5 authored by David Black's avatar David Black Committed by Commit Bot
Browse files
Options

Persist downloads to holding space persistence. 

Previously downloads were only persisted by the DownloadManager. Moving
forward, we want to be able to update holding space items when their
backing files are renamed/moved, but this will leave us unable to
restore any renamed/moved downloads unless we persist them ourselves.

Persisting downloads ourselves also greatly simplifies holding space
restoration on start up due to removal of the one-off case for
downloads.

Bug: 1136173
Change-Id: I867a793449f9668f9899c75f057af7a0d35174af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2458387Reviewed-by: default avatarAhmed Mehfooz <amehfooz@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#814898}
parent 9880f3be
Show whitespace changes
Inline Side-by-side
Showing with 12 additions and 298 deletions
chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.cc
View file @ 182706e5
......@@ -6,10 +6,7 @@
#include <vector>
#include "ash/public/cpp/holding_space/holding_space_constants.h"
#include "ash/public/cpp/holding_space/holding_space_prefs.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
#include "content/public/browser/browser_context.h"
namespace ash {
......@@ -18,22 +15,6 @@ namespace {
content::DownloadManager* download_manager_for_testing = nullptr;
// Helpers ---------------------------------------------------------------------
// Returns true if `download` is sufficiently recent, false otherwise.
bool IsRecentEnough(Profile* profile, const download::DownloadItem* download) {
const base::Time end_time = download->GetEndTime();
// A `download` must be more recent than the time of the holding space feature
// first becoming available.
PrefService* prefs = profile->GetPrefs();
if (end_time < holding_space_prefs::GetTimeOfFirstAvailability(prefs).value())
return false;
// A `download` must be more recent that `kMaxFileAge`.
return end_time >= base::Time::Now() - kMaxFileAge;
}
} // namespace
// HoldingSpaceDownloadsDelegate -----------------------------------------------
......@@ -41,11 +22,9 @@ bool IsRecentEnough(Profile* profile, const download::DownloadItem* download) {
HoldingSpaceDownloadsDelegate::HoldingSpaceDownloadsDelegate(
Profile* profile,
HoldingSpaceModel* model,
ItemDownloadedCallback item_downloaded_callback,
DownloadsRestoredCallback downloads_restored_callback)
ItemDownloadedCallback item_downloaded_callback)
: HoldingSpaceKeyedServiceDelegate(profile, model),
item_downloaded_callback_(item_downloaded_callback),
downloads_restored_callback_(std::move(downloads_restored_callback)) {}
item_downloaded_callback_(item_downloaded_callback) {}
HoldingSpaceDownloadsDelegate::~HoldingSpaceDownloadsDelegate() = default;
......@@ -90,37 +69,18 @@ void HoldingSpaceDownloadsDelegate::OnManagerInitialized() {
download::SimpleDownloadManager::DownloadVector downloads;
download_manager->GetAllDownloads(&downloads);
std::vector<base::FilePath> file_paths;
for (auto* download : downloads) {
switch (download->GetState()) {
case download::DownloadItem::COMPLETE:
if (IsRecentEnough(profile(), download))
file_paths.push_back(download->GetFullPath());
break;
case download::DownloadItem::IN_PROGRESS:
download_item_observer_.Add(download);
break;
case download::DownloadItem::COMPLETE:
case download::DownloadItem::CANCELLED:
case download::DownloadItem::INTERRUPTED:
case download::DownloadItem::MAX_DOWNLOAD_STATE:
break;
}
}
holding_space_util::PartitionFilePathsByExistence(
profile(), file_paths,
base::BindOnce(
[](const base::WeakPtr<HoldingSpaceDownloadsDelegate>& weak_ptr,
std::vector<base::FilePath> existing_file_paths,
std::vector<base::FilePath> non_existing_file_paths) {
if (weak_ptr) {
for (const auto& existing_file_path : existing_file_paths)
weak_ptr->OnDownloadCompleted(existing_file_path);
std::move(weak_ptr->downloads_restored_callback_).Run();
}
},
weak_factory_.GetWeakPtr()));
}
void HoldingSpaceDownloadsDelegate::ManagerGoingDown(
......
chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h
View file @ 182706e5
......@@ -30,14 +30,10 @@ class HoldingSpaceDownloadsDelegate : public HoldingSpaceKeyedServiceDelegate,
using ItemDownloadedCallback =
base::RepeatingCallback<void(const base::FilePath&)>;
// Callback to invoke when all downloads have been restored to holding space.
using DownloadsRestoredCallback = base::OnceClosure;
HoldingSpaceDownloadsDelegate(
Profile* profile,
HoldingSpaceModel* model,
ItemDownloadedCallback item_downloaded_callback,
DownloadsRestoredCallback downloads_restored_callback);
ItemDownloadedCallback item_downloaded_callback);
HoldingSpaceDownloadsDelegate(const HoldingSpaceDownloadsDelegate&) = delete;
HoldingSpaceDownloadsDelegate& operator=(
const HoldingSpaceDownloadsDelegate&) = delete;
......@@ -72,9 +68,6 @@ class HoldingSpaceDownloadsDelegate : public HoldingSpaceKeyedServiceDelegate,
// Callback to invoke when a download is completed.
ItemDownloadedCallback item_downloaded_callback_;
// Callback to invoke when all downloads have been restored to holding space.
DownloadsRestoredCallback downloads_restored_callback_;
ScopedObserver<content::DownloadManager, content::DownloadManager::Observer>
download_manager_observer_{this};
......
chrome/browser/ui/ash/holding_space/holding_space_keyed_service.cc
View file @ 182706e5
......@@ -9,9 +9,7 @@
#include "ash/public/cpp/holding_space/holding_space_item.h"
#include "ash/public/cpp/holding_space/holding_space_metrics.h"
#include "ash/public/cpp/holding_space/holding_space_prefs.h"
#include "base/barrier_closure.h"
#include "base/files/file_path.h"
#include "base/guid.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/chromeos/file_manager/app_id.h"
#include "chrome/browser/chromeos/file_manager/fileapi_util.h"
......@@ -62,14 +60,6 @@ HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
// the first time that holding space became available, this will no-op.
holding_space_prefs::MarkTimeOfFirstAvailability(profile_->GetPrefs());
// Model restoration is a multi-step process, currently consisting of a
// restoration from persistence followed by a restoration of downloads. Once
// all steps have indicated completion, `OnModelFullyRestored()` is invoked.
on_model_partially_restored_callback_ = base::BarrierClosure(
/*number_of_steps_before_fully_restored=*/2,
base::BindOnce(&HoldingSpaceKeyedService::OnModelFullyRestored,
base::Unretained(this)));
// The associated profile may not be ready yet. If it is, we can immediately
// proceed with profile dependent initialization.
ProfileManager* const profile_manager = GetProfileManager();
......@@ -205,9 +195,6 @@ void HoldingSpaceKeyedService::OnProfileReady() {
profile_, &holding_space_model_,
/*item_downloaded_callback=*/
base::BindRepeating(&HoldingSpaceKeyedService::AddDownload,
weak_factory_.GetWeakPtr()),
/*downloads_restored_callback=*/
base::BindOnce(&HoldingSpaceKeyedService::OnDownloadsRestored,
weak_factory_.GetWeakPtr())));
// The `HoldingSpaceFileSystemDelegate` monitors the file system for changes.
......@@ -243,19 +230,10 @@ void HoldingSpaceKeyedService::OnFileRemoved(const base::FilePath& file_path) {
std::cref(file_path)));
}
void HoldingSpaceKeyedService::OnDownloadsRestored() {
for (auto& delegate : delegates_)
delegate->NotifyDownloadsRestored();
on_model_partially_restored_callback_.Run();
}
void HoldingSpaceKeyedService::OnPersistenceRestored() {
for (auto& delegate : delegates_)
delegate->NotifyPersistenceRestored();
on_model_partially_restored_callback_.Run();
}
void HoldingSpaceKeyedService::OnModelFullyRestored() {
HoldingSpaceController::Get()->RegisterClientAndModelForUser(
account_id_, &holding_space_client_, &holding_space_model_);
}
......
chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h
View file @ 182706e5
......@@ -102,16 +102,9 @@ class HoldingSpaceKeyedService : public KeyedService,
// Invoked when the specified `file_path` is removed.
void OnFileRemoved(const base::FilePath& file_path);
// Invoked when all downloads have been restored to holding space.
void OnDownloadsRestored();
// Invoked when holding space persistence has been restored.
void OnPersistenceRestored();
// Invoked when the holding space model has been fully restored. This includes
// both holding space items restored from persistence as well as downloads.
void OnModelFullyRestored();
Profile* const profile_;
const AccountId account_id_;
......@@ -125,12 +118,6 @@ class HoldingSpaceKeyedService : public KeyedService,
// service. They operate autonomously of one another.
std::vector<std::unique_ptr<HoldingSpaceKeyedServiceDelegate>> delegates_;
// A `base::BarrierClosure` that is run when the holding space model has been
// partially restored. This will occur multiple times, after restoration from
// persistence and after restoration of downloads. Once all restoration steps
// have indicated completion, `OnModelFullyRestored()` is invoked.
base::RepeatingClosure on_model_partially_restored_callback_;
ScopedObserver<ProfileManager, ProfileManagerObserver>
profile_manager_observer_{this};
......
chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.cc
View file @ 182706e5
......@@ -21,12 +21,6 @@ HoldingSpaceKeyedServiceDelegate::~HoldingSpaceKeyedServiceDelegate() = default;
void HoldingSpaceKeyedServiceDelegate::Shutdown() {}
void HoldingSpaceKeyedServiceDelegate::NotifyDownloadsRestored() {
DCHECK(is_restoring_downloads_);
is_restoring_downloads_ = false;
OnDownloadsRestored();
}
void HoldingSpaceKeyedServiceDelegate::NotifyPersistenceRestored() {
DCHECK(is_restoring_persistence_);
is_restoring_persistence_ = false;
......@@ -48,8 +42,6 @@ void HoldingSpaceKeyedServiceDelegate::OnHoldingSpaceItemAdded(
void HoldingSpaceKeyedServiceDelegate::OnHoldingSpaceItemRemoved(
const HoldingSpaceItem* item) {}
void HoldingSpaceKeyedServiceDelegate::OnDownloadsRestored() {}
void HoldingSpaceKeyedServiceDelegate::OnPersistenceRestored() {}
} // namespace ash
chrome/browser/ui/ash/holding_space/holding_space_keyed_service_delegate.h
View file @ 182706e5
......@@ -29,10 +29,6 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
// Delegates should perform any necessary clean up.
virtual void Shutdown();
// Invoked by `HoldingSpaceKeyedService` to notify delegates when all
// downloads have been restored to holding space.
void NotifyDownloadsRestored();
// Invoked by `HoldingSpaceKeyedService` to notify delegates when holding
// space persistence has been restored.
void NotifyPersistenceRestored();
......@@ -46,9 +42,6 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
// Returns the holding space model owned by `HoldingSpaceKeyedService`.
const HoldingSpaceModel* model() const { return model_; }
// Returns if downloads are being restored.
bool is_restoring_downloads() const { return is_restoring_downloads_; }
// Returns if persistence is being restored.
bool is_restoring_persistence() const { return is_restoring_persistence_; }
......@@ -57,18 +50,12 @@ class HoldingSpaceKeyedServiceDelegate : public HoldingSpaceModelObserver {
void OnHoldingSpaceItemAdded(const HoldingSpaceItem* item) override;
void OnHoldingSpaceItemRemoved(const HoldingSpaceItem* item) override;
// Invoked when all downloads have been restored to holding space.
virtual void OnDownloadsRestored();
// Invoked when holding space persistence has been restored.
virtual void OnPersistenceRestored();
Profile* const profile_;
const HoldingSpaceModel* const model_;
// If downloads are being restored.
bool is_restoring_downloads_ = true;
// If persistence is being restored.
bool is_restoring_persistence_ = true;
......
chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc
View file @ 182706e5
......@@ -57,7 +57,7 @@ namespace {
std::vector<HoldingSpaceItem::Type> GetHoldingSpaceItemTypes() {
std::vector<HoldingSpaceItem::Type> types;
for (int i = 0; i < static_cast<int>(HoldingSpaceItem::Type::kMaxValue); ++i)
for (int i = 0; i <= static_cast<int>(HoldingSpaceItem::Type::kMaxValue); ++i)
types.push_back(static_cast<HoldingSpaceItem::Type>(i));
return types;
}
......@@ -477,10 +477,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, UpdatePersistentStorage) {
primary_holding_space_service->thumbnail_loader_for_testing(), type,
file_path));
// We do not persist `kDownload` type items.
if (type != HoldingSpaceItem::Type::kDownload)
persisted_holding_space_items.Append(holding_space_item->Serialize());
primary_holding_space_model->AddItem(std::move(holding_space_item));
EXPECT_EQ(*GetProfile()->GetPrefs()->GetList(
......@@ -493,10 +490,7 @@ TEST_F(HoldingSpaceKeyedServiceTest, UpdatePersistentStorage) {
const auto* holding_space_item =
primary_holding_space_model->items()[0].get();
// We do not persist `kDownload` type items.
if (holding_space_item->type() != HoldingSpaceItem::Type::kDownload)
persisted_holding_space_items.Remove(0, /*out_value=*/nullptr);
primary_holding_space_model->RemoveItem(holding_space_item->id());
EXPECT_EQ(*GetProfile()->GetPrefs()->GetList(
......@@ -527,10 +521,6 @@ TEST_F(HoldingSpaceKeyedServiceTest, RestorePersistentStorage) {
// Persist some holding space items of each type.
for (const HoldingSpaceItem::Type type : GetHoldingSpaceItemTypes()) {
// We do not persist `kDownload` type items.
if (type == HoldingSpaceItem::Type::kDownload)
continue;
const base::FilePath file = CreateArbitraryFile(downloads_mount);
const GURL file_system_url = GetFileSystemUrl(GetProfile(), file);
......@@ -629,10 +619,6 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderFilesFromPersistance) {
// Persist some holding space items of each type.
for (const HoldingSpaceItem::Type type : GetHoldingSpaceItemTypes()) {
// We do not persist `kDownload` type items.
if (type == HoldingSpaceItem::Type::kDownload)
continue;
const base::FilePath file = CreateArbitraryFile(downloads_mount);
const GURL file_system_url = GetFileSystemUrl(GetProfile(), file);
......@@ -647,10 +633,10 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderFilesFromPersistance) {
persisted_holding_space_items_before_restoration->Append(
fresh_holding_space_item->Serialize());
// We don't expect the screenshots to still be in persistence or
// restoration after model restoration since the file will be older
// than the time limit for restored files.
if (type != HoldingSpaceItem::Type::kScreenshot) {
// Only pinned files are exempt from age checks. In this test, we
// expect all holding space items of other types to be removed from
// persistence during restoration due to being older than kMaxFileAge.
if (type == HoldingSpaceItem::Type::kPinnedFile) {
persisted_holding_space_items_after_restoration.Append(
fresh_holding_space_item->Serialize());
restored_holding_space_items.push_back(
......@@ -712,82 +698,6 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderFilesFromPersistance) {
persisted_holding_space_items_after_restoration);
}
TEST_F(HoldingSpaceKeyedServiceTest, RetrieveHistory) {
// Create a test downloads mount point.
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;
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");
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));
// Set one item as an download in progress, which will complete afterwards.
if (i == 2) {
{
testing::InSequence s1;
EXPECT_CALL(*item, GetState())
.WillOnce(testing::Return(download::DownloadItem::IN_PROGRESS));
EXPECT_CALL(*item, GetState())
.WillOnce(testing::Return(download::DownloadItem::COMPLETE));
}
} 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();
HoldingSpaceModel* const model = HoldingSpaceController::Get()->model();
ASSERT_EQ(2u, model->items().size());
for (int i = 0; i < static_cast<int>(model->items().size()); ++i) {
EXPECT_EQ(full_paths[i], model->items()[i]->file_path());
EXPECT_EQ(virtual_paths[i],
GetVirtualPathFromUrl(model->items()[i]->file_system_url(),
downloads_mount.name()));
}
// Notify the holding space service of download completion. It should add
// the object to the model.
download::MockDownloadItem* in_progress_item =
static_cast<download::MockDownloadItem*>(download_items_mock[2]);
in_progress_item->NotifyObserversDownloadUpdated();
ASSERT_EQ(3u, model->items().size());
EXPECT_EQ(full_paths[2], model->items()[2]->file_path());
EXPECT_EQ(virtual_paths[2],
GetVirtualPathFromUrl(model->items()[2]->file_system_url(),
downloads_mount.name()));
for (int i = 0; i < 3; i++)
delete download_items_mock[i];
}
TEST_F(HoldingSpaceKeyedServiceTest, AddDownloadItem) {
TestingProfile* profile = GetProfile();
// Create a test downloads mount point.
......@@ -848,89 +758,4 @@ TEST_F(HoldingSpaceKeyedServiceTest, AddDownloadItem) {
downloads_mount.name()));
}
TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderDownloads) {
// Create a test downloads mount point.
ScopedDownloadsMountPoint downloads_mount(GetProfile());
ASSERT_TRUE(downloads_mount.IsValid());
content::DownloadManager::DownloadVector download_items_mock;
base::Time initial_testing_time = base::Time::Now();
content::MockDownloadManager* mock_download_manager = download_manager();
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 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.time_of_first_availability",
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());
}
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
chrome/browser/ui/ash/holding_space/holding_space_persistence_delegate.cc
View file @ 182706e5
......@@ -49,10 +49,6 @@ void HoldingSpacePersistenceDelegate::OnHoldingSpaceItemAdded(
if (is_restoring_persistence())
return;
// `kDownload` type holding space items have their own persistence mechanism.
if (item->type() == HoldingSpaceItem::Type::kDownload)
return;
// Write the new |item| to persistent storage.
ListPrefUpdate update(profile()->GetPrefs(), kPersistencePath);
update->Append(item->Serialize());
......@@ -63,10 +59,6 @@ void HoldingSpacePersistenceDelegate::OnHoldingSpaceItemRemoved(
if (is_restoring_persistence())
return;
// `kDownload` type holding space items have their own persistence mechanism.
if (item->type() == HoldingSpaceItem::Type::kDownload)
return;
// Remove the |item| from persistent storage.
ListPrefUpdate update(profile()->GetPrefs(), kPersistencePath);
update->EraseListValueIf([&item](const base::Value& persisted_item) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment