Scroll notification panel so that new/updated notification is visible.

 * Updated the test so that the notifications have some text and reasonable size. (Empty string was making them smaller.)
 * Don't leave KEEPS_SIZE state when new notification is added. (test updated)
 * A couple of clean ups
   - const
   - removed unused code
   - Added utility function to avoid repeating static_cast.

BUG=41011
TEST=new TestScrollBalloonToVisible test is added. updated TestKeepSizeState.

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

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

6 files changed, 158 insertions, 35 deletions

diff --git a/chrome/browser/chromeos/notifications/balloon_collection_impl.cc b/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
index 732edbc..83317aa 100644
--- a/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
+++ b/chrome/browser/chromeos/notifications/balloon_collection_impl.cc
@@ -20,13 +20,6 @@
 
 namespace {
 
-// Portion of the screen allotted for notifications. When notification balloons
-// extend over this, no new notifications are shown until some are closed.
-const double kPercentBalloonFillFactor = 0.7;
-
-// Allow at least this number of balloons on the screen.
-const int kMinAllowedBalloonCount = 2;
-
 // Margin from the edge of the work area
 const int kVerticalEdgeMargin = 5;
 const int kHorizontalEdgeMargin = 5;
@@ -35,7 +28,7 @@ class NotificationMatcher {
  public:
   explicit NotificationMatcher(const Notification& notification)
       : notification_(notification) {}
-  bool operator()(const Balloon* b) {
+  bool operator()(const Balloon* b) const {
     return notification_.IsSame(b->notification());
   }
  private:
diff --git a/chrome/browser/chromeos/notifications/balloon_collection_impl.h b/chrome/browser/chromeos/notifications/balloon_collection_impl.h
index 013cfcd..df9b534 100644
--- a/chrome/browser/chromeos/notifications/balloon_collection_impl.h
+++ b/chrome/browser/chromeos/notifications/balloon_collection_impl.h
@@ -98,6 +98,8 @@ class BalloonCollectionImpl : public BalloonCollection,
                                Profile* profile);
 
  private:
+  friend class NotificationPanelTester;
+
   // Shutdown the notification ui.
   void Shutdown();
 
diff --git a/chrome/browser/chromeos/notifications/notification_browsertest.cc b/chrome/browser/chromeos/notifications/notification_browsertest.cc
index 916dabe..a86a720 100644
--- a/chrome/browser/chromeos/notifications/notification_browsertest.cc
+++ b/chrome/browser/chromeos/notifications/notification_browsertest.cc
@@ -8,6 +8,7 @@
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/notifications/balloon_collection_impl.h"
+#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_delegate.h"
@@ -78,8 +79,9 @@ class NotificationTest : public InProcessBrowserTest,
   }
 
   Notification NewMockNotification(NotificationDelegate* delegate) {
+    std::string text = delegate->id();
     return SystemNotificationFactory::Create(
-        GURL(), ASCIIToUTF16(""), ASCIIToUTF16(""),
+        GURL(), ASCIIToUTF16(text.c_str()), ASCIIToUTF16(text.c_str()),
         delegate);
   }
 
@@ -97,6 +99,13 @@ class NotificationTest : public InProcessBrowserTest,
     }
   }
 
+  // Busy loop to wait until the webkit give some size to the notification.
+  void WaitForResize(BalloonViewImpl* view) {
+    while (view->bounds().IsEmpty()) {
+      ui_test_utils::RunAllPendingInMessageLoop();
+    }
+  }
+
   // NotificationObserver overrides.
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
@@ -161,8 +170,8 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestBasic) {
   ui_test_utils::RunAllPendingInMessageLoop();
 }
 
