[DownloadBubble] Remove last download time pref
This removes the last download time pref and updates code relying on it
to use the time supplied by DownloadBubbleUpdateService::GetDisplayInfo
instead. This updates the algorithm for calculating the last downloaded
time in the display info, by recording the time when a download is
updated and becomes no longer in-progress. We assume that when a
download is updated to a not-in-progress state, no further updates will
be sent aside from cancellation. In particular, this takes the time when
a download is marked insecure or dangerous (as they are considered not
in-progress).
This is done because:
1. The original reason for persisting the last downloaded time between
browser sessions is no longer relevant, because the download toolbar
button is no longer shown upon browser startup for performance
reasons. (The pref was added in crrev.com/c/3472822.)
2. Reading the pref on every single update may be expensive and doesn't
make a difference most of the time.
The resulting behavior change is that, in the case where a download
finished less than an hour ago in a previous Chrome session and the user
downloads another file (causing the toolbar button to be shown), we will
no longer schedule the toolbar button disappearance for 1 hr after the
original download, but will instead schedule it for 1 hr after the last
download finished *in the current browser session*.
Bug: 1482991
Change-Id: Iff1c2feed2b3f047cd8c952c51d2ff8dec0f7772
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4922235
Reviewed-by: Xinghui Lu <[email protected]>
Commit-Queue: Lily Chen <[email protected]>
Reviewed-by: Gabriel Charette <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1208381}
diff --git a/chrome/browser/download/bubble/download_bubble_ui_controller_unittest.cc b/chrome/browser/download/bubble/download_bubble_ui_controller_unittest.cc
index bc716f0..5c26af3 100644
--- a/chrome/browser/download/bubble/download_bubble_ui_controller_unittest.cc
+++ b/chrome/browser/download/bubble/download_bubble_ui_controller_unittest.cc
@@ -300,13 +300,8 @@
DownloadDangerType::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) {
DCHECK_GT(items_.size(), static_cast<size_t>(item_index));
EXPECT_CALL(item(item_index), GetState()).WillRepeatedly(Return(state));
- if (state == DownloadState::COMPLETE) {
- EXPECT_CALL(item(item_index), IsDone()).WillRepeatedly(Return(true));
- DownloadPrefs::FromDownloadManager(&manager())
- ->SetLastCompleteTime(base::Time::Now());
- } else {
- EXPECT_CALL(item(item_index), IsDone()).WillRepeatedly(Return(false));
- }
+ EXPECT_CALL(item(item_index), IsDone())
+ .WillRepeatedly(Return(state == DownloadState::COMPLETE));
EXPECT_CALL(item(item_index), IsDangerous())
.WillRepeatedly(
Return(danger_type !=
diff --git a/chrome/browser/download/bubble/download_bubble_update_service.cc b/chrome/browser/download/bubble/download_bubble_update_service.cc
index b6dfd32..3fb1520 100644
--- a/chrome/browser/download/bubble/download_bubble_update_service.cc
+++ b/chrome/browser/download/bubble/download_bubble_update_service.cc
@@ -146,15 +146,16 @@
return false;
}
+// `update_is_for_model` is whether the current call to this function was
+// triggered on behalf of `model`.
void UpdateInfoForModel(const DownloadUIModel& model,
+ bool update_is_for_model,
base::Time cutoff_time,
DownloadBubbleDisplayInfo& info) {
if (!ShouldIncludeModel(&model, cutoff_time)) {
return;
}
++info.all_models_size;
- info.last_completed_time =
- std::max(info.last_completed_time, model.GetEndTime());
if (model.GetDangerType() == download::DOWNLOAD_DANGER_TYPE_ASYNC_SCANNING &&
model.GetState() != download::DownloadItem::CANCELLED) {
info.has_deep_scanning = true;
@@ -167,6 +168,20 @@
if (model.IsPaused()) {
++info.paused_count;
}
+ } else {
+ base::Time cur_completed_time = model.GetEndTime();
+ if (cur_completed_time.is_null() && update_is_for_model &&
+ model.GetState() != download::DownloadItem::CANCELLED) {
+ // Given that we consider dangerous/insecure downloads to be complete, the
+ // completion time should reflect the time they were marked as
+ // dangerous/insecure. Since download is still technically IN_PROGRESS in
+ // this scenario and thus has a null end time, we just use the current
+ // time based on the assumption that a download in a dangerous/insecure
+ // state does not receive further updates besides cancellation.
+ cur_completed_time = base::Time::Now();
+ }
+ info.last_completed_time =
+ std::max(info.last_completed_time, cur_completed_time);
}
}
@@ -509,7 +524,40 @@
return DownloadBubbleDisplayInfo::EmptyInfo();
}
-void DownloadBubbleUpdateService::CacheManager::UpdateDisplayInfo() {
+void DownloadBubbleUpdateService::CacheManager::UpdateDisplayInfo(
+ const std::string& updating_for_item) {
+#if DCHECK_IS_ON()
+ ConsistencyCheckCaches();
+#endif // DCHECK_IS_ON()
+
+ // A new info is constructed from scratch based on the current cache contents.
+ DownloadBubbleDisplayInfo info;
+ base::Time cutoff_time = GetCutoffTime();
+
+ // Iterate over the two sorted caches (download items and offline items) in
+ // combined/merged sorted order. This is done in the same way as in
+ // GetAllItemsToDisplay() to ensure that the info most accurately represents
+ // the list of items that would be returned from that method.
+ IterateOverMergedCaches(
+ base::BindRepeating(
+ &DownloadBubbleUpdateService::CacheManager::
+ UpdateDisplayInfoForDownloadItem,
+ base::Unretained(this),
+ base::optional_ref<const std::string>(updating_for_item), cutoff_time,
+ std::ref(info)),
+ base::BindRepeating(&DownloadBubbleUpdateService::CacheManager::
+ UpdateDisplayInfoForOfflineItem,
+ base::Unretained(this), absl::nullopt, cutoff_time,
+ std::ref(info)),
+ base::BindRepeating(&DownloadBubbleUpdateService::CacheManager::
+ ShouldStopUpdatingDisplayInfo,
+ base::Unretained(this), std::ref(info)));
+
+ display_info_ = info;
+}
+
+void DownloadBubbleUpdateService::CacheManager::UpdateDisplayInfo(
+ const ContentId& updating_for_item) {
#if DCHECK_IS_ON()
ConsistencyCheckCaches();
#endif // DCHECK_IS_ON()
@@ -525,10 +573,14 @@
IterateOverMergedCaches(
base::BindRepeating(&DownloadBubbleUpdateService::CacheManager::
UpdateDisplayInfoForDownloadItem,
- base::Unretained(this), cutoff_time, std::ref(info)),
- base::BindRepeating(&DownloadBubbleUpdateService::CacheManager::
- UpdateDisplayInfoForOfflineItem,
- base::Unretained(this), cutoff_time, std::ref(info)),
+ base::Unretained(this), absl::nullopt, cutoff_time,
+ std::ref(info)),
+ base::BindRepeating(
+ &DownloadBubbleUpdateService::CacheManager::
+ UpdateDisplayInfoForOfflineItem,
+ base::Unretained(this),
+ base::optional_ref<const ContentId>(updating_for_item), cutoff_time,
+ std::ref(info)),
base::BindRepeating(&DownloadBubbleUpdateService::CacheManager::
ShouldStopUpdatingDisplayInfo,
base::Unretained(this), std::ref(info)));
@@ -538,24 +590,32 @@
void DownloadBubbleUpdateService::CacheManager::
UpdateDisplayInfoForDownloadItem(
+ base::optional_ref<const std::string> updating_for_item,
base::Time cutoff_time,
DownloadBubbleDisplayInfo& info,
SortedDownloadItems::iterator& download_item_it) {
DownloadItemModel model(
download_item_it->second,
std::make_unique<DownloadUIModel::BubbleStatusTextBuilder>());
- UpdateInfoForModel(model, cutoff_time, info);
+ bool update_is_for_model =
+ updating_for_item.has_value() &&
+ *updating_for_item == GetItemId(download_item_it->second);
+ UpdateInfoForModel(model, update_is_for_model, cutoff_time, info);
++download_item_it;
}
void DownloadBubbleUpdateService::CacheManager::UpdateDisplayInfoForOfflineItem(
+ base::optional_ref<const ContentId> updating_for_item,
base::Time cutoff_time,
DownloadBubbleDisplayInfo& info,
SortedOfflineItems::iterator& offline_item_it) {
OfflineItemModel model(
update_service_->GetOfflineManager(), offline_item_it->second,
std::make_unique<DownloadUIModel::BubbleStatusTextBuilder>());
- UpdateInfoForModel(model, cutoff_time, info);
+ bool update_is_for_model =
+ updating_for_item.has_value() &&
+ *updating_for_item == GetItemId(offline_item_it->second);
+ UpdateInfoForModel(model, update_is_for_model, cutoff_time, info);
++offline_item_it;
}
@@ -990,7 +1050,7 @@
cache.erase(to_remove);
}
- UpdateDisplayInfo();
+ UpdateDisplayInfo(id);
CHECK(!cache.empty());
auto last_it = GetLastIter(cache);
@@ -1022,7 +1082,7 @@
cache.erase(iter_map_it->second);
iter_map.erase(iter_map_it);
- UpdateDisplayInfo();
+ UpdateDisplayInfo(id);
CHECK(cache.size() < GetNumItemsToCache());
return true;
@@ -1036,10 +1096,11 @@
IterMap<Id, Item>& iter_map) {
CHECK(iter != cache.end());
auto next_iter = std::next(iter);
- iter_map.erase(GetItemId(iter->second));
+ Id id = GetItemId(iter->second);
+ iter_map.erase(id);
cache.erase(iter);
- UpdateDisplayInfo();
+ UpdateDisplayInfo(id);
return next_iter;
}
@@ -1232,7 +1293,7 @@
return;
}
- GetCacheForItem(item).UpdateDisplayInfo();
+ GetCacheForItem(item).UpdateDisplayInfo(guid);
auto* web_app_data = DownloadItemWebAppData::Get(item);
for (Browser* browser : chrome::FindAllBrowsersWithProfile(profile_)) {
diff --git a/chrome/browser/download/bubble/download_bubble_update_service.h b/chrome/browser/download/bubble/download_bubble_update_service.h
index ca49839..44ed8f1b 100644
--- a/chrome/browser/download/bubble/download_bubble_update_service.h
+++ b/chrome/browser/download/bubble/download_bubble_update_service.h
@@ -12,6 +12,7 @@
#include "base/memory/weak_ptr.h"
#include "base/scoped_observation.h"
#include "base/time/time.h"
+#include "base/types/optional_ref.h"
#include "chrome/browser/download/bubble/download_bubble_display_info.h"
#include "chrome/browser/download/bubble/download_display_controller.h"
#include "chrome/browser/download/download_ui_model.h"
@@ -294,13 +295,20 @@
// Updates |display_info_| based on the current contents of the cache.
// This is kept updated as items are added or removed from the cache.
- void UpdateDisplayInfo();
+ // `updating_for_item` is the id of the item (download or offline item)
+ // whose update triggered calling this function.
+ void UpdateDisplayInfo(const std::string& updating_for_item);
+ void UpdateDisplayInfo(
+ const offline_items_collection::ContentId& updating_for_item);
// Implements the above.
void UpdateDisplayInfoForDownloadItem(
+ base::optional_ref<const std::string> updating_for_item,
base::Time cutoff_time,
DownloadBubbleDisplayInfo& info,
SortedDownloadItems::iterator& download_item_it);
void UpdateDisplayInfoForOfflineItem(
+ base::optional_ref<const offline_items_collection::ContentId>
+ updating_for_item,
base::Time cutoff_time,
DownloadBubbleDisplayInfo& info,
SortedOfflineItems::iterator& offline_item_it);
diff --git a/chrome/browser/download/bubble/download_bubble_update_service_unittest.cc b/chrome/browser/download/bubble/download_bubble_update_service_unittest.cc
index 40fb351..4ac3bc7 100644
--- a/chrome/browser/download/bubble/download_bubble_update_service_unittest.cc
+++ b/chrome/browser/download/bubble/download_bubble_update_service_unittest.cc
@@ -192,7 +192,12 @@
EXPECT_CALL(item, GetGuid()).WillRepeatedly(ReturnRefOfCopy(guid));
EXPECT_CALL(item, GetState()).WillRepeatedly(Return(state));
EXPECT_CALL(item, GetStartTime()).WillRepeatedly(Return(start_time));
- EXPECT_CALL(item, GetEndTime()).WillRepeatedly(Return(start_time));
+ if (state == DownloadState::COMPLETE) {
+ EXPECT_CALL(item, GetEndTime())
+ .WillRepeatedly(Return(start_time + base::Minutes(1)));
+ } else {
+ EXPECT_CALL(item, GetEndTime()).WillRepeatedly(Return(base::Time()));
+ }
EXPECT_CALL(item, GetTargetFilePath())
.WillRepeatedly(
ReturnRefOfCopy(base::FilePath(FILE_PATH_LITERAL("foo"))));
@@ -791,7 +796,57 @@
EXPECT_EQ(app_b_progress_info.progress_percentage, 50);
}
-TEST_F(DownloadBubbleUpdateServiceTest, GetAllUIModelsInfo) {
+TEST_F(DownloadBubbleUpdateServiceTest, GetDisplayInfo_InProgress) {
+ base::Time now = base::Time::Now();
+ base::Time two_hours_ago = now - base::Hours(2);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "now_download",
+ /*is_paused=*/false, now);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "two_hours_ago_download",
+ /*is_paused=*/false, two_hours_ago);
+ InitOfflineItems({OfflineItemState::PAUSED, OfflineItemState::PAUSED},
+ {"now_offline_item", "two_hours_ago_offline_item"},
+ {now, two_hours_ago});
+
+ DownloadBubbleDisplayInfo info =
+ update_service_->GetDisplayInfo(/*web_app_id=*/nullptr);
+ EXPECT_EQ(info.all_models_size, 4u);
+ // The last_completed_time is null, because no downloads have finished.
+ EXPECT_EQ(info.last_completed_time, base::Time());
+ EXPECT_EQ(info.in_progress_count, 4);
+ EXPECT_EQ(info.paused_count, 2);
+ EXPECT_TRUE(info.has_unactioned);
+ EXPECT_FALSE(info.has_deep_scanning);
+}
+
+TEST_F(DownloadBubbleUpdateServiceTest,
+ GetDisplayInfo_UpdateForCompleteDownload) {
+ base::Time now = base::Time::Now();
+ base::Time two_hours_ago = now - base::Hours(2);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "now_download",
+ /*is_paused=*/false, now);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "two_hours_ago_download",
+ /*is_paused=*/false, two_hours_ago);
+ InitDownloadItem(DownloadState::COMPLETE, "completed_download",
+ /*is_paused=*/false, two_hours_ago);
+ InitOfflineItems({OfflineItemState::PAUSED, OfflineItemState::PAUSED},
+ {"now_offline_item", "two_hours_ago_offline_item"},
+ {now, two_hours_ago});
+
+ // Make the download completed to update the last_completed_time.
+ UpdateDownloadItem(0, DownloadState::COMPLETE);
+
+ DownloadBubbleDisplayInfo info =
+ update_service_->GetDisplayInfo(/*web_app_id=*/nullptr);
+ EXPECT_EQ(info.all_models_size, 5u);
+ EXPECT_EQ(info.last_completed_time, now);
+ EXPECT_EQ(info.in_progress_count, 3);
+ EXPECT_EQ(info.paused_count, 2);
+ EXPECT_TRUE(info.has_unactioned);
+ EXPECT_FALSE(info.has_deep_scanning);
+}
+
+TEST_F(DownloadBubbleUpdateServiceTest,
+ GetDisplayInfo_ExistingCompleteDownload) {
base::Time now = base::Time::Now();
base::Time two_hours_ago = now - base::Hours(2);
InitDownloadItem(DownloadState::IN_PROGRESS, "now_download",
@@ -807,13 +862,43 @@
DownloadBubbleDisplayInfo info =
update_service_->GetDisplayInfo(/*web_app_id=*/nullptr);
EXPECT_EQ(info.all_models_size, 5u);
- EXPECT_EQ(info.last_completed_time, now);
+ // The last_completed_time should be set to the end time of the completed
+ // download, which is 1 minute past its start time (set in InitDownloadItem).
+ EXPECT_EQ(info.last_completed_time, two_hours_ago + base::Minutes(1));
EXPECT_EQ(info.in_progress_count, 4);
EXPECT_EQ(info.paused_count, 2);
EXPECT_TRUE(info.has_unactioned);
EXPECT_FALSE(info.has_deep_scanning);
}
+TEST_F(DownloadBubbleUpdateServiceTest, GetDisplayInfo_UpdateForDangerous) {
+ base::Time now = base::Time::Now();
+ base::Time two_hours_ago = now - base::Hours(2);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "now_download",
+ /*is_paused=*/false, now);
+ InitDownloadItem(DownloadState::IN_PROGRESS, "two_hours_ago_download",
+ /*is_paused=*/false, two_hours_ago);
+ InitDownloadItem(DownloadState::COMPLETE, "completed_download",
+ /*is_paused=*/false, two_hours_ago);
+ InitOfflineItems({OfflineItemState::PAUSED, OfflineItemState::PAUSED},
+ {"now_offline_item", "two_hours_ago_offline_item"},
+ {now, two_hours_ago});
+
+ // Make the download dangerous to update the last_completed_time.
+ UpdateDownloadItem(
+ 0, DownloadState::IN_PROGRESS, /*is_paused=*/false,
+ DownloadDangerType::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT);
+
+ DownloadBubbleDisplayInfo info =
+ update_service_->GetDisplayInfo(/*web_app_id=*/nullptr);
+ EXPECT_EQ(info.all_models_size, 5u);
+ EXPECT_EQ(info.last_completed_time, now);
+ EXPECT_EQ(info.in_progress_count, 3);
+ EXPECT_EQ(info.paused_count, 2);
+ EXPECT_TRUE(info.has_unactioned);
+ EXPECT_FALSE(info.has_deep_scanning);
+}
+
TEST_F(DownloadBubbleUpdateServiceTest, GetAllUIModelsInfoForWebApp) {
base::Time now = base::Time::Now();
base::Time two_hours_ago = now - base::Hours(2);
diff --git a/chrome/browser/download/bubble/download_display_controller.cc b/chrome/browser/download/bubble/download_display_controller.cc
index 8fb7cf0..71062bc 100644
--- a/chrome/browser/download/bubble/download_display_controller.cc
+++ b/chrome/browser/download/bubble/download_display_controller.cc
@@ -56,6 +56,17 @@
return info.in_progress_count == info.paused_count;
}
+// Whether the last download complete time is more recent than `interval` ago.
+bool HasRecentCompleteDownload(base::TimeDelta interval,
+ base::Time last_complete_time) {
+ if (last_complete_time.is_null()) {
+ return false;
+ }
+ base::TimeDelta time_since_last_completion =
+ base::Time::Now() - last_complete_time;
+ return time_since_last_completion < interval;
+}
+
} // namespace
DownloadDisplayController::DownloadDisplayController(
@@ -174,6 +185,7 @@
}
void DownloadDisplayController::HideToolbarButton() {
+ // TODO(chlily): This should only hide the bubble/button when it is not open.
if (display_->IsShowing()) {
display_->Hide();
}
@@ -234,7 +246,6 @@
HideToolbarButton();
return;
}
- base::Time last_complete_time = GetLastCompleteTime(info.last_completed_time);
DownloadDisplay::IconUpdateInfo updates;
@@ -247,7 +258,7 @@
updates.new_state = DownloadIconState::kComplete;
bool complete_unactioned =
HasRecentCompleteDownload(kToolbarIconActiveTimeInterval,
- last_complete_time) &&
+ info.last_completed_time) &&
info.has_unactioned;
bool exited_fullscreen_owed_details =
!display_->IsFullscreenWithParentViewHidden() &&
@@ -266,7 +277,7 @@
if (updates.new_state != DownloadIconState::kComplete ||
HasRecentCompleteDownload(kToolbarIconVisibilityTimeInterval,
- last_complete_time)) {
+ info.last_completed_time)) {
ShowToolbarButton();
}
@@ -310,13 +321,6 @@
&DownloadDisplayController::UpdateDownloadIconToInactive);
}
-base::Time DownloadDisplayController::GetLastCompleteTime(
- base::Time last_completed_time_from_current_models) const {
- base::Time last_time = DownloadPrefs::FromBrowserContext(browser_->profile())
- ->GetLastCompleteTime();
- return std::max(last_time, last_completed_time_from_current_models);
-}
-
void DownloadDisplayController::MaybeShowButtonWhenCreated() {
if (!download::ShouldShowDownloadBubble(browser_->profile())) {
return;
@@ -324,20 +328,12 @@
const DownloadBubbleDisplayInfo& info = UpdateButtonStateFromUpdateService();
if (display_->IsShowing()) {
+ base::Time disappearance_time =
+ info.last_completed_time + kToolbarIconVisibilityTimeInterval;
+ base::TimeDelta interval_until_disappearance =
+ disappearance_time - base::Time::Now();
+ // Avoid passing a negative time interval.
ScheduleToolbarDisappearance(
- kToolbarIconVisibilityTimeInterval -
- (base::Time::Now() - GetLastCompleteTime(info.last_completed_time)));
+ std::max(base::TimeDelta(), interval_until_disappearance));
}
}
-
-bool DownloadDisplayController::HasRecentCompleteDownload(
- base::TimeDelta interval,
- base::Time last_complete_time) {
- base::Time current_time = base::Time::Now();
- base::TimeDelta time_since_last_completion =
- current_time - last_complete_time;
- // Also check that the current time is not smaller than the last complete
- // time, this can happen if the system clock has moved backward.
- return time_since_last_completion < interval &&
- current_time >= last_complete_time;
-}
diff --git a/chrome/browser/download/bubble/download_display_controller.h b/chrome/browser/download/bubble/download_display_controller.h
index da26e048..4e71c6c 100644
--- a/chrome/browser/download/bubble/download_display_controller.h
+++ b/chrome/browser/download/bubble/download_display_controller.h
@@ -128,12 +128,6 @@
// Decides whether the toolbar button should be shown when it is created.
virtual void MaybeShowButtonWhenCreated();
- // Whether the last download complete time is less than `interval` ago.
- bool HasRecentCompleteDownload(base::TimeDelta interval,
- base::Time last_complete_time);
-
- base::Time GetLastCompleteTime(
- base::Time last_completed_time_from_current_models) const;
// The pointer is created in ToolbarView and owned by ToolbarView.
raw_ptr<DownloadDisplay> const display_;
diff --git a/chrome/browser/download/bubble/download_display_controller_unittest.cc b/chrome/browser/download/bubble/download_display_controller_unittest.cc
index 963e340..526cd31 100644
--- a/chrome/browser/download/bubble/download_display_controller_unittest.cc
+++ b/chrome/browser/download/bubble/download_display_controller_unittest.cc
@@ -416,8 +416,8 @@
.WillRepeatedly(Return(is_insecure));
if (state == DownloadState::COMPLETE) {
EXPECT_CALL(item(item_index), IsDone()).WillRepeatedly(Return(true));
- DownloadPrefs::FromBrowserContext(profile())->SetLastCompleteTime(
- base::Time::Now());
+ EXPECT_CALL(item(item_index), GetEndTime())
+ .WillRepeatedly(Return(base::Time::Now()));
} else {
EXPECT_CALL(item(item_index), IsDone()).WillRepeatedly(Return(false));
}
@@ -876,44 +876,6 @@
/*is_active=*/true));
}
-TEST_F(DownloadDisplayControllerTest, InitialState_OldLastDownload) {
- InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
- download::DownloadItem::COMPLETE);
- base::Time current_time = base::Time::Now();
- // Set the last complete time to more than 1 hour ago.
- DownloadPrefs::FromBrowserContext(profile())->SetLastCompleteTime(
- current_time - base::Minutes(61));
-
- DownloadDisplayController controller(&display(), browser(),
- &bubble_controller());
- EXPECT_TRUE(VerifyDisplayState(/*shown=*/false, /*detail_shown=*/false,
- /*icon_state=*/DownloadIconState::kComplete,
- /*is_active=*/false));
-}
-
-TEST_F(DownloadDisplayControllerTest, InitialState_NewLastDownload) {
- InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
- download::DownloadItem::COMPLETE);
- base::Time current_time = base::Time::Now();
- // Set the last complete time to less than 1 hour ago.
- DownloadPrefs::FromBrowserContext(profile())->SetLastCompleteTime(
- current_time - base::Minutes(59));
-
- DownloadDisplayController controller(&display(), browser(),
- &bubble_controller());
- // The initial state should not display details.
- EXPECT_TRUE(VerifyDisplayState(/*shown=*/true, /*detail_shown=*/false,
- /*icon_state=*/DownloadIconState::kComplete,
- /*is_active=*/false));
-
- // The display should stop showing once the last download is more than 1 day
- // ago.
- task_environment_.FastForwardBy(base::Minutes(1));
- EXPECT_TRUE(VerifyDisplayState(/*shown=*/false, /*detail_shown=*/false,
- /*icon_state=*/DownloadIconState::kComplete,
- /*is_active=*/false));
-}
-
TEST_F(DownloadDisplayControllerTest, InitialState_InProgressDownload) {
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
download::DownloadItem::IN_PROGRESS);
@@ -927,23 +889,6 @@
/*is_active=*/true));
}
-TEST_F(DownloadDisplayControllerTest,
- InitialState_NewLastDownloadWithEmptyItem) {
- base::Time current_time = base::Time::Now();
- // Set the last complete time to less than 1 hour ago.
- DownloadPrefs::FromBrowserContext(profile())->SetLastCompleteTime(
- current_time - base::Minutes(59));
-
- DownloadDisplayController controller(&display(), browser(),
- &bubble_controller());
- // Although the last complete time is set, the download display is not shown
- // because the download item list is empty. This can happen if the download
- // history is deleted by the user.
- EXPECT_TRUE(VerifyDisplayState(/*shown=*/false, /*detail_shown=*/false,
- /*icon_state=*/DownloadIconState::kComplete,
- /*is_active=*/false));
-}
-
TEST_F(DownloadDisplayControllerTest, InitialState_NoLastDownload) {
DownloadDisplayController controller(&display(), browser(),
&bubble_controller());
diff --git a/chrome/browser/download/download_prefs.cc b/chrome/browser/download/download_prefs.cc
index c4c7b837..fdbc3e4 100644
--- a/chrome/browser/download/download_prefs.cc
+++ b/chrome/browser/download/download_prefs.cc
@@ -309,8 +309,6 @@
default_download_path);
registry->RegisterFilePathPref(prefs::kSaveFileDefaultDirectory,
default_download_path);
- registry->RegisterTimePref(prefs::kDownloadLastCompleteTime,
- /*default_value=*/base::Time());
#if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS) || \
BUILDFLAG(IS_MAC)
registry->RegisterBooleanPref(prefs::kOpenPdfDownloadInSystemReader, false);
@@ -388,15 +386,6 @@
save_file_type_.SetValue(type);
}
-base::Time DownloadPrefs::GetLastCompleteTime() {
- return profile_->GetPrefs()->GetTime(prefs::kDownloadLastCompleteTime);
-}
-
-void DownloadPrefs::SetLastCompleteTime(const base::Time& last_complete_time) {
- profile_->GetPrefs()->SetTime(prefs::kDownloadLastCompleteTime,
- last_complete_time);
-}
-
bool DownloadPrefs::PromptForDownload() const {
// If the DownloadDirectory policy is set, then |prompt_for_download_| should
// always be false.
diff --git a/chrome/browser/download/download_prefs.h b/chrome/browser/download/download_prefs.h
index 4bb6f3198..27311fcc 100644
--- a/chrome/browser/download/download_prefs.h
+++ b/chrome/browser/download/download_prefs.h
@@ -10,7 +10,6 @@
#include "base/files/file_path.h"
#include "base/memory/raw_ptr.h"
-#include "base/time/time.h"
#include "build/build_config.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_member.h"
@@ -79,8 +78,6 @@
void SetSaveFilePath(const base::FilePath& path);
int save_file_type() const { return *save_file_type_; }
void SetSaveFileType(int type);
- base::Time GetLastCompleteTime();
- void SetLastCompleteTime(const base::Time& last_complete_time);
DownloadRestriction download_restriction() const {
return static_cast<DownloadRestriction>(*download_restriction_);
}
diff --git a/chrome/browser/download/download_status_updater.cc b/chrome/browser/download/download_status_updater.cc
index 8e3ee69..202f062f 100644
--- a/chrome/browser/download/download_status_updater.cc
+++ b/chrome/browser/download/download_status_updater.cc
@@ -115,7 +115,6 @@
void DownloadStatusUpdater::OnDownloadUpdated(content::DownloadManager* manager,
download::DownloadItem* item) {
- UpdatePrefsOnDownloadUpdated(manager, item);
if (item->GetState() == download::DownloadItem::IN_PROGRESS &&
!item->IsTransient()) {
// If the item was interrupted/cancelled and then resumed/restarted, then
@@ -186,18 +185,3 @@
// TODO(avi): Implement for Android?
}
#endif
-
-void DownloadStatusUpdater::UpdatePrefsOnDownloadUpdated(
- content::DownloadManager* manager,
- download::DownloadItem* download) {
- if (!manager) {
- // Can be null in tests.
- return;
- }
-
- if (download->GetState() == download::DownloadItem::COMPLETE &&
- !download->IsTransient()) {
- DownloadPrefs::FromDownloadManager(manager)->SetLastCompleteTime(
- base::Time::Now());
- }
-}
diff --git a/chrome/browser/download/download_status_updater.h b/chrome/browser/download/download_status_updater.h
index 60c2612..c9e2825 100644
--- a/chrome/browser/download/download_status_updater.h
+++ b/chrome/browser/download/download_status_updater.h
@@ -62,10 +62,6 @@
// in-progress downloads, and the browser is not tearing down yet.
void UpdateProfileKeepAlive(content::DownloadManager* manager);
- // Updates the download prefs when downloads are updated.
- void UpdatePrefsOnDownloadUpdated(content::DownloadManager* manager,
- download::DownloadItem* download);
-
private:
std::vector<std::unique_ptr<download::AllDownloadItemNotifier>> notifiers_;
std::map<Profile*, std::unique_ptr<ScopedProfileKeepAlive>>
diff --git a/chrome/browser/download/download_status_updater_unittest.cc b/chrome/browser/download/download_status_updater_unittest.cc
index db86f1a..a404b828 100644
--- a/chrome/browser/download/download_status_updater_unittest.cc
+++ b/chrome/browser/download/download_status_updater_unittest.cc
@@ -407,55 +407,6 @@
profile1, ProfileKeepAliveOrigin::kDownloadInProgress));
}
-// Test that the last completion time is logged in pref.
-TEST_F(DownloadStatusUpdaterTest, LogLastCompletionTimeInPrefs) {
- SetupManagers(/*manager_count=*/1);
- AddItems(/*manager_index=*/0, /*item_count=*/3, /*in_progress_count=*/0);
- LinkManager(0);
-
- DownloadPrefs* download_prefs =
- DownloadPrefs::FromDownloadManager(Manager(0));
- base::Time initial_time = download_prefs->GetLastCompleteTime();
- base::Time current_time = base::Time::Now();
-
- // Set the first download to in progress and notify the update.
- SetItemValues(/*manager_index=*/0, /*item_index=*/0, /*received_bytes=*/90,
- /*total_bytes=*/100, /*notify=*/true);
- // The last complete time is still the initial time, because the download is
- // not complete yet.
- EXPECT_EQ(initial_time, download_prefs->GetLastCompleteTime());
-
- // The first download has completed.
- CompleteItem(/*manager_index=*/0, /*item_index=*/0);
- // The last complete time is updated.
- EXPECT_EQ(current_time, download_prefs->GetLastCompleteTime());
-
- task_environment_.FastForwardBy(base::Hours(1));
- // Set the second download item to in progress and notify the update.
- SetItemValues(/*manager_index=*/0, /*item_index=*/1, /*received_bytes=*/90,
- /*total_bytes=*/100, /*notify=*/true);
- // The last complete time is not updated yet, because the second download is
- // still in progress.
- EXPECT_EQ(current_time, download_prefs->GetLastCompleteTime());
-
- task_environment_.FastForwardBy(base::Hours(1));
- // The second download has completed.
- CompleteItem(/*manager_index=*/0, /*item_index=*/1);
- // Completed time is updated
- EXPECT_EQ(current_time + base::Hours(2),
- download_prefs->GetLastCompleteTime());
-
- task_environment_.FastForwardBy(base::Hours(1));
- SetItemValues(/*manager_index=*/0, /*item_index=*/2, /*received_bytes=*/90,
- /*total_bytes=*/100, /*notify=*/true);
- EXPECT_CALL(*Item(/*manager_index=*/0, /*item_index=*/2), IsTransient())
- .WillRepeatedly(Return(true));
- CompleteItem(/*manager_index=*/0, /*item_index=*/2);
- // Completed time is not updated, because this download is transient.
- EXPECT_EQ(current_time + base::Hours(2),
- download_prefs->GetLastCompleteTime());
-}
-
// Tests that transient download will not trigger any updates.
TEST_F(DownloadStatusUpdaterTest, TransientDownload) {
SetupManagers(/*manager_count=*/1);
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index daf90c5..aefc80b 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -903,39 +903,36 @@
const char kPrivacySandboxM1Unrestricted[] = "privacy_sandbox.m1.unrestricted";
#if BUILDFLAG(IS_WIN)
const char kSwReporter[] = "software_reporter";
-inline constexpr char kChromeCleaner[] = "chrome_cleaner";
-inline constexpr char kSettingsResetPrompt[] = "settings_reset_prompt";
+const char kChromeCleaner[] = "chrome_cleaner";
+const char kSettingsResetPrompt[] = "settings_reset_prompt";
#endif
// A boolean specifying whether the new download bubble UI is enabled. If it is
// set to false, the old download shelf UI will be shown instead.
-inline constexpr char kDownloadBubbleEnabled[] = "download_bubble_enabled";
+const char kDownloadBubbleEnabled[] = "download_bubble_enabled";
// Deprecated 09/2023.
#if BUILDFLAG(IS_CHROMEOS_ASH)
-constexpr char kGestureEducationNotificationShown[] =
+const char kGestureEducationNotificationShown[] =
"ash.gesture_education.notification_shown";
// Note that this very name is used outside ChromeOS Ash, where it isn't
// deprecated.
-constexpr char kSyncInitialSyncFeatureSetupCompleteOnAsh[] =
+const char kSyncInitialSyncFeatureSetupCompleteOnAsh[] =
"sync.has_setup_completed";
#endif
// Deprecated 09/2023.
-// Synced boolean that indicates if a user has manually toggled the settings
-// associated with the PrivacySandboxSettings feature.
-inline constexpr char kPrivacySandboxManuallyControlled[] =
+const char kPrivacySandboxManuallyControlled[] =
"privacy_sandbox.manually_controlled";
// Deprecated 09/2023.
-// Boolean value indicating whether the regular prefs were migrated to UPM
-// settings for syncing users.
#if BUILDFLAG(IS_ANDROID)
const char kSettingsMigratedToUPM[] = "profile.settings_migrated_to_upm";
#endif
// Deprecated 10/2023.
-inline constexpr char kSyncRequested[] = "sync.requested";
+const char kSyncRequested[] = "sync.requested";
+const char kDownloadLastCompleteTime[] = "download.last_complete_time";
// Deprecated 10/2023.
#if BUILDFLAG(IS_CHROMEOS_ASH)
@@ -1321,6 +1318,7 @@
registry->RegisterStringPref(kLastSuccessfulDomainPref, std::string());
registry->RegisterBooleanPref(kShouldAttemptReenable, true);
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
+ registry->RegisterTimePref(kDownloadLastCompleteTime, base::Time());
}
void ClearSyncRequestedPrefAndMaybeMigrate(PrefService* profile_prefs) {
diff --git a/chrome/browser/ui/download/download_display.h b/chrome/browser/ui/download/download_display.h
index e04ba60..6f3c6ca 100644
--- a/chrome/browser/ui/download/download_display.h
+++ b/chrome/browser/ui/download/download_display.h
@@ -55,7 +55,7 @@
// Shows the download display.
virtual void Show() = 0;
- // Hides the download display.
+ // Hides the download display immediately.
virtual void Hide() = 0;
// Returns whether or not the download display is visible.
