Update first download notification correctly if there are multiple notifications

chrome/browser/download/notification/download_item_notification.cc

 // Copyright 2015 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/download/notification/download_item_notification.h"
 
 #include "base/files/file_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
@@ skipping 145 matching lines @@
       break;
   }
 }
 
 }  // anonymous namespace
 
 DownloadItemNotification::DownloadItemNotification(
     content::DownloadItem* item,
     DownloadNotificationManagerForProfile* manager)
     : item_(item),
+      message_center_(manager->message_center()),
       weak_factory_(this) {
+
   // Creates the notification instance. |title|, |body| and |icon| will be
   // overridden by UpdateNotificationData() below.
   notification_.reset(new Notification(
       message_center::NOTIFICATION_TYPE_PROGRESS,
       base::string16(),  // title
       base::string16(),  // body
       gfx::Image(),      // icon
       message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
                                  kDownloadNotificationNotifierId),
       base::string16(),                    // display_source
@@ skipping 15 matching lines @@
 
 bool DownloadItemNotification::HasNotificationClickedListener() {
   if (item_->IsDangerous()) {
     // Dangerous notifications don't have a click handler.
     return false;
   }
   return true;
 }
 
 void DownloadItemNotification::OnNotificationClose() {
-  visible_ = false;
-
   if (item_ && item_->IsDangerous() && !item_->IsDone()) {
     content::RecordAction(
         UserMetricsAction("DownloadNotification.Close_Dangerous"));
     item_->Cancel(true /* by_user */);
     return;
   }
 
   if (image_decode_status_ == IN_PROGRESS) {
     image_decode_status_ = NOT_STARTED;
     ImageDecoder::Cancel(this);
@@ skipping 93 matching lines @@
   g_browser_process->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(
+  message_center_->RemoveNotification(
       notification_id_in_message_center, true /* by_user */);
 }
 
 void DownloadItemNotification::Update() {
   auto download_state = item_->GetState();
 
   // When the download is just completed, interrupted or transitions to
   // dangerous, close the notification once and re-show it immediately so
   // it'll pop up.
   bool popup =
       ((item_->IsDangerous() && !previous_dangerous_state_) ||
        (download_state == content::DownloadItem::COMPLETE &&
         previous_download_state_ != content::DownloadItem::COMPLETE) ||
        (download_state == content::DownloadItem::INTERRUPTED &&
         previous_download_state_ != content::DownloadItem::INTERRUPTED));
 
-  if (visible_) {
+  if (IsNotificationVisible()) {

[yoshiki, 2015/10/10 05:31:13]

The cause of the issue is that |visible_| is not updated correctly when the
notification is closed by DisablePopup(). So this patch removes this variable
and makes it use IsNotificationVisible() instead.

     UpdateNotificationData(popup ? UPDATE_AND_POPUP : UPDATE);
   } else {
-    if (show_next_ || popup) {
+    if (show_next_ || popup)
       UpdateNotificationData(ADD);
-      visible_ = true;
-    }
   }
 
   show_next_ = false;
   previous_download_state_ = item_->GetState();
   previous_dangerous_state_ = item_->IsDangerous();
 }
 
 void DownloadItemNotification::UpdateNotificationData(
     NotificationUpdateType type) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 71 matching lines @@
         Add(*notification_, profile());
   } else if (type == UPDATE ||
              // If the notification is already visible as popup or in the
              // notification center, doesn't pop it up.
              (type == UPDATE_AND_POPUP && IsNotificationVisible())) {
     // Shows a notifiation as progress type once so the visible content will be
     // updated. Only progress-type notification's content will be updated
     // immediately when the message center is visible.
     // See the comment in MessageCenterImpl::UpdateNotification() for detail.
     if (type == UPDATE_AND_POPUP &&
-        g_browser_process->message_center()->IsMessageCenterVisible() &&
+        message_center_->IsMessageCenterVisible() &&
         (item_->GetState() == content::DownloadItem::COMPLETE ||
          item_->GetState() == content::DownloadItem::INTERRUPTED)) {
       DCHECK_EQ(notification_->type(),
                 message_center::NOTIFICATION_TYPE_BASE_FORMAT);
 
       notification_->set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
       g_browser_process->notification_ui_manager()->
           Update(*notification_, profile());
       notification_->set_type(message_center::NOTIFICATION_TYPE_BASE_FORMAT);
     }
@@ skipping 451 matching lines @@
   return browser_displayer.browser();
 }
 
 Profile* DownloadItemNotification::profile() const {
   return Profile::FromBrowserContext(item_->GetBrowserContext());
 }
 
 bool DownloadItemNotification::IsNotificationVisible() const {
   const std::string& notification_id = watcher()->id();
   const ProfileID profile_id = NotificationUIManager::GetProfileID(profile());
-  const std::string notification_id_in_message_center =
-      ProfileNotification::GetProfileNotificationId(notification_id,
-                                                    profile_id);
+  if (!g_browser_process->notification_ui_manager())
+    return false;
+  const Notification* notification = g_browser_process->
+      notification_ui_manager()->FindById(notification_id, profile_id);

[yoshiki, 2015/10/10 05:31:13]

Instead of generating notification-id-in-message-center, this retrieves it from
the actual notification. These ID must be same.

The reason why I changed is that StubNotificationUIManager generates the ID in
different way in test.

+  if (!notification)
+    return false;
+
+  const std::string notification_id_in_message_center = notification->id();
 
   message_center::NotificationList::Notifications visible_notifications =
-      g_browser_process->message_center()->GetVisibleNotifications();
+      message_center_->GetVisibleNotifications();
   for (const auto& notification : visible_notifications) {
     if (notification->id() == notification_id_in_message_center)
       return true;
   }
   return false;
 }