Skip to content

Commit

Permalink
Close the notification after user clicks cancel button in message center
Browse files Browse the repository at this point in the history 
When the message center opens, NotificationUIManager::CancelById method doesn't close the notification. We need to call MessageCenter::RemoveNotification in addition.

BUG=481078
TEST=manually tested, added test succees

Review URL: https://codereview.chromium.org/1099053003

Cr-Commit-Position: refs/heads/master@{#328728}
  • Loading branch information
@yoshikig
yoshikig authored and Commit bot committed May 7, 2015
1 parent 176d269 commit 6a1efa3
Show file tree
Hide file tree
Showing 3 changed files with 193 additions and 17 deletions.
143 changes: 141 additions & 2 deletions chrome/browser/download/notification/download_notification_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {}
Expand All @@ -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)
Expand Down Expand Up @@ -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<content::DownloadItem*> 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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<content::DownloadItem*> 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<content::DownloadItem*> 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<content::DownloadItem*> downloads;
GetDownloadManager(browser())->GetAllDownloads(&downloads);
EXPECT_EQ(1u, downloads.size());
EXPECT_EQ(content::DownloadItem::CANCELLED, downloads[0]->GetState());
}

//////////////////////////////////////////////////
// Test with multi profiles
//////////////////////////////////////////////////
Expand Down
59 changes: 45 additions & 14 deletions chrome/browser/download/notification/download_notification_item.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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() {
Expand All @@ -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) {
Expand All @@ -134,20 +131,42 @@ 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);
}

// DownloadItem::Observer methods
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_);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 7 additions & 1 deletion chrome/browser/download/notification/download_notification_item.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 6a1efa3

Please sign in to comment.