Skip to content

Commit

Permalink
[M112][DownloadBubble] Open partial bubble on completion of all downl…
Browse files Browse the repository at this point in the history 
…oads

This changes the partial view to open only when all downloads whose
state was DownloadItem::IN_PROGRESS* have completed**, and not when
initiating a download. The animated icon is still shown on initiating
a download.

To explain caveats:
*  We exclude any IN_PROGRESS downloads that are dangerous, except for
   those which are dangerous and pending deep scanning. Downloads
   pending deep scanning are considered in-progress because we want the
   user to interact with them by initiating deep scanning.
** Completed means reached a quiescent state, either paused or pending
   deep scanning, or excluded for being dangerous, or actually
   completed (i.e. no longer IN_PROGRESS).

In fullscreen mode, if the partial view would have been shown normally,
instead it is shown when the user exits fullscreen mode.

This also fixes a bug whereby the animation was shown in all browser
windows, not just the currently active window.

(cherry picked from commit 4937aa6)

Bug: 1416926
Change-Id: I4a7939180658ebebe96b25cf6026574995ca1974
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4284377
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1109316}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4295919
Auto-Submit: Lily Chen <chlily@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/branch-heads/5615@{#34}
Cr-Branched-From: 9c6408e-refs/heads/main@{#1109224}
  • Loading branch information
@chlily1
chlily1 authored and Chromium LUCI CQ committed Feb 27, 2023
1 parent c326a04 commit e3bbe35
Show file tree
Hide file tree
Showing 8 changed files with 303 additions and 199 deletions.
38 changes: 20 additions & 18 deletions chrome/browser/download/bubble/download_bubble_controller.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "chrome/browser/safe_browsing/safe_browsing_service.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
#include "components/download/public/common/download_danger_type.h"
#include "components/download/public/common/download_item.h"
#include "components/download/public/common/download_stats.h"
#include "components/offline_items_collection/core/offline_content_aggregator.h"
Expand All @@ -52,6 +53,12 @@ bool DownloadUIModelIsRecent(const DownloadUIModel* model,
model->GetStartTime() > cutoff_time);
}

bool IsPendingDeepScanning(const DownloadUIModel& model) {
return model.GetState() == download::DownloadItem::IN_PROGRESS &&
model.GetDangerType() ==
download::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING;
}

using DownloadUIModelPtrList = std::list<DownloadUIModelPtr>;

// Sorting order is 1) Active in-progress downloads, 2) Paused in-progress
Expand Down Expand Up @@ -201,31 +208,22 @@ void DownloadBubbleUIController::OnContentProviderGoingDown() {
void DownloadBubbleUIController::OnItemsAdded(
const OfflineContentProvider::OfflineItemList& items) {
bool any_new = false;
bool any_in_progress = false;
for (const OfflineItem& item : items) {
if (MaybeAddOfflineItem(item, /*is_new=*/true)) {
if (item.state == OfflineItemState::IN_PROGRESS) {
any_in_progress = true;
}
any_new = true;
}
}
if (any_new) {
display_controller_->OnNewItem(
/*show_details=*/(
any_in_progress &&
(browser_ == chrome::FindLastActiveWithProfile(profile_.get()))),
/*show_animation=*/false);
display_controller_->OnNewItem(/*show_animation=*/false);
}
}

void DownloadBubbleUIController::OnNewItem(download::DownloadItem* item,
bool show_details) {
bool may_show_animation) {
auto model = std::make_unique<DownloadItemModel>(item);
model->SetActionedOn(false);
display_controller_->OnNewItem(
(item->GetState() == download::DownloadItem::IN_PROGRESS) && show_details,
model->ShouldShowDownloadStartedAnimation());
display_controller_->OnNewItem(may_show_animation &&
model->ShouldShowDownloadStartedAnimation());
}

bool DownloadBubbleUIController::ShouldShowIncognitoIcon(
Expand Down Expand Up @@ -266,8 +264,9 @@ void DownloadBubbleUIController::OnItemUpdated(
}),
offline_items_.end());
bool was_added = MaybeAddOfflineItem(item, /*is_new=*/false);
OfflineItemModel model(offline_manager_, item);
display_controller_->OnUpdatedItem(
std::make_unique<OfflineItemModel>(offline_manager_, item)->IsDone(),
model.IsDone(), IsPendingDeepScanning(model),
was_added &&
(browser_ == chrome::FindLastActiveWithProfile(profile_.get())));
}
Expand All @@ -277,15 +276,18 @@ void DownloadBubbleUIController::OnDownloadUpdated(
download::DownloadItem* item) {
// manager can be different from download_notifier_ when the current profile
// is off the record.
DownloadItemModel model(item);
if (manager != download_notifier_.GetManager()) {
display_controller_->OnUpdatedItem(item->IsDone(),
/*show_details_if_done=*/false);
IsPendingDeepScanning(model),
/*may_show_details=*/false);
return;
}
bool show_details_if_done =
std::make_unique<DownloadItemModel>(item)->ShouldShowInBubble() &&
bool may_show_details =
model.ShouldShowInBubble() &&
(browser_ == chrome::FindLastActiveWithProfile(profile_.get()));
display_controller_->OnUpdatedItem(item->IsDone(), show_details_if_done);
display_controller_->OnUpdatedItem(
item->IsDone(), IsPendingDeepScanning(model), may_show_details);
}

