diff --git a/chrome/browser/download/notification/download_notification_browsertest.cc b/chrome/browser/download/notification/download_notification_browsertest.cc index 287bbbd453645d..e8e2dffc360331 100644 --- a/chrome/browser/download/notification/download_notification_browsertest.cc +++ b/chrome/browser/download/notification/download_notification_browsertest.cc @@ -54,6 +54,16 @@ static const TestAccountInfo kTestAccounts[] = { {"charlie@invalid.domain", "10003", "hashcharl", "Charlie"}, }; +bool IsInNotifications( + const message_center::NotificationList::Notifications& notifications, + const std::string& id) { + for (const auto& notification : notifications) { + if (notification->id() == id) + return true; + } + return false; +} + // Base class observing notification events. class MessageCenterChangeObserver : public message_center::MessageCenterObserver { @@ -128,7 +138,7 @@ class NotificationAddObserver : public MessageCenterChangeObserver { // Class observing of "UPDATE" notification events. class NotificationUpdateObserver : public MessageCenterChangeObserver { public: - NotificationUpdateObserver() : waiting_(false) { + NotificationUpdateObserver() { MessageCenterChangeObserver(); } ~NotificationUpdateObserver() override {} @@ -154,11 +164,47 @@ class NotificationUpdateObserver : public MessageCenterChangeObserver { private: std::string notification_id_; - bool waiting_; + bool waiting_ = false; DISALLOW_COPY_AND_ASSIGN(NotificationUpdateObserver); }; +// Class observing of "REMOVE" notification events. +class NotificationRemoveObserver : public MessageCenterChangeObserver { + public: + NotificationRemoveObserver() { + MessageCenterChangeObserver(); + } + ~NotificationRemoveObserver() override {} + + std::string Wait() { + if (!notification_id_.empty()) + return notification_id_; + + waiting_ = true; + RunLoop(); + waiting_ = false; + return notification_id_; + } + + // message_center::MessageCenterObserver: + void OnNotificationRemoved( + const std::string& notification_id, bool by_user) override { + if (notification_id_.empty()) { + notification_id_ = notification_id; + + if (waiting_) + QuitRunLoop(); + } + } + + private: + std::string notification_id_; + bool waiting_ = false; + + DISALLOW_COPY_AND_ASSIGN(NotificationRemoveObserver); +}; + class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate { public: explicit TestChromeDownloadManagerDelegate(Profile* profile) @@ -235,6 +281,43 @@ class DownloadNotificationTest : public DownloadNotificationTestBase { DownloadServiceFactory::GetForBrowserContext(browser()->profile()) ->GetDownloadManagerDelegate()); } + + void CreateDownload() { + GURL url(net::URLRequestSlowDownloadJob::kKnownSizeUrl); + + // Starts a download. + NotificationAddObserver download_start_notification_observer; + ui_test_utils::NavigateToURL(browser(), url); + EXPECT_TRUE(download_start_notification_observer.Wait()); + + // Confirms that a notification is created. + notification_id_ = download_start_notification_observer.notification_id(); + EXPECT_FALSE(notification_id_.empty()); + ASSERT_TRUE(notification()); + + // Confirms that there is only one notification. + message_center::NotificationList::Notifications + visible_notifications = GetMessageCenter()->GetVisibleNotifications(); + EXPECT_EQ(1u, visible_notifications.size()); + EXPECT_TRUE(IsInNotifications(visible_notifications, notification_id_)); + + // Confirms that a download is also started. + std::vector downloads; + GetDownloadManager(browser())->GetAllDownloads(&downloads); + EXPECT_EQ(1u, downloads.size()); + download_item_ = downloads[0]; + ASSERT_TRUE(download_item_); + } + + content::DownloadItem* download_item() const { return download_item_; } + std::string notification_id() const { return notification_id_; } + message_center::Notification* notification() const { + return GetNotification(notification_id_); + } + + private: + content::DownloadItem* download_item_ = nullptr; + std::string notification_id_; }; IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadFile) { @@ -280,6 +363,9 @@ IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadFile) { EXPECT_EQ(message_center::NOTIFICATION_TYPE_SIMPLE, GetNotification(notification_id)->type()); + // Opens the message center. + GetMessageCenter()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER); + // Try to open the downloaded item by clicking the notification. EXPECT_FALSE(GetDownloadManagerDelegate()->opened()); GetMessageCenter()->ClickOnNotification(notification_id); @@ -347,6 +433,59 @@ IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, DownloadMultipleFiles) { GetNotification(notification_id2)->type()); } +IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, CancelDownload) { + CreateDownload(); + + // Opens the message center. + GetMessageCenter()->SetVisibility(message_center::VISIBILITY_MESSAGE_CENTER); + + // Cancels the notification by clicking the "cancel' button. + NotificationRemoveObserver notification_close_observer; + notification()->ButtonClick(1); + EXPECT_EQ(notification_id(), notification_close_observer.Wait()); + EXPECT_EQ(0u, GetMessageCenter()->GetVisibleNotifications().size()); + + // Confirms that a download is also cancelled. + std::vector downloads; + GetDownloadManager(browser())->GetAllDownloads(&downloads); + EXPECT_EQ(1u, downloads.size()); + EXPECT_EQ(content::DownloadItem::CANCELLED, downloads[0]->GetState()); +} + +IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, + DownloadCancelledByUserExternally) { + CreateDownload(); + + // Cancels the notification by clicking the "cancel' button. + NotificationRemoveObserver notification_close_observer; + download_item()->Cancel(true /* by_user */); + EXPECT_EQ(notification_id(), notification_close_observer.Wait()); + EXPECT_EQ(0u, GetMessageCenter()->GetVisibleNotifications().size()); + + // Confirms that a download is also cancelled. + std::vector downloads; + GetDownloadManager(browser())->GetAllDownloads(&downloads); + EXPECT_EQ(1u, downloads.size()); + EXPECT_EQ(content::DownloadItem::CANCELLED, downloads[0]->GetState()); +} + +IN_PROC_BROWSER_TEST_F(DownloadNotificationTest, + DownloadCancelledExternally) { + CreateDownload(); + + // Cancels the notification by clicking the "cancel' button. + NotificationRemoveObserver notification_close_observer; + download_item()->Cancel(false /* by_user */); + EXPECT_EQ(notification_id(), notification_close_observer.Wait()); + EXPECT_EQ(0u, GetMessageCenter()->GetVisibleNotifications().size()); + + // Confirms that a download is also cancelled. + std::vector downloads; + GetDownloadManager(browser())->GetAllDownloads(&downloads); + EXPECT_EQ(1u, downloads.size()); + EXPECT_EQ(content::DownloadItem::CANCELLED, downloads[0]->GetState()); +} + ////////////////////////////////////////////////// // Test with multi profiles ////////////////////////////////////////////////// diff --git a/chrome/browser/download/notification/download_notification_item.cc b/chrome/browser/download/notification/download_notification_item.cc index 63f6f13bf8323d..e29d0b6e13c72a 100644 --- a/chrome/browser/download/notification/download_notification_item.cc +++ b/chrome/browser/download/notification/download_notification_item.cc @@ -14,6 +14,7 @@ #include "chrome/grit/generated_resources.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/download_interrupt_reasons.h" #include "content/public/browser/download_item.h" #include "content/public/browser/web_contents.h" #include "grit/theme_resources.h" @@ -101,9 +102,7 @@ DownloadNotificationItem::DownloadNotificationItem(content::DownloadItem* item, notification_->set_progress(0); notification_->set_never_timeout(false); - UpdateNotificationData(); - - notification_ui_manager()->Add(*notification_, profile); + UpdateNotificationData(ADD_NEW); } DownloadNotificationItem::~DownloadNotificationItem() { @@ -119,10 +118,8 @@ void DownloadNotificationItem::OnNotificationClick() { item_->SetOpenWhenComplete(!item_->GetOpenWhenComplete()); // Toggle } - if (item_->IsDone()) { - notification_ui_manager()->CancelById( - watcher_->id(), NotificationUIManager::GetProfileID(profile_)); - } + if (item_->IsDone()) + CloseNotificationByUser(); } void DownloadNotificationItem::OnNotificationButtonClick(int button_index) { @@ -134,6 +131,11 @@ void DownloadNotificationItem::OnNotificationButtonClick(int button_index) { } DownloadCommands::Command command = button_actions_->at(button_index); + if (command != DownloadCommands::PAUSE && + command != DownloadCommands::RESUME) { + CloseNotificationByUser(); + } + DownloadCommands(item_).ExecuteCommand(command); } @@ -141,13 +143,30 @@ void DownloadNotificationItem::OnNotificationButtonClick(int button_index) { void DownloadNotificationItem::OnDownloadUpdated(content::DownloadItem* item) { DCHECK_EQ(item, item_); - UpdateNotificationData(); + UpdateNotificationData(UPDATE_EXISTING); +} - // Updates notification. - notification_ui_manager()->Update(*notification_, profile_); +void DownloadNotificationItem::CloseNotificationByUser() { + const std::string& notification_id = watcher_->id(); + const ProfileID profile_id = NotificationUIManager::GetProfileID(profile_); + const std::string& notification_id_in_message_center = + notification_ui_manager()->FindById(notification_id, profile_id)->id(); + + notification_ui_manager()->CancelById(notification_id, profile_id); + + // When the message center is visible, |NotificationUIManager::CancelByID()| + // delays the close hence the notification is not closed at this time. But + // from the viewpoint of UX of MessageCenter, we should close it immediately + // because it's by user action. So, we request closing of it directlly to + // MessageCenter instance. + // Note that: this calling has no side-effect even when the message center + // is not opened. + g_browser_process->message_center()->RemoveNotification( + notification_id_in_message_center, true /* by_user */); } -void DownloadNotificationItem::UpdateNotificationData() { +void DownloadNotificationItem::UpdateNotificationData( + NotificationUpdateType type) { DownloadItemModel model(item_); DownloadCommands command(item_); @@ -200,9 +219,14 @@ void DownloadNotificationItem::UpdateNotificationData() { // TODO(yoshiki): Popup a notification again. break; case content::DownloadItem::CANCELLED: - notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE); - SetNotificationImage(IDR_DOWNLOAD_NOTIFICATION_WARNING); - break; + // Confgirms that a download is cancelled by user action. + DCHECK(item_->GetLastReason() == + content::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED || + item_->GetLastReason() == + content::DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN); + + CloseNotificationByUser(); + return; // Skips the remaining since the notification has closed. case content::DownloadItem::INTERRUPTED: notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE); SetNotificationImage(IDR_DOWNLOAD_NOTIFICATION_WARNING); @@ -236,6 +260,13 @@ void DownloadNotificationItem::UpdateNotificationData() { if (item_->IsDone()) { // TODO(yoshiki): If the downloaded file is an image, show the thumbnail. } + + if (type == ADD_NEW) + notification_ui_manager()->Add(*notification_, profile_); + else if (type == UPDATE_EXISTING) + notification_ui_manager()->Update(*notification_, profile_); + else + NOTREACHED(); } void DownloadNotificationItem::OnDownloadOpened(content::DownloadItem* item) { diff --git a/chrome/browser/download/notification/download_notification_item.h b/chrome/browser/download/notification/download_notification_item.h index 3e71bee377120c..ab51542f7b6cff 100644 --- a/chrome/browser/download/notification/download_notification_item.h +++ b/chrome/browser/download/notification/download_notification_item.h @@ -42,6 +42,11 @@ class DownloadNotificationItem : public content::DownloadItem::Observer { private: friend class test::DownloadNotificationItemTest; + enum NotificationUpdateType { + ADD_NEW, + UPDATE_EXISTING + }; + class NotificationWatcher : public NotificationDelegate { public: explicit NotificationWatcher(DownloadNotificationItem* item); @@ -78,7 +83,8 @@ class DownloadNotificationItem : public content::DownloadItem::Observer { void OnDownloadRemoved(content::DownloadItem* item) override; void OnDownloadDestroyed(content::DownloadItem* item) override; - void UpdateNotificationData(); + void CloseNotificationByUser(); + void UpdateNotificationData(NotificationUpdateType type); void SetNotificationImage(int resource_id); NotificationUIManager* notification_ui_manager() const;