Close privet printer notifications when clicked.

chrome/browser/printing/cloud_print/privet_notifications.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/printing/cloud_print/privet_notifications.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 237 matching lines @@
       message_center::ButtonInfo(l10n_util::GetStringUTF16(
           IDS_LOCAL_DISCOVERY_NOTIFICATIONS_DISABLE_BUTTON_LABEL)));
 
   base::string16 title = l10n_util::GetPluralStringFUTF16(
       IDS_LOCAL_DISCOVERY_NOTIFICATION_TITLE_PRINTER, devices_active);
   base::string16 body = l10n_util::GetPluralStringFUTF16(
       IDS_LOCAL_DISCOVERY_NOTIFICATION_CONTENTS_PRINTER, 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(
           IDR_LOCAL_DISCOVERY_CLOUDPRINT_ICON),
       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()) {
     ReportPrivetUmaEvent(PRIVET_NOTIFICATION_SHOWN);
     notification_ui_manager->Add(notification, profile);
   }
 }
 
 void PrivetNotificationService::PrivetRemoveNotification() {
   ReportPrivetUmaEvent(PRIVET_NOTIFICATION_CANCELED);
@@ skipping 63 matching lines @@
 
   std::unique_ptr<PrivetHTTPAsynchronousFactory> http_factory(
       PrivetHTTPAsynchronousFactory::CreateInstance(
           content::BrowserContext::GetDefaultStoragePartition(profile_)->
               GetURLRequestContext()));
 
   privet_notifications_listener_.reset(
       new PrivetNotificationsListener(std::move(http_factory), this));
 }
 
-PrivetNotificationDelegate::PrivetNotificationDelegate(
-    content::BrowserContext* profile)
-    :  profile_(profile) {
+PrivetNotificationDelegate::PrivetNotificationDelegate(Profile* profile)
+    : profile_(profile) {
 }
 
 PrivetNotificationDelegate::~PrivetNotificationDelegate() {
 }
 
 std::string PrivetNotificationDelegate::id() const {
   return kPrivetNotificationID;
 }
 
 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));
     return;
   }
 
   DCHECK_EQ(1, button_index);
   ReportPrivetUmaEvent(PRIVET_DISABLE_NOTIFICATIONS_CLICKED);
   DisableNotifications();
 }
 
 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