void DownloadBubbleUIController::PruneOfflineItems() {
Expand Down
6 changes: 4 additions & 2 deletions chrome/browser/download/bubble/download_bubble_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ class DownloadBubbleUIController
bool is_main_view);

// Notify when a new download is ready to be shown on UI, and if the window
// this controller belongs to should show the partial view.
void OnNewItem(download::DownloadItem* item, bool show_details);
// this controller belongs to may show the animation. (Whether the animation
// is actually shown may depend on the download and the device's graphics
// capabilities.)
void OnNewItem(download::DownloadItem* item, bool may_show_animation);

// Notify when a download toolbar button (in any window) is pressed.
void HandleButtonPressed();
Expand Down
77 changes: 50 additions & 27 deletions chrome/browser/download/bubble/download_bubble_controller_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "chrome/test/base/testing_browser_process.h"
#include "chrome/test/base/testing_profile.h"
#include "chrome/test/base/testing_profile_manager.h"
#include "components/download/public/common/download_danger_type.h"
#include "components/download/public/common/mock_download_item.h"
#include "components/offline_items_collection/core/offline_item.h"
#include "components/offline_items_collection/core/test_support/mock_offline_content_provider.h"
Expand All @@ -39,6 +40,7 @@ using testing::SetArgPointee;

namespace {
using StrictMockDownloadItem = testing::StrictMock<download::MockDownloadItem>;
using DownloadDangerType = download::DownloadDangerType;
using DownloadIconState = download::DownloadIconState;
using DownloadState = download::DownloadItem::DownloadState;
const char kProviderNamespace[] = "mock_namespace";
Expand All @@ -49,8 +51,8 @@ class MockDownloadDisplayController : public DownloadDisplayController {
DownloadBubbleUIController* bubble_controller)
: DownloadDisplayController(nullptr, browser, bubble_controller) {}
void MaybeShowButtonWhenCreated() override {}
MOCK_METHOD2(OnNewItem, void(bool, bool));
MOCK_METHOD2(OnUpdatedItem, void(bool, bool));
MOCK_METHOD1(OnNewItem, void(bool));
MOCK_METHOD3(OnUpdatedItem, void(bool, bool, bool));
MOCK_METHOD1(OnRemovedItem, void(const ContentId&));
};