-// [CLOSED] -add->[STICKY_AND_NEW] -mouse-> [KEEP_SIZE] -remove->
-// [CLOSED] -add-> [STICKY_AND_NEW] -remove-> [CLOSED]
+// [CLOSED] -add->[STICKY_AND_NEW] -mouse-> [KEEP_SIZE] -remove/add->
+// [KEEP_SIZE] -remove-> [CLOSED] -add-> [STICKY_AND_NEW] -remove-> [CLOSED]
 IN_PROC_BROWSER_TEST_F(NotificationTest, TestKeepSizeState) {
   BalloonCollectionImpl* collection = GetBalloonCollectionImpl();
   NotificationPanel* panel = GetNotificationPanel();
@@ -185,6 +194,18 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestKeepSizeState) {
   EXPECT_EQ(1, tester->GetNotificationCount());
   EXPECT_EQ(NotificationPanel::KEEP_SIZE, tester->state());
 
+  collection->Add(NewMockNotification("1"), browser()->profile());
+  ui_test_utils::RunAllPendingInMessageLoop();
+  EXPECT_EQ(2, tester->GetNewNotificationCount());
+  EXPECT_EQ(2, tester->GetNotificationCount());
+  EXPECT_EQ(NotificationPanel::KEEP_SIZE, tester->state());
+
+  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->Remove(NewMockNotification("2"));
   ui_test_utils::RunAllPendingInMessageLoop();
   EXPECT_EQ(0, tester->GetNotificationCount());
@@ -352,4 +373,60 @@ IN_PROC_BROWSER_TEST_F(NotificationTest, TestCleanupOnExit) {
   // end without closing.
 }
 
+IN_PROC_BROWSER_TEST_F(NotificationTest, TestScrollBalloonToVisible) {
+  BalloonCollectionImpl* collection = GetBalloonCollectionImpl();
+  NotificationPanel* panel = GetNotificationPanel();
+  NotificationPanelTester* tester = panel->GetTester();
+  Profile* profile = browser()->profile();
+
+  // Create notifications enough to overflow the panel size.
+  const int create_count = 15;
+
+  // new notification is always visible
+  for (int i = 0; i < create_count; i++) {
+    {
+      SCOPED_TRACE(StringPrintf("new n%d", i));
+      std::string id = StringPrintf("n%d", i);
+      collection->Add(NewMockNotification(id), profile);
+      EXPECT_EQ(NotificationPanel::STICKY_AND_NEW, tester->state());
+      BalloonViewImpl* view =
+          tester->GetBalloonView(collection, NewMockNotification(id));
+      WaitForResize(view);
+      EXPECT_TRUE(tester->IsVisible(view));
+    }
+    {
+      SCOPED_TRACE(StringPrintf("new s%d", i));
+      std::string id = StringPrintf("s%d", i);
+      collection->AddSystemNotification(
+          NewMockNotification(id), browser()->profile(), true, false);
+      ui_test_utils::RunAllPendingInMessageLoop();
+      BalloonViewImpl* view =
+          tester->GetBalloonView(collection, NewMockNotification(id));
+      WaitForResize(view);
+      EXPECT_TRUE(tester->IsVisible(view));
+    }
+  }
+  // updated notification must be also visible
+  for (int i = 0; i < create_count; i++) {
+    {
+      SCOPED_TRACE(StringPrintf("update n%d", i));
+      std::string id = StringPrintf("n%d", i);
+      EXPECT_TRUE(collection->UpdateNotification(NewMockNotification(id)));
+      ui_test_utils::RunAllPendingInMessageLoop();
+      BalloonViewImpl* view =
+          tester->GetBalloonView(collection, NewMockNotification(id));
+      EXPECT_TRUE(tester->IsVisible(view));
+    }
+    {
+      SCOPED_TRACE(StringPrintf("update s%d", i));
+      std::string id = StringPrintf("s%d", i);
+      EXPECT_TRUE(collection->UpdateNotification(NewMockNotification(id)));
+      ui_test_utils::RunAllPendingInMessageLoop();
+      BalloonViewImpl* view =
+          tester->GetBalloonView(collection, NewMockNotification(id));
+      EXPECT_TRUE(tester->IsVisible(view));
+    }
+  }
+}
+
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/notifications/notification_panel.cc b/chrome/browser/chromeos/notifications/notification_panel.cc
index fbd56c5..858f46f 100644
--- a/chrome/browser/chromeos/notifications/notification_panel.cc
+++ b/chrome/browser/chromeos/notifications/notification_panel.cc
@@ -56,6 +56,10 @@ const char* ToStr(const NotificationPanel::State state) {
 }
 #endif
 
+chromeos::BalloonViewImpl* GetBalloonViewOf(const Balloon* balloon) {
+  return static_cast<chromeos::BalloonViewImpl*>(balloon->view());
+}
+
 class PanelWidget : public views::WidgetGtk {
  public:
   explicit PanelWidget(chromeos::NotificationPanel* panel)
@@ -198,9 +202,8 @@ class BalloonSubContainer : public views::View {
     ConvertPointToView(GetRootView(), this, &copy);
     for (int i = GetChildViewCount() - 1; i >= 0; --i) {
       views::View* view = GetChildViewAt(i);
-      if (view->bounds().Contains(copy)) {
+      if (view->bounds().Contains(copy))
         return static_cast<BalloonViewImpl*>(view);
-      }
     }
     return NULL;
   }
@@ -256,15 +259,13 @@ class BalloonContainer : public views::View {
 
   // Adds a ballon to the panel.
   void Add(Balloon* balloon) {
-    BalloonViewImpl* view =
-        static_cast<BalloonViewImpl*>(balloon->view());
+    BalloonViewImpl* view = GetBalloonViewOf(balloon);
     GetContainerFor(balloon)->AddChildView(view);
   }
 
   // Updates the position of the |balloon|.
   bool Update(Balloon* balloon) {
-    BalloonViewImpl* view =
-        static_cast<BalloonViewImpl*>(balloon->view());
+    BalloonViewImpl* view = GetBalloonViewOf(balloon);
     View* container = NULL;
     if (sticky_container_->HasChildView(view)) {
       container = sticky_container_;
@@ -282,8 +283,7 @@ class BalloonContainer : public views::View {
 
   // Removes a ballon from the panel.
   BalloonViewImpl* Remove(Balloon* balloon) {
-    BalloonViewImpl* view =
-        static_cast<BalloonViewImpl*>(balloon->view());
+    BalloonViewImpl* view = GetBalloonViewOf(balloon);
     GetContainerFor(balloon)->RemoveChildView(view);
     return view;
   }
@@ -350,8 +350,7 @@ class BalloonContainer : public views::View {
 
  private:
   BalloonSubContainer* GetContainerFor(Balloon* balloon) const {
-    BalloonViewImpl* view =
-        static_cast<BalloonViewImpl*>(balloon->view());
+    BalloonViewImpl* view = GetBalloonViewOf(balloon);
     return view->sticky() ?
         sticky_container_ : non_sticky_container_;
   }
@@ -372,7 +371,8 @@ NotificationPanel::NotificationPanel()
       task_factory_(this),
       min_bounds_(0, 0, kBalloonMinWidth, kBalloonMinHeight),
       stale_timeout_(1000 * kStaleTimeoutInSeconds),
-      active_(NULL) {
+      active_(NULL),
+      scroll_to_(NULL) {
   Init();
 }
 
@@ -424,7 +424,7 @@ void NotificationPanel::Hide() {
 
 void NotificationPanel::Add(Balloon* balloon) {
   balloon_container_->Add(balloon);
-  if (state_ == CLOSED || state_ == MINIMIZED || state_ == KEEP_SIZE)
+  if (state_ == CLOSED || state_ == MINIMIZED)
     SET_STATE(STICKY_AND_NEW);
   Show();
   // Don't resize the panel yet. The panel will be resized when WebKit tells
@@ -432,6 +432,8 @@ void NotificationPanel::Add(Balloon* balloon) {
   UpdatePanel(false);
   UpdateControl();
   StartStaleTimer(balloon);
+  if (is_visible())
+      scroll_to_ = balloon;
 }
 
 bool NotificationPanel::Update(Balloon* balloon) {
@@ -441,6 +443,8 @@ bool NotificationPanel::Update(Balloon* balloon) {
     Show();
     UpdatePanel(true);
     StartStaleTimer(balloon);
+    if (is_visible())
+      ScrollBalloonToVisible(balloon);
     return true;
   } else {
     return false;
@@ -449,9 +453,10 @@ bool NotificationPanel::Update(Balloon* balloon) {
 
 void NotificationPanel::Remove(Balloon* balloon) {
   BalloonViewImpl* view = balloon_container_->Remove(balloon);
-  if (view == active_) {
+  if (view == active_)
     active_ = NULL;
-  }
+  if (scroll_to_ == balloon)
+    scroll_to_ = NULL;
 
   // TODO(oshima): May be we shouldn't close
   // if the mouse pointer is still on the panel.
@@ -480,16 +485,19 @@ void NotificationPanel::ResizeNotification(
       std::max(kBalloonMinHeight,
                std::min(kBalloonMaxHeight, size.height())));
   balloon->set_content_size(real_size);
-  static_cast<BalloonViewImpl*>(balloon->view())->Layout();
+  GetBalloonViewOf(balloon)->Layout();
   UpdatePanel(true);
+  if (scroll_to_ == balloon) {
+    ScrollBalloonToVisible(scroll_to_);
+    scroll_to_ = NULL;
+  }
 }
 
 void NotificationPanel::SetActiveView(BalloonViewImpl* view) {
   // Don't change the active view if it's same notification,
   // or the notification is being closed.
-  if (active_ == view || (view && view->closed())) {
+  if (active_ == view || (view && view->closed()))
     return;
-  }
   if (active_)
     active_->Deactivated();
   active_ = view;
@@ -587,8 +595,15 @@ void NotificationPanel::UnregisterNotification() {
                       Source<PanelController>(panel_controller_.get()));
 }
 
-void NotificationPanel::UpdatePanel(bool contents_changed) {
-  if (contents_changed) {
+void NotificationPanel::ScrollBalloonToVisible(Balloon* balloon) {
+  BalloonViewImpl* view = GetBalloonViewOf(balloon);
+  if (!view->closed()) {
+    view->ScrollRectToVisible(gfx::Rect(0, 0, view->width(), view->height()));
+  }
+}
+
+void NotificationPanel::UpdatePanel(bool update_panel_size) {
+  if (update_panel_size) {
     balloon_container_->UpdateBounds();
     scroll_view_->Layout();
   }
@@ -653,7 +668,7 @@ gfx::Rect NotificationPanel::GetStickyNewBounds() {
 }
 
 void NotificationPanel::StartStaleTimer(Balloon* balloon) {
-  BalloonViewImpl* view = static_cast<BalloonViewImpl*>(balloon->view());
+  BalloonViewImpl* view = GetBalloonViewOf(balloon);
   MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       task_factory_.NewRunnableMethod(
@@ -713,8 +728,25 @@ void NotificationPanelTester::MarkStale(const Notification& notification) {
   panel_->MarkStale(notification);
 }
 
-PanelController* NotificationPanelTester::GetPanelController() {
+PanelController* NotificationPanelTester::GetPanelController() const {
   return panel_->panel_controller_.get();
 }
 
+BalloonViewImpl* NotificationPanelTester::GetBalloonView(
+    BalloonCollectionImpl* collection,
+    const Notification& notification) {
+  BalloonCollectionImpl::Balloons::iterator iter =
+      collection->FindBalloon(notification);
+  DCHECK(iter != collection->balloons_.end());
+  Balloon* balloon = (*iter);
+  return GetBalloonViewOf(balloon);
+}
+
+bool NotificationPanelTester::IsVisible(const BalloonViewImpl* view) const {
+  gfx::Rect rect = panel_->scroll_view_->GetVisibleRect();
+  gfx::Point origin(0, 0);
+  views::View::ConvertPointToView(view, panel_->balloon_container_, &origin);
+  return rect.Contains(gfx::Rect(origin, view->bounds().size()));
+}
+
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/notifications/notification_panel.h b/chrome/browser/chromeos/notifications/notification_panel.h
index dfc76a5..3f86eec 100644
--- a/chrome/browser/chromeos/notifications/notification_panel.h
+++ b/chrome/browser/chromeos/notifications/notification_panel.h
@@ -120,7 +120,10 @@ class NotificationPanel : public PanelController::Delegate,
   void UnregisterNotification();
 
   // Update the Panel Size according to its state.
-  void UpdatePanel(bool contents_changed);
+  void UpdatePanel(bool update_panel_size);
+
+  // Scroll the panel so that the |balloon| is visible.
+  void ScrollBalloonToVisible(Balloon* balloon);
 
   // Update the notification's control view state.
   void UpdateControl();
@@ -145,6 +148,11 @@ class NotificationPanel : public PanelController::Delegate,
   // Mark the given notification as stale.
   void MarkStale(const Notification& notification);
 
+  // True if the panel is visible.
+  bool is_visible() {
+    return state_ != CLOSED && state_ != MINIMIZED;
+  }
+
   // Contains all notifications.
   BalloonContainer* balloon_container_;
 
@@ -177,6 +185,9 @@ class NotificationPanel : public PanelController::Delegate,
   // is out of the panel.
   BalloonViewImpl* active_;
 
+  // A balloon that should be visible when it gets some size.
+  Balloon* scroll_to_;
+
   // An object that provides interfacce for tests.
   scoped_ptr<NotificationPanelTester> tester_;
 
@@ -208,7 +219,15 @@ class NotificationPanelTester {
   // Mark the given notification as stale.
   void MarkStale(const Notification& notification);
 
-  PanelController* GetPanelController();
+  // Returns the notification panel's PanelController.
+  PanelController* GetPanelController() const;
+
+  // Returns the BalloonView object of the notification.
+  BalloonViewImpl* GetBalloonView(BalloonCollectionImpl* collection,
+                                  const Notification& notification);
+
+  // True if the view is in visible in the ScrollView.
+  bool IsVisible(const BalloonViewImpl* view) const;
 
  private:
   NotificationPanel* panel_;
diff --git a/chrome/browser/notifications/balloon.h b/chrome/browser/notifications/balloon.h
index 2df44d9..a60d9a9 100644
--- a/chrome/browser/notifications/balloon.h
+++ b/chrome/browser/notifications/balloon.h
@@ -73,7 +73,7 @@ class Balloon {
   void set_view(BalloonView* balloon_view);
 
   // Returns the balloon view associated with the balloon.
-  BalloonView* view() {
+  BalloonView* view() const {
     return balloon_view_.get();
   }