Wait for notification balloon hosts' render view to be ready

so that killing the process will remove the balloon.
Fixes pyauto test testKillNotificationProcess.
BUG=80510
TEST=none

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

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

7 files changed, 65 insertions, 41 deletions

diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index 6dcaaa5..a0e9b90 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -2188,14 +2188,6 @@ void AutofillChangedObserver::IndicateDone() {
 
 namespace {
 
-// Returns whether the notification's host has a non-null process handle.
-bool IsNotificationProcessReady(Balloon* balloon) {
-  return balloon->view() &&
-         balloon->view()->GetHost() &&
-         balloon->view()->GetHost()->render_view_host() &&
-         balloon->view()->GetHost()->render_view_host()->process()->GetHandle();
-}
-
 // Returns whether all active notifications have an associated process ID.
 bool AreActiveNotificationProcessesReady() {
   NotificationUIManager* manager = g_browser_process->notification_ui_manager();
@@ -2203,7 +2195,7 @@ bool AreActiveNotificationProcessesReady() {
       manager->balloon_collection()->GetActiveBalloons();
   BalloonCollection::Balloons::const_iterator iter;
   for (iter = balloons.begin(); iter != balloons.end(); ++iter) {
-    if (!IsNotificationProcessReady(*iter))
+    if (!(*iter)->view()->GetHost()->IsRenderViewReady())
       return false;
   }
   return true;
@@ -2214,11 +2206,12 @@ bool AreActiveNotificationProcessesReady() {
 GetActiveNotificationsObserver::GetActiveNotificationsObserver(
     AutomationProvider* automation,
     IPC::Message* reply_message)
-    : reply_(automation, reply_message) {
+    : automation_(automation->AsWeakPtr()),
+      reply_message_(reply_message) {
   if (AreActiveNotificationProcessesReady()) {
     SendMessage();
   } else {
-    registrar_.Add(this, NotificationType::RENDERER_PROCESS_CREATED,
+    registrar_.Add(this, NotificationType::NOTIFY_BALLOON_CONNECTED,
                    NotificationService::AllSources());
   }
 }
@@ -2229,6 +2222,10 @@ void GetActiveNotificationsObserver::Observe(
     NotificationType type,
     const NotificationSource& source,
     const NotificationDetails& details) {
+  if (!automation_) {
+    delete this;
+    return;
+  }
   if (AreActiveNotificationProcessesReady())
     SendMessage();
 }
@@ -2238,9 +2235,9 @@ void GetActiveNotificationsObserver::SendMessage() {
       g_browser_process->notification_ui_manager();
   const BalloonCollection::Balloons& balloons =
       manager->balloon_collection()->GetActiveBalloons();
-  scoped_ptr<DictionaryValue> return_value(new DictionaryValue);
+  DictionaryValue return_value;
   ListValue* list = new ListValue;
-  return_value->Set("notifications", list);
+  return_value.Set("notifications", list);
   BalloonCollection::Balloons::const_iterator iter;
   for (iter = balloons.begin(); iter != balloons.end(); ++iter) {
     const Notification& notification = (*iter)->notification();
@@ -2253,26 +2250,45 @@ void GetActiveNotificationsObserver::SendMessage() {
         view->GetHost()->render_view_host()->process()->GetHandle()));
     list->Append(balloon);
   }
-  reply_.SendSuccess(return_value.get());
+  AutomationJSONReply(automation_,
+                      reply_message_.release()).SendSuccess(&return_value);
   delete this;
 }
 
 OnNotificationBalloonCountObserver::OnNotificationBalloonCountObserver(
     AutomationProvider* provider,
     IPC::Message* reply_message,
-    BalloonCollection* collection,
     int count)
-    : reply_(provider, reply_message),
-      collection_(collection),
+    : automation_(provider->AsWeakPtr()),
+      reply_message_(reply_message),
+      collection_(
+          g_browser_process->notification_ui_manager()->balloon_collection()),
       count_(count) {
-  collection->set_on_collection_changed_callback(NewCallback(
-      this, &OnNotificationBalloonCountObserver::OnBalloonCollectionChanged));
+  registrar_.Add(this, NotificationType::NOTIFY_BALLOON_CONNECTED,
+                 NotificationService::AllSources());
+  collection_->set_on_collection_changed_callback(NewCallback(
+      this, &OnNotificationBalloonCountObserver::CheckBalloonCount));
+  CheckBalloonCount();
+}
+
+void OnNotificationBalloonCountObserver::Observe(
+    NotificationType type,
+    const NotificationSource& source,
+    const NotificationDetails& details) {
+  CheckBalloonCount();
 }
 
-void OnNotificationBalloonCountObserver::OnBalloonCollectionChanged() {
-  if (static_cast<int>(collection_->GetActiveBalloons().size()) == count_) {
+void OnNotificationBalloonCountObserver::CheckBalloonCount() {
+  bool balloon_count_met = AreActiveNotificationProcessesReady() &&
+      static_cast<int>(collection_->GetActiveBalloons().size()) == count_;
+
+  if (balloon_count_met && automation_) {
+    AutomationJSONReply(automation_,
+                        reply_message_.release()).SendSuccess(NULL);
+  }
+
+  if (balloon_count_met || !automation_) {
     collection_->set_on_collection_changed_callback(NULL);
-    reply_.SendSuccess(NULL);
     delete this;
   }
 }
diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h
index 8a7b081..79b9ef7 100644
--- a/chrome/browser/automation/automation_provider_observers.h
+++ b/chrome/browser/automation/automation_provider_observers.h
@@ -1291,27 +1291,39 @@ class GetActiveNotificationsObserver : public NotificationObserver {
                        const NotificationDetails& details);
 
  private:
+  // Sends a message via the |AutomationProvider|. |automation_| must be valid.
+  // Deletes itself after the message is sent.
   void SendMessage();
 
-  AutomationJSONReply reply_;
   NotificationRegistrar registrar_;
+  base::WeakPtr<AutomationProvider> automation_;
+  scoped_ptr<IPC::Message> reply_message_;
 
   DISALLOW_COPY_AND_ASSIGN(GetActiveNotificationsObserver);
 };
 
 // Allows the automation provider to wait for a given number of
 // notification balloons.
-class OnNotificationBalloonCountObserver {
+class OnNotificationBalloonCountObserver : public NotificationObserver {
  public:
   OnNotificationBalloonCountObserver(AutomationProvider* provider,
                                      IPC::Message* reply_message,
-                                     BalloonCollection* collection,
                                      int count);
 
-  void OnBalloonCollectionChanged();
+  // Sends an automation reply message if |automation_| is still valid and the
+  // number of ready balloons matches the desired count. Deletes itself if the
+  // message is sent or if |automation_| is invalid.
+  void CheckBalloonCount();
+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
 
  private:
-  AutomationJSONReply reply_;
+  NotificationRegistrar registrar_;
+  base::WeakPtr<AutomationProvider> automation_;
+  scoped_ptr<IPC::Message> reply_message_;
+
   BalloonCollection* collection_;
   int count_;
 
diff --git a/chrome/browser/automation/testing_automation_provider.cc b/chrome/browser/automation/testing_automation_provider.cc
index 96bd033..e6ac9f9 100644
--- a/chrome/browser/automation/testing_automation_provider.cc
+++ b/chrome/browser/automation/testing_automation_provider.cc
@@ -4744,7 +4744,7 @@ void TestingAutomationProvider::CloseNotification(
   }
   // This will delete itself when finished.
   new OnNotificationBalloonCountObserver(
-      this, reply_message, collection, balloon_count - 1);
+      this, reply_message, balloon_count - 1);
   manager->CancelById(balloons[index]->notification().notification_id());
 }
 
@@ -4761,16 +4761,8 @@ void TestingAutomationProvider::WaitForNotificationCount(
         .SendError("'count' missing or invalid.");
     return;
   }
-  NotificationUIManager* manager = g_browser_process->notification_ui_manager();
-  BalloonCollection* collection = manager->balloon_collection();
-  const BalloonCollection::Balloons& balloons = collection->GetActiveBalloons();
-  if (static_cast<int>(balloons.size()) == count) {
-    AutomationJSONReply(this, reply_message).SendSuccess(NULL);
-    return;
-  }
   // This will delete itself when finished.
-  new OnNotificationBalloonCountObserver(
-      this, reply_message, collection, count);
+  new OnNotificationBalloonCountObserver(this, reply_message, count);
 }
 
 // Sample JSON input: { "command": "GetNTPInfo" }
diff --git a/chrome/browser/notifications/balloon_host.cc b/chrome/browser/notifications/balloon_host.cc
index 7219be0..250309d 100644
--- a/chrome/browser/notifications/balloon_host.cc
+++ b/chrome/browser/notifications/balloon_host.cc
@@ -253,3 +253,7 @@ void BalloonHost::NotifyDisconnect() {
       NotificationType::NOTIFY_BALLOON_DISCONNECTED,
       Source<BalloonHost>(this), NotificationService::NoDetails());
 }
+
+bool BalloonHost::IsRenderViewReady() const {
+  return should_notify_on_disconnect_;
+}
diff --git a/chrome/browser/notifications/balloon_host.h b/chrome/browser/notifications/balloon_host.h
index 2a03506..840d8ed 100644
--- a/chrome/browser/notifications/balloon_host.h
+++ b/chrome/browser/notifications/balloon_host.h
@@ -124,6 +124,9 @@ class BalloonHost : public RenderViewHostDelegate,
                                       const std::string& value);
   virtual void ClearInspectorSettings();
 
+  // Returns whether the associated render view is ready. Used only for testing.
+  bool IsRenderViewReady() const;
+
  protected:
   virtual ~BalloonHost();
   // Must override in platform specific implementations.
diff --git a/chrome/test/functional/PYAUTO_TESTS b/chrome/test/functional/PYAUTO_TESTS
index 773b06a..a92dffb 100644
--- a/chrome/test/functional/PYAUTO_TESTS
+++ b/chrome/test/functional/PYAUTO_TESTS
@@ -56,8 +56,6 @@
       'instant',
       'navigation',
       'notifications',
-      # crbug.com/80510
-      '-notifications.NotificationsTest.testKillNotificationProcess',
       'ntp',
       'omnibox',
       '-omnibox.OmniboxTest.testHistoryResult',  # crbug.com/71715
diff --git a/chrome/test/functional/notifications.py b/chrome/test/functional/notifications.py
index 1d2cd45..c659b58 100644
--- a/chrome/test/functional/notifications.py
+++ b/chrome/test/functional/notifications.py
@@ -450,8 +450,7 @@ class NotificationsTest(pyauto.PyUITest):
     self._AllowAllOrigins()
     self.NavigateToURL(self.TEST_PAGE_URL)
     self._CreateHTMLNotification(self.EMPTY_PAGE_URL)
-    self.KillRendererProcess(
-        self.GetActiveNotifications()[0]['pid'])
+    self.KillRendererProcess(self.GetActiveNotifications()[0]['pid'])
     self.assertTrue(self.IsBrowserRunning())
     self.WaitForNotificationCount(0)