Close privet printer notifications when clicked.

chrome/browser/printing/cloud_print/privet_notifications.cc

Index: chrome/browser/printing/cloud_print/privet_notifications.cc
diff --git a/chrome/browser/printing/cloud_print/privet_notifications.cc b/chrome/browser/printing/cloud_print/privet_notifications.cc
index d7eb4e9c691efe352c8e562449b71062b8a4a10e..784e8a3c782c6e4d3a0489488ad0f36bb8972ba2 100644
--- a/chrome/browser/printing/cloud_print/privet_notifications.cc
+++ b/chrome/browser/printing/cloud_print/privet_notifications.cc
@@ -255,6 +255,7 @@ void PrivetNotificationService::PrivetNotify(int devices_active,
   base::string16 product_name =
       l10n_util::GetStringUTF16(IDS_LOCAL_DISCOVERY_SERVICE_NAME_PRINTER);
 
+  Profile* profile = Profile::FromBrowserContext(profile_);

[dewittj, 2016/10/25 20:08:30]

if you are storing |profile_| as a Profile*, why is this here?

[Lei Zhang, 2016/10/25 22:22:52]

This is PrivetNotificationService, not PrivetNotificationDelegate. I haven't
converted PrivetNotificationService yet.

[dewittj, 2016/10/25 23:35:56]

Acknowledged.

   Notification notification(
       message_center::NOTIFICATION_TYPE_SIMPLE, title, body,
       ui::ResourceBundle::GetSharedInstance().GetImageNamed(
@@ -262,10 +263,9 @@ void PrivetNotificationService::PrivetNotify(int devices_active,
       message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
                                  kPrivetNotificationID),
       product_name, GURL(kPrivetNotificationOriginUrl), kPrivetNotificationID,
-      rich_notification_data, new PrivetNotificationDelegate(profile_));
+      rich_notification_data, new PrivetNotificationDelegate(profile));
 
   auto* notification_ui_manager = g_browser_process->notification_ui_manager();
-  Profile* profile = Profile::FromBrowserContext(profile_);
   bool updated = notification_ui_manager->Update(notification, profile);
   if (!updated && added &&
       !local_discovery::LocalDiscoveryUIHandler::GetHasVisible()) {
@@ -349,9 +349,8 @@ void PrivetNotificationService::StartLister() {
       new PrivetNotificationsListener(std::move(http_factory), this));
 }
 
-PrivetNotificationDelegate::PrivetNotificationDelegate(
-    content::BrowserContext* profile)
-    :  profile_(profile) {
+PrivetNotificationDelegate::PrivetNotificationDelegate(Profile* profile)
+    : profile_(profile) {
 }
 
 PrivetNotificationDelegate::~PrivetNotificationDelegate() {
@@ -362,6 +361,8 @@ std::string PrivetNotificationDelegate::id() const {
 }
 
 void PrivetNotificationDelegate::ButtonClick(int button_index) {
+  CloseNotification();

[dewittj, 2016/10/25 20:08:30]

Since I don't know the details of this particular delegate, I'm a little wary of
closing the notification right in the observer method, especially since no tests
were added/changed.  Perhaps a post task is in order?

[Lei Zhang, 2016/10/25 22:22:52]

Can you help me understand what the issue(s) are with closing the notification
early?

Once we reach ButtonClick() here, I don't think we need the notification for
anything else. Does CloseNotification() end up deleting |this|
PrivetNotificationDelegate?

privet_notifications_unittest.cc exists, but it does not appear to be testing
this code path. I can certainly fill in some of the test coverage now that I
noticed this.

[dewittj, 2016/10/25 23:35:56]

-warming up cache-

NotificationDelegate is refcounted, and a reference is held by the notification.
 It is possible that closing a notification right now will indeed destroy
|this|, since there don't appear to be any other objects holding references to
PrivetNotificationDelegate.

+
   if (button_index == 0) {
     ReportPrivetUmaEvent(PRIVET_NOTIFICATION_CLICKED);
     OpenTab(GURL(kPrivetNotificationOriginUrl));
@@ -374,17 +375,20 @@ void PrivetNotificationDelegate::ButtonClick(int button_index) {
 }
 
 void PrivetNotificationDelegate::OpenTab(const GURL& url) {
-  Profile* profile = Profile::FromBrowserContext(profile_);
-  chrome::NavigateParams params(profile, url,
+  chrome::NavigateParams params(profile_, url,
                                 ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
   params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
   chrome::Navigate(&params);
 }
 
 void PrivetNotificationDelegate::DisableNotifications() {
-  Profile* profile = Profile::FromBrowserContext(profile_);
-  profile->GetPrefs()->SetBoolean(prefs::kLocalDiscoveryNotificationsEnabled,
-                                  false);
+  profile_->GetPrefs()->SetBoolean(prefs::kLocalDiscoveryNotificationsEnabled,
+                                   false);
+}
+
+void PrivetNotificationDelegate::CloseNotification() {
+  g_browser_process->notification_ui_manager()->CancelById(
+      id(), NotificationUIManager::GetProfileID(profile_));
 }
 
 }  // namespace cloud_print