Fix Ash notification updates

In Chrome, it turns out that every Notification has a unique id. In the Ash implementation we need to track and update that id when an existing Bubble is updated.

This also ensures that the UI is safely created initially (in case the initial asynchronous Update is delayed) to prevent crashes and to make Layout more consistent.

Also includes some cleanup of the WebNotificationTray API.

BUG=141285


Review URL: https://chromiumcodereview.appspot.com/10855079

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150979 0039d316-1c4b-4281-b951-d872f2087c98

ash/system/web_notification/web_notification_tray.cc

@@ -124,11 +124,6 @@ class WebNotificationList {
                        const string16& display_source,
                        const std::string& extension_id) {
     WebNotification notification;
-    Notifications::iterator iter = GetNotification(id);
-    if (iter != notifications_.end()) {
-      notification = *iter;
-      EraseNotification(iter);
-    }
     notification.id = id;
     notification.title = title;
     notification.message = message;
@@ -138,14 +133,16 @@ class WebNotificationList {
     PushNotification(notification);
   }
 
-  void UpdateNotificationMessage(const std::string& id,
+  void UpdateNotificationMessage(const std::string& old_id,
+                                 const std::string& new_id,
                                  const string16& title,
                                  const string16& message) {
-    Notifications::iterator iter = GetNotification(id);
+    Notifications::iterator iter = GetNotification(old_id);
     if (iter == notifications_.end())
       return;
     // Copy and update notification, then move it to the front of the list.
     WebNotification notification(*iter);
+    notification.id = new_id;
     notification.title = title;
     notification.message = message;
     notification.is_read = false;
@@ -206,6 +203,10 @@ class WebNotificationList {
     return notifications_.front().id;
   }
 
+  bool HasNotification(const std::string& id) {
+    return GetNotification(id) != notifications_.end();
+  }
+
   const Notifications& notifications() const { return notifications_; }
   int unread_count() const { return unread_count_; }
 
@@ -226,6 +227,12 @@ class WebNotificationList {
   }
 
   void PushNotification(const WebNotification& notification) {
+    // Ensure that notification.id is unique by erasing any existing
+    // notification with the same id (shouldn't normally happen).
+    Notifications::iterator iter = GetNotification(notification.id);
+    if (iter != notifications_.end())
+      EraseNotification(iter);
+    // Add the notification to the front (top) of the list.
     if (!is_visible_)
       ++unread_count_;
     notifications_.push_front(notification);
@@ -439,10 +446,8 @@ class WebNotificationView : public views::View,
   // Overridden from ButtonListener.
   virtual void ButtonPressed(views::Button* sender,
                              const views::Event& event) OVERRIDE {
-    if (sender == close_button_) {
-      tray_->RemoveNotification(notification_.id);
-      tray_->HideMessageCenterBubbleIfEmpty();
-    }
+    if (sender == close_button_)
+      tray_->SendRemoveNotification(notification_.id);
   }
 
   // Overridden from MenuButtonListener.
@@ -515,10 +520,8 @@ class WebNotificationButtonView : public views::View,
   // Overridden from ButtonListener.
   virtual void ButtonPressed(views::Button* sender,
                              const views::Event& event) OVERRIDE {
-    if (sender == close_all_button_) {
-      tray_->RemoveAllNotifications();
-      tray_->HideMessageCenterBubbleIfEmpty();
-    }
+    if (sender == close_all_button_)
+      tray_->SendRemoveAllNotifications();
   }
 
  private:
@@ -592,10 +595,14 @@ class MessageCenterContentsView : public WebContentsView {
 
     button_view_ = new internal::WebNotificationButtonView(tray);
     AddChildView(button_view_);
+
+    // Build initial view with no notifications.
+    Update(WebNotificationList::Notifications());
   }
 
   void Update(const WebNotificationList::Notifications& notifications) {
     scroll_content_->RemoveAllChildViews(true);
+    scroll_content_->set_preferred_size(gfx::Size());
     int num_children = 0;
     for (WebNotificationList::Notifications::const_iterator iter =
              notifications.begin(); iter != notifications.end(); ++iter) {
@@ -616,6 +623,7 @@ class MessageCenterContentsView : public WebContentsView {
     }
     SizeScrollContent();
     Layout();
+    if (GetWidget())
       GetWidget()->GetRootView()->SchedulePaint();
   }
 
@@ -655,15 +663,20 @@ class WebNotificationContentsView : public WebContentsView {
     content_->SetLayoutManager(
         new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 1));
     AddChildView(content_);
+
+    // Build initial view with no notification.
+    Update(WebNotificationList::Notifications());
   }
 
   void Update(const WebNotificationList::Notifications& notifications) {
     content_->RemoveAllChildViews(true);
-    WebNotificationList::Notifications::const_iterator iter =
-        notifications.begin();
-    WebNotificationView* view = new WebNotificationView(tray_, *iter);
+    const WebNotification& notification = (notifications.size() > 0) ?
+        notifications.front() : WebNotification();
+    WebNotificationView* view = new WebNotificationView(tray_, notification);
     content_->AddChildView(view);
+    content_->SizeToPreferredSize();
     Layout();
+    if (GetWidget())
       GetWidget()->GetRootView()->SchedulePaint();
   }
 
@@ -837,6 +850,9 @@ void WebNotificationTray::SetDelegate(Delegate* delegate) {
   delegate_ = delegate;
 }
 
+// Add/Update/RemoveNotification are called by the client code, i.e the
+// Delegate implementation or its proxy.
+
 void WebNotificationTray::AddNotification(const std::string& id,
                                           const string16& title,
                                           const string16& message,
@@ -848,10 +864,11 @@ void WebNotificationTray::AddNotification(const std::string& id,
   ShowNotificationBubble();
 }
 
-void WebNotificationTray::UpdateNotification(const std::string& id,
+void WebNotificationTray::UpdateNotification(const std::string& old_id,
+                                             const std::string& new_id,
                                              const string16& title,
                                              const string16& message) {
-  notification_list_->UpdateNotificationMessage(id, title, message);
+  notification_list_->UpdateNotificationMessage(old_id, new_id, title, message);
   UpdateTrayAndBubble();
   ShowNotificationBubble();
 }
@@ -861,26 +878,6 @@ void WebNotificationTray::RemoveNotification(const std::string& id) {
     HideNotificationBubble();
   if (!notification_list_->RemoveNotification(id))
     return;
-  if (delegate_)
-    delegate_->NotificationRemoved(id);
-  UpdateTrayAndBubble();
-}
-
-void WebNotificationTray::RemoveAllNotifications() {
-  const WebNotificationList::Notifications& notifications =
-      notification_list_->notifications();
-  if (delegate_) {
-    for (WebNotificationList::Notifications::const_iterator loopiter =
-             notifications.begin();
-         loopiter != notifications.end(); ) {
-      WebNotificationList::Notifications::const_iterator curiter = loopiter++;
-      std::string notification_id = curiter->id;
-      // May call RemoveNotification and erase curiter.
-      delegate_->NotificationRemoved(notification_id);
-    }
-  }
-  notification_list_->RemoveAllNotifications();
-  HideMessageCenterBubble();
   UpdateTrayAndBubble();
 }
 
@@ -893,27 +890,6 @@ void WebNotificationTray::SetNotificationImage(const std::string& id,
     ShowNotificationBubble();
 }
 
-void WebNotificationTray::DisableByExtension(const std::string& id) {
-  // When we disable notifications, we remove any existing matching
-  // notifications to avoid adding complicated UI to re-enable the source.
-  if (id == notification_list_->GetFirstId())
-    HideNotificationBubble();
-  notification_list_->RemoveNotificationsByExtension(id);
-  UpdateTrayAndBubble();
-  if (delegate_)
-    delegate_->DisableExtension(id);
-}
-
-void WebNotificationTray::DisableByUrl(const std::string& id) {
-  // See comment for DisableByExtension.
-  if (id == notification_list_->GetFirstId())
-    HideNotificationBubble();
-  notification_list_->RemoveNotificationsBySource(id);
-  UpdateTrayAndBubble();
-  if (delegate_)
-    delegate_->DisableNotificationsFromSource(id);
-}
-
 void WebNotificationTray::ShowMessageCenterBubble() {
   if (status_area_widget()->login_status() == user::LOGGED_IN_LOCKED)
     return;
@@ -938,11 +914,6 @@ void WebNotificationTray::HideMessageCenterBubble() {
   status_area_widget()->SetHideSystemNotifications(false);
 }
 
-void WebNotificationTray::HideMessageCenterBubbleIfEmpty() {
-  if (GetNotificationCount() == 0)
-    HideMessageCenterBubble();
-}
-
 void WebNotificationTray::ShowNotificationBubble() {
   if (status_area_widget()->login_status() == user::LOGGED_IN_LOCKED)
     return;
@@ -979,16 +950,6 @@ void WebNotificationTray::UpdateAfterLoginStatusChange(
   UpdateTray();
 }
 
-void WebNotificationTray::ShowSettings(const std::string& id) {
-  if (delegate_)
-    delegate_->ShowSettings(id);
-}
-
-void WebNotificationTray::OnClicked(const std::string& id) {
-  if (delegate_)
-    delegate_->OnClicked(id);
-}
-
 void WebNotificationTray::SetShelfAlignment(ShelfAlignment alignment) {
   if (alignment == shelf_alignment())
     return;
@@ -1002,6 +963,50 @@ void WebNotificationTray::SetShelfAlignment(ShelfAlignment alignment) {
   HideNotificationBubble();
 }
 
+// Protected methods (invoked only from Bubble and its child classes)
+
+void WebNotificationTray::SendRemoveNotification(const std::string& id) {
+  // If this is the only notification in the list, close the bubble.
+  if (notification_list_->notifications().size() == 1 &&
+      id == notification_list_->GetFirstId()) {
+    HideMessageCenterBubble();
+  }
+  if (delegate_)
+    delegate_->NotificationRemoved(id);
+}
+
+void WebNotificationTray::SendRemoveAllNotifications() {
+  HideMessageCenterBubble();
+  if (delegate_) {
+    const WebNotificationList::Notifications& notifications =
+        notification_list_->notifications();
+    for (WebNotificationList::Notifications::const_iterator loopiter =
+             notifications.begin();
+         loopiter != notifications.end(); ) {
+      WebNotificationList::Notifications::const_iterator curiter = loopiter++;
+      std::string notification_id = curiter->id;
+      // May call RemoveNotification and erase curiter.
+      delegate_->NotificationRemoved(notification_id);
+    }
+  }
+}
+
+// When we disable notifications, we remove any existing matching
+// notifications to avoid adding complicated UI to re-enable the source.
+void WebNotificationTray::DisableByExtension(const std::string& id) {
+  // Will call SendRemoveNotification for each matching notification.
+  notification_list_->RemoveNotificationsByExtension(id);
+  if (delegate_)
+    delegate_->DisableExtension(id);
+}
+
+void WebNotificationTray::DisableByUrl(const std::string& id) {
+  // Will call SendRemoveNotification for each matching notification.
+  notification_list_->RemoveNotificationsBySource(id);
+  if (delegate_)
+    delegate_->DisableNotificationsFromSource(id);
+}
+
 bool WebNotificationTray::PerformAction(const views::Event& event) {
   if (message_center_bubble())
     HideMessageCenterBubble();
@@ -1014,6 +1019,18 @@ int WebNotificationTray::GetNotificationCount() const {
   return notification_list()->notifications().size();
 }
 
+void WebNotificationTray::ShowSettings(const std::string& id) {
+  if (delegate_)
+    delegate_->ShowSettings(id);
+}
+
+void WebNotificationTray::OnClicked(const std::string& id) {
+  if (delegate_)
+    delegate_->OnClicked(id);
+}
+
+// Private methods
+
 void WebNotificationTray::UpdateTray() {
   count_label_->SetText(UTF8ToUTF16(
       GetNotificationText(notification_list()->unread_count())));
@@ -1048,4 +1065,8 @@ void WebNotificationTray::HideBubble(Bubble* bubble) {
   }
 }
 
+bool WebNotificationTray::HasNotificationForTest(const std::string& id) const {
+  return notification_list_->HasNotification(id);
+}
+
 }  // namespace ash

ash/system/web_notification/web_notification_tray.h

@@ -27,7 +27,10 @@ namespace ash {
 
 namespace internal {
 class StatusAreaWidget;
+class WebNotificationButtonView;
 class WebNotificationList;
+class WebNotificationMenuModel;
+class WebNotificationView;
 }
 
 // Status area tray for showing browser and app notifications. The client
@@ -85,34 +88,25 @@ class ASH_EXPORT WebNotificationTray : public internal::TrayBackgroundView {
                        const string16& display_source,
                        const std::string& extension_id);
 
-  // Update an existing notification.
-  void UpdateNotification(const std::string& id,
+  // Update an existing notification with id = old_id and set its id to new_id.
+  void UpdateNotification(const std::string& old_id,
+                          const std::string& new_id,
                           const string16& title,
                           const string16& message);
 
-  // Remove an existing notification and notify the delegate.
+  // Remove an existing notification.
   void RemoveNotification(const std::string& id);
 
-  // Remove all notifications and notify the delegate.
-  void RemoveAllNotifications();
-
   // Set the notification image.
   void SetNotificationImage(const std::string& id,
                             const gfx::ImageSkia& image);
 
-  // Disable all notifications matching notification |id|.
-  void DisableByExtension(const std::string& id);
-  void DisableByUrl(const std::string& id);
-
   // Show the message center bubble. Should only be called by StatusAreaWidget.
   void ShowMessageCenterBubble();
 
   // Hide the message center bubble. Should only be called by StatusAreaWidget.
   void HideMessageCenterBubble();
 
-  // Hide the message center bubble if there are no notifications.
-  void HideMessageCenterBubbleIfEmpty();
-
   // Show a single notification bubble for the most recent notification.
   void ShowNotificationBubble();
 
@@ -122,26 +116,42 @@ class ASH_EXPORT WebNotificationTray : public internal::TrayBackgroundView {
   // Updates tray visibility login status of the system changes.
   void UpdateAfterLoginStatusChange(user::LoginStatus login_status);
 
-  // Request the Delegate to the settings dialog.
-  void ShowSettings(const std::string& id);
-
-  // Called when a notification is clicked on. Event is passed to the Delegate.
-  void OnClicked(const std::string& id);
-
   // Overridden from TrayBackgroundView.
   virtual void SetShelfAlignment(ShelfAlignment alignment) OVERRIDE;
 
   // Overridden from internal::ActionableView.
   virtual bool PerformAction(const views::Event& event) OVERRIDE;
 
+ protected:
+  // Send a remove request to the delegate.
+  void SendRemoveNotification(const std::string& id);
+
+  // Send a remove request for all notifications to the delegate.
+  void SendRemoveAllNotifications();
+
+  // Disable all notifications matching notification |id|.
+  void DisableByExtension(const std::string& id);
+  void DisableByUrl(const std::string& id);
+
+  // Request the Delegate to the settings dialog.
+  void ShowSettings(const std::string& id);
+
+  // Called when a notification is clicked on. Event is passed to the Delegate.
+  void OnClicked(const std::string& id);
+
  private:
   class Bubble;
+  friend class internal::WebNotificationButtonView;
+  friend class internal::WebNotificationMenuModel;
+  friend class internal::WebNotificationView;
   FRIEND_TEST_ALL_PREFIXES(WebNotificationTrayTest, WebNotifications);
+  FRIEND_TEST_ALL_PREFIXES(WebNotificationTrayTest, WebNotificationBubble);
 
   int GetNotificationCount() const;
   void UpdateTray();
   void UpdateTrayAndBubble();
   void HideBubble(Bubble* bubble);
+  bool HasNotificationForTest(const std::string& id) const;
 
   const internal::WebNotificationList* notification_list() const {
     return notification_list_.get();

ash/system/web_notification/web_notification_tray_unittest.cc

@@ -55,8 +55,19 @@ class TestDelegate : public WebNotificationTray::Delegate {
                           "" /* extension id */);
   }
 
+  void UpdateNotification(WebNotificationTray* tray,
+                          const std::string& old_id,
+                          const std::string& new_id) {
+    notification_ids_.erase(old_id);
+    notification_ids_.insert(new_id);
+    tray->UpdateNotification(old_id, new_id,
+                             ASCIIToUTF16("Updated Web Notification"),
+                             ASCIIToUTF16("Updated message body."));
+  }
+
   void RemoveNotification(WebNotificationTray* tray, const std::string& id) {
     tray->RemoveNotification(id);
+    notification_ids_.erase(id);
   }
 
   bool HasNotificationId(const std::string& id) {
@@ -80,23 +91,57 @@ TEST_F(WebNotificationTrayTest, WebNotifications) {
 
   ASSERT_TRUE(tray->GetWidget());
 
-  // Adding a notification should show the bubble.
+  // Add a notification.
   delegate->AddNotification(tray, "test_id1");
-  EXPECT_TRUE(tray->notification_bubble() != NULL);
   EXPECT_EQ(1, tray->GetNotificationCount());
+  EXPECT_TRUE(tray->HasNotificationForTest("test_id1"));
   delegate->AddNotification(tray, "test_id2");
   delegate->AddNotification(tray, "test_id2");
   EXPECT_EQ(2, tray->GetNotificationCount());
-  // Ensure that removing a notification removes it from the tray, and signals
-  // the delegate.
-  EXPECT_TRUE(delegate->HasNotificationId("test_id2"));
-  delegate->RemoveNotification(tray, "test_id2");
+  EXPECT_TRUE(tray->HasNotificationForTest("test_id2"));
+
+  // Ensure that updating a notification does not affect the count.
+  delegate->UpdateNotification(tray, "test_id2", "test_id3");
+  delegate->UpdateNotification(tray, "test_id3", "test_id3");
+  EXPECT_EQ(2, tray->GetNotificationCount());
   EXPECT_FALSE(delegate->HasNotificationId("test_id2"));
-  EXPECT_EQ(1, tray->GetNotificationCount());
+  EXPECT_FALSE(tray->HasNotificationForTest("test_id2"));
+  EXPECT_TRUE(delegate->HasNotificationId("test_id3"));
 
-  // Removing the last notification should hide the bubble.
+  // Ensure that Removing the first notification removes it from the tray.
   delegate->RemoveNotification(tray, "test_id1");
+  EXPECT_FALSE(delegate->HasNotificationId("test_id1"));
+  EXPECT_FALSE(tray->HasNotificationForTest("test_id1"));
+  EXPECT_EQ(1, tray->GetNotificationCount());
+
+  // Remove the remianing notification.
+  delegate->RemoveNotification(tray, "test_id3");
   EXPECT_EQ(0, tray->GetNotificationCount());
+  EXPECT_FALSE(tray->HasNotificationForTest("test_id3"));
+}
+
+TEST_F(WebNotificationTrayTest, WebNotificationBubble) {
+  WebNotificationTray* tray = GetWebNotificationTray();
+  scoped_ptr<TestDelegate> delegate(new TestDelegate);
+  tray->SetDelegate(delegate.get());
+
+  ASSERT_TRUE(tray->GetWidget());
+
+  // Adding a notification should show the bubble.
+  delegate->AddNotification(tray, "test_id1");
+  EXPECT_TRUE(tray->notification_bubble() != NULL);
+
+  // Updating a notification should not hide the bubble.
+  delegate->AddNotification(tray, "test_id2");
+  delegate->UpdateNotification(tray, "test_id2", "test_id3");
+  EXPECT_TRUE(tray->notification_bubble() != NULL);
+
+  // Removing the first notification should not hide the bubble.
+  delegate->RemoveNotification(tray, "test_id1");
+  EXPECT_TRUE(tray->notification_bubble() != NULL);
+
+  // Removing the visible notification should hide the bubble.
+  delegate->RemoveNotification(tray, "test_id3");
   EXPECT_TRUE(tray->notification_bubble() == NULL);
 }
 

chrome/browser/ui/views/ash/balloon_view_ash.cc

@@ -93,8 +93,9 @@ BalloonViewAsh::~BalloonViewAsh() {
 void BalloonViewAsh::Show(Balloon* balloon) {
   balloon_ = balloon;
   const Notification& notification = balloon_->notification();
+  current_notification_id_ = notification.notification_id();
   std::string extension_id = GetExtensionId(balloon);
-  GetWebNotificationTray()->AddNotification(notification.notification_id(),
+  GetWebNotificationTray()->AddNotification(current_notification_id_,
                                             notification.title(),
                                             notification.body(),
                                             notification.display_source(),
@@ -104,9 +105,12 @@ void BalloonViewAsh::Show(Balloon* balloon) {
 
 void BalloonViewAsh::Update() {
   const Notification& notification = balloon_->notification();
-  GetWebNotificationTray()->UpdateNotification(notification.notification_id(),
+  std::string new_notification_id = notification.notification_id();
+  GetWebNotificationTray()->UpdateNotification(current_notification_id_,
+                                               new_notification_id,
                                                notification.title(),
                                                notification.body());
+  current_notification_id_ = new_notification_id;
   FetchIcon(notification);
 }
 

chrome/browser/ui/views/ash/balloon_view_ash.h

@@ -30,6 +30,8 @@ class BalloonViewAsh : public BalloonView {
   BalloonCollection* collection_;
   Balloon* balloon_;
   scoped_ptr<IconFetcher> icon_fetcher_;
+  // Track the current notification id so that it can be updated properly.
+  std::string current_notification_id_;
 
   DISALLOW_COPY_AND_ASSIGN(BalloonViewAsh);
 };