Revert 65879 - When an extension is uninstalled, close all desktop notifications from that extension.

This change also refactors the balloon collection code to remove duplication between chrome and chromeos.

Removes some gross removal code which was using fake notifications just to get the right ID.

BUG=58266
TEST=open notifications from extension, uninstall extensions

Review URL: http://codereview.chromium.org/4635007

TBR=johnnyg@chromium.org
Review URL: http://codereview.chromium.org/4855002

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

8 files changed, 118 insertions, 55 deletions

diff --git a/chrome/browser/chromeos/notifications/balloon_collection_impl.cc b/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
index 14da2d6..56a3d1c 100644
--- a/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
+++ b/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "base/logging.h"
+#include "base/stl_util-inl.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/chromeos/notifications/balloon_view.h"
 #include "chrome/browser/chromeos/notifications/notification_panel.h"
@@ -23,6 +24,17 @@ namespace {
 const int kVerticalEdgeMargin = 5;
 const int kHorizontalEdgeMargin = 5;
 
+class NotificationMatcher {
+ public:
+  explicit NotificationMatcher(const Notification& notification)
+      : notification_(notification) {}
+  bool operator()(const Balloon* b) const {
+    return notification_.IsSame(b->notification());
+  }
+ private:
+  Notification notification_;
+};
+
 }  // namespace
 
 namespace chromeos {
@@ -40,7 +52,7 @@ BalloonCollectionImpl::~BalloonCollectionImpl() {
 void BalloonCollectionImpl::Add(const Notification& notification,
                                 Profile* profile) {
   Balloon* new_balloon = MakeBalloon(notification, profile);
-  base_.Add(new_balloon);
+  balloons_.push_back(new_balloon);
   new_balloon->Show();
   notification_ui_->Add(new_balloon);
 
@@ -53,13 +65,13 @@ bool BalloonCollectionImpl::AddDOMUIMessageCallback(
     const Notification& notification,
     const std::string& message,
     MessageCallback* callback) {
-  Balloon* balloon = FindBalloon(notification);
-  if (!balloon) {
+  Balloons::iterator iter = FindBalloon(notification);
+  if (iter == balloons_.end()) {
     delete callback;
     return false;
   }
   BalloonViewHost* host =
-      static_cast<BalloonViewHost*>(balloon->view()->GetHost());
+      static_cast<BalloonViewHost*>((*iter)->view()->GetHost());
   return host->AddDOMUIMessageCallback(message, callback);
 }
 
@@ -68,11 +80,10 @@ void BalloonCollectionImpl::AddSystemNotification(
     Profile* profile,
     bool sticky,
     bool control) {
-
   Balloon* new_balloon = new Balloon(notification, profile, this);
   new_balloon->set_view(
       new chromeos::BalloonViewImpl(sticky, control, true));
-  base_.Add(new_balloon);
+  balloons_.push_back(new_balloon);
   new_balloon->Show();
   notification_ui_->Add(new_balloon);
 
@@ -83,9 +94,10 @@ void BalloonCollectionImpl::AddSystemNotification(
 
 bool BalloonCollectionImpl::UpdateNotification(
     const Notification& notification) {
-  Balloon* balloon = FindBalloon(notification);
-  if (!balloon)
+  Balloons::iterator iter = FindBalloon(notification);
+  if (iter == balloons_.end())
     return false;
+  Balloon* balloon = *iter;
   balloon->Update(notification);
   notification_ui_->Update(balloon);
   return true;
@@ -93,9 +105,10 @@ bool BalloonCollectionImpl::UpdateNotification(
 
 bool BalloonCollectionImpl::UpdateAndShowNotification(
     const Notification& notification) {
-  Balloon* balloon = FindBalloon(notification);
-  if (!balloon)
+  Balloons::iterator iter = FindBalloon(notification);
+  if (iter == balloons_.end())
     return false;
+  Balloon* balloon = *iter;
   balloon->Update(notification);
   bool updated = notification_ui_->Update(balloon);
   DCHECK(updated);
@@ -103,12 +116,15 @@ bool BalloonCollectionImpl::UpdateAndShowNotification(
   return true;
 }
 
-bool BalloonCollectionImpl::RemoveById(const std::string& id) {
-  return base_.CloseById(id);
-}
-
-bool BalloonCollectionImpl::RemoveBySourceOrigin(const GURL& origin) {
-  return base_.CloseAllBySourceOrigin(origin);
+bool BalloonCollectionImpl::Remove(const Notification& notification) {
+  Balloons::iterator iter = FindBalloon(notification);
+  if (iter != balloons_.end()) {
+    // Balloon.CloseByScript() will cause OnBalloonClosed() to be called on
+    // this object, which will remove it from the collection and free it.
+    (*iter)->CloseByScript();
+    return true;
+  }
+  return false;
 }
 
 bool BalloonCollectionImpl::HasSpace() const {
@@ -121,9 +137,15 @@ void BalloonCollectionImpl::ResizeBalloon(Balloon* balloon,
 }
 
 void BalloonCollectionImpl::OnBalloonClosed(Balloon* source) {
+  // We want to free the balloon when finished.
+  scoped_ptr<Balloon> closed(source);
+
   notification_ui_->Remove(source);
-  base_.Remove(source);
 
+  Balloons::iterator iter = FindBalloon(source->notification());
+  if (iter != balloons_.end()) {
+    balloons_.erase(iter);
+  }
   // There may be no listener in a unit test.
   if (space_change_listener_)
     space_change_listener_->OnBalloonSpaceChanged();
@@ -148,6 +170,7 @@ void BalloonCollectionImpl::Shutdown() {
   // themselves from the parent.
   DVLOG(1) << "Shutting down notification UI";
   notification_ui_.reset();
+  STLDeleteElements(&balloons_);
 }
 
 Balloon* BalloonCollectionImpl::MakeBalloon(const Notification& notification,
@@ -157,6 +180,13 @@ Balloon* BalloonCollectionImpl::MakeBalloon(const Notification& notification,
   return new_balloon;
 }
 
+std::deque<Balloon*>::iterator BalloonCollectionImpl::FindBalloon(
+    const Notification& notification) {
+  return std::find_if(balloons_.begin(),
+                      balloons_.end(),
+                      NotificationMatcher(notification));
+}
+
 }  // namespace chromeos
 
 // static
diff --git a/chrome/browser/chromeos/notifications/balloon_collection_impl.h b/chrome/browser/chromeos/notifications/balloon_collection_impl.h
index c9c74b5..79cfe14 100644
--- a/chrome/browser/chromeos/notifications/balloon_collection_impl.h
+++ b/chrome/browser/chromeos/notifications/balloon_collection_impl.h
@@ -12,7 +12,6 @@
 #include "base/scoped_ptr.h"
 #include "chrome/browser/chromeos/notifications/balloon_view_host.h"
 #include "chrome/browser/notifications/balloon_collection.h"
-#include "chrome/browser/notifications/balloon_collection_base.h"
 #include "chrome/common/notification_registrar.h"
 #include "gfx/point.h"
 #include "gfx/rect.h"
@@ -61,13 +60,12 @@ class BalloonCollectionImpl : public BalloonCollection,
   // BalloonCollectionInterface overrides
   virtual void Add(const Notification& notification,
                    Profile* profile);
-  virtual bool RemoveById(const std::string& id);
-  virtual bool RemoveBySourceOrigin(const GURL& origin);
+  virtual bool Remove(const Notification& notification);
   virtual bool HasSpace() const;
   virtual void ResizeBalloon(Balloon* balloon, const gfx::Size& size);
   virtual void DisplayChanged() {}
   virtual void OnBalloonClosed(Balloon* source);
-  virtual const Balloons& GetActiveBalloons() { return base_.balloons(); }
+  virtual const Balloons& GetActiveBalloons() { return balloons_; }
 
   // NotificationObserver overrides:
   virtual void Observe(NotificationType type,
@@ -117,18 +115,19 @@ class BalloonCollectionImpl : public BalloonCollection,
   virtual Balloon* MakeBalloon(const Notification& notification,
                                Profile* profile);
 
-  // Base implementation for the collection of active balloons.
-  BalloonCollectionBase base_;
-
  private:
   friend class NotificationPanelTester;
 
   // Shutdown the notification ui.
   void Shutdown();
 
-  Balloon* FindBalloon(const Notification& notification) {
-    return base_.FindBalloon(notification);
-  }
+  // The number of balloons being displayed.
+  int count() const { return balloons_.size(); }
+
+  Balloons::iterator FindBalloon(const Notification& notification);
+
+  // Queue of active balloons.
+  Balloons balloons_;
 
   scoped_ptr<NotificationUI> notification_ui_;
 
diff --git a/chrome/browser/chromeos/notifications/balloon_view.cc b/chrome/browser/chromeos/notifications/balloon_view.cc
index ff5ea0f..24a6004 100644
--- a/chrome/browser/chromeos/notifications/balloon_view.cc
+++ b/chrome/browser/chromeos/notifications/balloon_view.cc
@@ -15,7 +15,6 @@
 #include "chrome/browser/chromeos/notifications/notification_panel.h"
 #include "chrome/browser/notifications/balloon.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
-#include "chrome/browser/notifications/notification.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/renderer_host/render_widget_host_view.h"
@@ -301,8 +300,7 @@ void BalloonViewImpl::Observe(NotificationType type,
 // BalloonViewImpl public.
 
 bool BalloonViewImpl::IsFor(const Notification& notification) const {
-  return balloon_->notification().notification_id() ==
-      notification.notification_id();
+  return balloon_->notification().IsSame(notification);
 }
 
 void BalloonViewImpl::Activated() {
diff --git a/chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc b/chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc
index 377828b..8c0a4be 100644
--- a/chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc
+++ b/chrome/browser/chromeos/notifications/desktop_notifications_unittest.cc
@@ -24,24 +24,33 @@ class MockNotificationUI : public BalloonCollectionImpl::NotificationUI {
   virtual void SetActiveView(BalloonViewImpl* view) {}
 };
 
-MockBalloonCollection::MockBalloonCollection() {
+MockBalloonCollection::MockBalloonCollection()
+    : log_proxy_(new LoggingNotificationProxy()) {
   set_notification_ui(new MockNotificationUI());
 }
 
 void MockBalloonCollection::Add(const Notification& notification,
                                 Profile* profile) {
-  // Swap in a logging proxy for the purpose of logging calls that
+  // Swap in the logging proxy for the purpose of logging calls that
   // would be made into javascript, then pass this down to the
   // balloon collection.
-  Notification test_notification(
-      notification.origin_url(),
-      notification.content_url(),
-      notification.display_source(),
-      notification.replace_id(),
-      new LoggingNotificationProxy(notification.notification_id()));
+  Notification test_notification(notification.origin_url(),
+                                 notification.content_url(),
+                                 notification.display_source(),
+                                 string16(), /* replace_id */
+                                 log_proxy_.get());
   BalloonCollectionImpl::Add(test_notification, profile);
 }
 
+bool MockBalloonCollection::Remove(const Notification& notification) {
+  Notification test_notification(notification.origin_url(),
+                                 notification.content_url(),
+                                 notification.display_source(),
+                                 string16(), /* replace_id */
+                                 log_proxy_.get());
+  return BalloonCollectionImpl::Remove(test_notification);
+}
+
 Balloon* MockBalloonCollection::MakeBalloon(const Notification& notification,
                                             Profile* profile) {
   // Start with a normal balloon but mock out the view.
diff --git a/chrome/browser/chromeos/notifications/desktop_notifications_unittest.h b/chrome/browser/chromeos/notifications/desktop_notifications_unittest.h
index 5471b3b..8e0d7ae 100644
--- a/chrome/browser/chromeos/notifications/desktop_notifications_unittest.h
+++ b/chrome/browser/chromeos/notifications/desktop_notifications_unittest.h
@@ -17,6 +17,7 @@
 #include "chrome/browser/notifications/balloon.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/notification.h"
+#include "chrome/browser/notifications/notification_object_proxy.h"
 #include "chrome/browser/notifications/notification_test_util.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/notifications/notifications_prefs_cache.h"
@@ -27,7 +28,7 @@
 namespace chromeos {
 
 class DesktopNotificationsTest;
-typedef LoggingNotificationDelegate<DesktopNotificationsTest>
+typedef LoggingNotificationProxyBase<DesktopNotificationsTest>
     LoggingNotificationProxy;
 
 // Test version of the balloon collection which counts the number
@@ -39,6 +40,7 @@ class MockBalloonCollection : public BalloonCollectionImpl {
   // BalloonCollectionImpl overrides
   virtual void Add(const Notification& notification,
                    Profile* profile);
+  virtual bool Remove(const Notification& notification);
   virtual Balloon* MakeBalloon(const Notification& notification,
                                Profile* profile);
   virtual void OnBalloonClosed(Balloon* source);
@@ -52,6 +54,7 @@ class MockBalloonCollection : public BalloonCollectionImpl {
 
  private:
   std::set<Balloon*> balloons_;
+  scoped_refptr<LoggingNotificationProxy> log_proxy_;
 };
 
 class DesktopNotificationsTest : public testing::Test {
diff --git a/chrome/browser/chromeos/notifications/notification_browsertest.cc b/chrome/browser/chromeos/notifications/notification_browsertest.cc
index 3a8758f..5901454 100644
--- a/chrome/browser/chromeos/notifications/notification_browsertest.cc
+++ b/chrome/browser/chromeos/notifications/notification_browsertest.cc
@@ -16,7 +16,7 @@
 #include "chrome/browser/chromeos/notifications/balloon_view.h"
 #include "chrome/browser/chromeos/notifications/notification_panel.h"
 #include "chrome/browser/chromeos/notifications/system_notification_factory.h"
-#include "chrome/browser/notifications/notification_test_util.h"
+#include "chrome/browser/notifications/notification_delegate.h"
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/test/in_process_browser_test.h"
@@ -24,6 +24,22 @@
 
 namespace {
 
+class MockNotificationDelegate : public NotificationDelegate {
+ public:
+  explicit MockNotificationDelegate(const std::string& id) : id_(id) {}
+
+  virtual void Display() {}
+  virtual void Error() {}
+  virtual void Close(bool by_user) {}
+  virtual void Click() {}
+  virtual std::string id() const { return id_; }
+
+ private:
+  std::string id_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockNotificationDelegate);
+};
+
 // The name of ChromeOS's window manager.
 const char* kChromeOsWindowManagerName = "chromeos-wm";
 
@@ -146,14 +162,14 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestBasic) {
   EXPECT_EQ(2, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
 
-  collection->RemoveById("1");
+  collection->Remove(NewMockNotification("1"));
   ui_test_utils::RunAllPendingInMessageLoop();
 
   EXPECT_EQ(1, tester->GetNewNotificationCount());
   EXPECT_EQ(1, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
 
-  collection->RemoveById("2");
+  collection->Remove(NewMockNotification("2"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNewNotificationCount());
   EXPECT_EQ(0, tester->GetNotificationCount());
@@ -182,7 +198,7 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestKeepSizeState) {
   panel->OnMouseMotion(gfx::Point(10, 10));
   EXPECT_EQ(NotificationPanel::KEEP_SIZE, tester->state());
 
-  collection->RemoveById("1");
+  collection->Remove(NewMockNotification("1"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(1, tester->GetNewNotificationCount());
   EXPECT_EQ(1, tester->GetNotificationCount());
@@ -194,20 +210,20 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestKeepSizeState) {
   EXPECT_EQ(2, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::KEEP_SIZE, tester->state());
 
-  collection->RemoveById("1");
+  collection->Remove(NewMockNotification("1"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(1, tester->GetNewNotificationCount());
   EXPECT_EQ(1, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::KEEP_SIZE, tester->state());
 
-  collection->RemoveById("2");
+  collection->Remove(NewMockNotification("2"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::CLOSED, tester->state());
 
   collection->Add(NewMockNotification("3"), browser()->profile());
   EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
-  collection->RemoveById("3");
+  collection->Remove(NewMockNotification("3"));
 
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNotificationCount());
@@ -241,7 +257,10 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestSystemNotification) {
   EXPECT_EQ(1, tester->GetStickyNotificationCount());
 
   // Dismiss the notification.
-  collection->RemoveById(delegate->id());
+  // TODO(oshima): Consider updating API to Remove(NotificationDelegate)
+  // or Remove(std::string id);
+  collection->Remove(Notification(GURL(), GURL(), string16(), string16(),
+                                  delegate.get()));
   ui_test_utils::RunAllPendingInMessageLoop();
 
   EXPECT_EQ(0, tester->GetStickyNotificationCount());
@@ -268,11 +287,11 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestStateTransition1) {
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(NotificationPanel::MINIMIZED, tester->state());
 
-  collection->RemoveById("2");
+  collection->Remove(NewMockNotification("2"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(NotificationPanel::MINIMIZED, tester->state());
 
-  collection->RemoveById("1");
+  collection->Remove(NewMockNotification("1"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::CLOSED, tester->state());
@@ -338,19 +357,20 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, FLAKY_TestStateTransition2) {
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
 
-  collection->RemoveById("1");
+  collection->Remove(NewMockNotification("1"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
 
   // Removing the system notification should minimize the panel.
-  collection->RemoveById("3");
+  collection->Remove(NewMockNotification("3"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(1, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::MINIMIZED, tester->state());
   WaitForPanelState(tester, PanelController::MINIMIZED);
 
   // Removing the last notification. Should close the panel.
-  collection->RemoveById("2");
+
+  collection->Remove(NewMockNotification("2"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::CLOSED, tester->state());
diff --git a/chrome/browser/chromeos/notifications/notification_panel.cc b/chrome/browser/chromeos/notifications/notification_panel.cc
index 6ab3b31..1ff6014 100644
--- a/chrome/browser/chromeos/notifications/notification_panel.cc
+++ b/chrome/browser/chromeos/notifications/notification_panel.cc
@@ -848,8 +848,10 @@ PanelController* NotificationPanelTester::GetPanelController() const {
 BalloonViewImpl* NotificationPanelTester::GetBalloonView(
     BalloonCollectionImpl* collection,
     const Notification& notification) {
-  Balloon* balloon = collection->FindBalloon(notification);
-  DCHECK(balloon);
+  BalloonCollectionImpl::Balloons::iterator iter =
+      collection->FindBalloon(notification);
+  DCHECK(iter != collection->balloons_.end());
+  Balloon* balloon = (*iter);
   return GetBalloonViewOf(balloon);
 }
 
diff --git a/chrome/browser/chromeos/notifications/system_notification.cc b/chrome/browser/chromeos/notifications/system_notification.cc
index 0d85687..f83c414 100644
--- a/chrome/browser/chromeos/notifications/system_notification.cc
+++ b/chrome/browser/chromeos/notifications/system_notification.cc
@@ -65,7 +65,9 @@ void SystemNotification::Show(const string16& message,
 
 void SystemNotification::Hide() {
   if (visible_) {
-    collection_->RemoveById(delegate_->id());
+    collection_->Remove(Notification(GURL(), GURL(), string16(), string16(),
+                                     delegate_.get()));
+
     visible_ = false;
     urgent_ = false;
   }