Expand Down Expand Up @@ -158,10 +160,10 @@ class DownloadBubbleUIControllerTest : public testing::Test {

void InitDownloadItem(const base::FilePath::CharType* path,
DownloadState state,
std::string& id,
const std::string& id,
bool is_transient = false,
base::Time start_time = base::Time::Now(),
bool show_details = true) {
bool may_show_animation = true) {
size_t index = items_.size();
items_.push_back(std::make_unique<StrictMockDownloadItem>());
EXPECT_CALL(item(index), GetId())
Expand Down Expand Up @@ -207,12 +209,15 @@ class DownloadBubbleUIControllerTest : public testing::Test {
item(index).AddObserver(&controller().get_download_notifier_for_testing());
content::DownloadItemUtils::AttachInfoForTesting(&(item(index)), profile_,
nullptr);
controller().OnNewItem(&item(index), show_details);
controller().OnNewItem(&item(index), may_show_animation);
}

void UpdateDownloadItem(int item_index,
DownloadState state,
bool is_paused = false) {
void UpdateDownloadItem(
int item_index,
DownloadState state,
bool is_paused = false,
DownloadDangerType danger_type =
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) {
Expand All @@ -222,6 +227,12 @@ class DownloadBubbleUIControllerTest : public testing::Test {
} else {
EXPECT_CALL(item(item_index), IsDone()).WillRepeatedly(Return(false));
}
EXPECT_CALL(item(item_index), IsDangerous())
.WillRepeatedly(
Return(danger_type !=
DownloadDangerType::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS));
EXPECT_CALL(item(item_index), GetDangerType())
.WillRepeatedly(Return(danger_type));
EXPECT_CALL(item(item_index), IsPaused()).WillRepeatedly(Return(is_paused));
item(item_index).NotifyObserversDownloadUpdated();
}
Expand Down Expand Up @@ -264,38 +275,48 @@ class DownloadBubbleUIControllerTest : public testing::Test {

TEST_F(DownloadBubbleUIControllerTest, ProcessesNewItems) {
std::vector<std::string> ids = {"Download 1", "Download 2", "Offline 1",
"Download 3"};
EXPECT_CALL(display_controller(), OnNewItem(true, true)).Times(1);
"Download 3", "Offline 2"};
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(2);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
download::DownloadItem::IN_PROGRESS, ids[0]);
EXPECT_CALL(display_controller(), OnNewItem(false, true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar2.pdf"),
download::DownloadItem::COMPLETE, ids[1]);
EXPECT_CALL(display_controller(), OnNewItem(true, false)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(false)).Times(1);
InitOfflineItem(OfflineItemState::IN_PROGRESS, ids[2]);
EXPECT_CALL(display_controller(), OnNewItem(false, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
download::DownloadItem::IN_PROGRESS, ids[3],
/*is_transient=*/false, base::Time::Now(),
/*show_details=*/false);
/*is_transient=*/false, base::Time::Now());
EXPECT_CALL(display_controller(), OnNewItem(false)).Times(1);
InitOfflineItem(OfflineItemState::IN_PROGRESS, ids[4]);
}

TEST_F(DownloadBubbleUIControllerTest, ProcessesUpdatedItems) {
std::vector<std::string> ids = {"Download 1", "Offline 1"};
EXPECT_CALL(display_controller(), OnNewItem(true, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
download::DownloadItem::IN_PROGRESS, ids[0]);
EXPECT_CALL(display_controller(), OnUpdatedItem(false, true)).Times(1);
EXPECT_CALL(display_controller(), OnUpdatedItem(false, false, true)).Times(1);
UpdateDownloadItem(/*item_index=*/0, DownloadState::IN_PROGRESS);
EXPECT_CALL(display_controller(), OnUpdatedItem(true, true)).Times(1);
EXPECT_CALL(display_controller(), OnUpdatedItem(true, false, true)).Times(1);
UpdateDownloadItem(/*item_index=*/0, DownloadState::COMPLETE);

EXPECT_CALL(display_controller(), OnNewItem(true, false)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(false)).Times(1);
InitOfflineItem(OfflineItemState::IN_PROGRESS, ids[1]);
EXPECT_CALL(display_controller(), OnUpdatedItem(true, true)).Times(1);
EXPECT_CALL(display_controller(), OnUpdatedItem(true, false, true)).Times(1);
UpdateOfflineItem(/*item_index=*/0, OfflineItemState::COMPLETE);
}

TEST_F(DownloadBubbleUIControllerTest, UpdatedItemIsPendingDeepScanning) {
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
download::DownloadItem::IN_PROGRESS, "Download 1");
EXPECT_CALL(display_controller(), OnUpdatedItem(false, true, true)).Times(1);
UpdateDownloadItem(
/*item_index=*/0, DownloadState::IN_PROGRESS, false,
DownloadDangerType::DOWNLOAD_DANGER_TYPE_PROMPT_FOR_SCANNING);
}

TEST_F(DownloadBubbleUIControllerTest, TransientDownloadShouldNotShow) {
std::vector<std::string> ids = {"Download 1", "Download 2"};
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar.pdf"),
Expand Down Expand Up @@ -394,21 +415,21 @@ TEST_F(DownloadBubbleUIControllerTest, NoItemsReturnedForPartialViewTooSoon) {
"Download 4"};

// First time showing the partial view should work.
EXPECT_CALL(display_controller(), OnNewItem(true, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar1.pdf"),
download::DownloadItem::IN_PROGRESS, ids[0]);
EXPECT_EQ(controller().GetPartialView().size(), 1u);

// No items are returned for a partial view because it is too soon.
task_environment_.FastForwardBy(base::Seconds(14));
EXPECT_CALL(display_controller(), OnNewItem(false, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar2.pdf"),
download::DownloadItem::COMPLETE, ids[1]);
EXPECT_EQ(controller().GetPartialView().size(), 0u);

// Partial view can now be shown, and contains all the items.
task_environment_.FastForwardBy(base::Seconds(1));
EXPECT_CALL(display_controller(), OnNewItem(false, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar3.pdf"),
download::DownloadItem::COMPLETE, ids[1]);
EXPECT_EQ(controller().GetPartialView().size(), 3u);
Expand All @@ -420,7 +441,7 @@ TEST_F(DownloadBubbleUIControllerTest, NoItemsReturnedForPartialViewTooSoon) {

// Main view resets the partial view time, so the partial view can now be
// shown.
EXPECT_CALL(display_controller(), OnNewItem(true, true)).Times(1);
EXPECT_CALL(display_controller(), OnNewItem(true)).Times(1);
InitDownloadItem(FILE_PATH_LITERAL("/foo/bar4.pdf"),
download::DownloadItem::IN_PROGRESS, ids[3]);
EXPECT_EQ(controller().GetPartialView().size(), 1u);
Expand Down Expand Up @@ -491,10 +512,12 @@ TEST_F(DownloadBubbleUIControllerIncognitoTest, DoesNotShowDetailsIfDone) {
incognito_controller_->get_original_notifier_for_testing());
content::DownloadItemUtils::AttachInfoForTesting(&(item(0)),
incognito_profile_, nullptr);
// `show_details_if_done` is false because the download is initiated from the
// `may_show_details` is false because the download is initiated from the
// main profile.
EXPECT_CALL(*incognito_display_controller_,
OnUpdatedItem(/*is_done=*/true, /*show_details_if_done=*/false))
EXPECT_CALL(
*incognito_display_controller_,
OnUpdatedItem(/*is_done=*/true, /*is_pending_deep_scanning=*/false,
/*may_show_details=*/false))
.Times(1);
item(0).NotifyObserversDownloadUpdated();
}

0 comments on commit e3bbe35

Please sign in to comment.