Add WaitForCondition() and use it in ExtensionTestNotificationObserver.

chrome/browser/extensions/extension_test_notification_observer.cc

Index: chrome/browser/extensions/extension_test_notification_observer.cc
diff --git a/chrome/browser/extensions/extension_test_notification_observer.cc b/chrome/browser/extensions/extension_test_notification_observer.cc
index f8b7a6d4ecb1865c55ee1b6a87f2938dac03ba69..7d089aa79e59bfe4982f86bec778d56ef7f034fb 100644
--- a/chrome/browser/extensions/extension_test_notification_observer.cc
+++ b/chrome/browser/extensions/extension_test_notification_observer.cc
@@ -20,6 +20,36 @@ using extensions::Extension;
 
 namespace {
 
+class ConditionRunLoop {
+ public:
+  typedef base::Callback<bool(void)> ConditionCallback;
+
+  explicit ConditionRunLoop(const ConditionCallback& callback);
+
+  void Wait();
+
+  void Check();
+
+ private:
+  base::RunLoop run_loop_;
+  ConditionCallback condition_callback_;
+};
+
+ConditionRunLoop::ConditionRunLoop(const ConditionCallback& callback)
+    : condition_callback_(callback) {}
+
+void ConditionRunLoop::Wait() {
+  if (condition_callback_.Run())
+    return;
+
+  run_loop_.Run();
+}
+
+void ConditionRunLoop::Check() {
+  if (condition_callback_.Run())
+    run_loop_.Quit();
+}
+
 bool HasExtensionPageActionCountReachedTarget(LocationBarTesting* location_bar,
                                               int target_page_action_count) {
   VLOG(1) << "Number of page actions: " << location_bar->PageActionCount();
@@ -35,6 +65,68 @@ bool HasExtensionPageActionVisibilityReachedTarget(
          target_visible_page_action_count;
 }
 
+bool HaveAllExtensionRenderViewHostsFinishedLoading(
+    ExtensionProcessManager* manager) {
+  ExtensionProcessManager::ViewSet all_views = manager->GetAllViews();
+  for (ExtensionProcessManager::ViewSet::const_iterator iter =
+           all_views.begin();
+       iter != all_views.end(); ++iter) {
+    if ((*iter)->IsLoading())
+      return false;
+  }
+  return true;
+}
+
+class NotificationCallbackRunner : public content::NotificationObserver {

[Jeffrey Yasskin, 2013/10/24 23:49:23]

What if we had something like:

class NotificationSet : public content::NotificationObserver {
 public:
  NotificationSet* Add(int type, const content::NotificationSource& source =
content::NotificationService::AllSources());
  // Notified any time an Add()ed notification is received.
  // The details of the notification are dropped.
  CallbackList<void()>& callback_list();
}

Then we could write WaitForCondition(Callback<bool()>, NotificationSet*) to
replace the multi-notification case in WaitForExtensionViewsToLoad().

We could, instead, make WaitForCondition(Callback<bool()> a member of the
NotificationSet, which would replace the callback_list() and possibly the
inheritance from NotificationObserver, but might make the class a little less
general.

[Bernhard Bauer, 2013/10/29 16:37:02]

Done. As an aside, the interface to CallbackList is Not Awesome.
Or we could add a method ConditionRunLoop::WaitWithCallbackList, which would
subscribe itself to the callback list, then wait on it? 

[Jeffrey Yasskin, 2013/11/06 03:33:59]

:-/
I'd actually suggest getting rid of ConditionRunLoop now that it's only used in
one place. See my comment on WaitForCondition.

+ public:
+  NotificationCallbackRunner(int type,
+                             const content::NotificationSource& source,
+                             const base::Closure& callback);
+
+ private:
+  // content::NotificationObserver
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  content::NotificationRegistrar notification_registrar_;
+  base::Closure callback_;
+};
+
+NotificationCallbackRunner::NotificationCallbackRunner(
+    int type,
+    const content::NotificationSource& source,
+    const base::Closure& callback)
+    : callback_(callback) {
+  notification_registrar_.Add(this, type, source);
+}
+
+void NotificationCallbackRunner::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  callback_.Run();
+}
+
+void WaitForConditionWithNotification(
+    const base::Callback<bool(void)>& condition,
+    int type,
+    const content::NotificationSource& source) {
+  ConditionRunLoop run_loop(condition);
+  NotificationCallbackRunner callback_runner(
+      type,
+      source,
+      base::Bind(&ConditionRunLoop::Check, base::Unretained(&run_loop)));
+  run_loop.Wait();
+}
+
+void WaitForConditionWithNotification(
+    const base::Callback<bool(void)>& condition,
+    int type) {
+  WaitForConditionWithNotification(
+      condition, type, content::NotificationService::AllSources());
+}
+
 }  // namespace
 
 ExtensionTestNotificationObserver::ExtensionTestNotificationObserver(
@@ -74,57 +166,38 @@ bool ExtensionTestNotificationObserver::WaitForPageActionCountChangeTo(
     int count) {
   LocationBarTesting* location_bar =
       browser_->window()->GetLocationBar()->GetLocationBarForTesting();
-  if (!HasExtensionPageActionCountReachedTarget(location_bar, count)) {
-    content::WindowedNotificationObserver(
-        chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_COUNT_CHANGED,
-        base::Bind(
-            &HasExtensionPageActionCountReachedTarget, location_bar, count))
-        .Wait();
-  }
-  return HasExtensionPageActionCountReachedTarget(location_bar, count);
+  WaitForConditionWithNotification(

[Jeffrey Yasskin, 2013/10/24 23:49:23]

I definitely like this since it removes the duplicated condition.

+      base::Bind(
+          &HasExtensionPageActionCountReachedTarget, location_bar, count),
+      chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_COUNT_CHANGED);
+  return true;
 }
 
 bool ExtensionTestNotificationObserver::WaitForPageActionVisibilityChangeTo(
     int count) {
   LocationBarTesting* location_bar =
       browser_->window()->GetLocationBar()->GetLocationBarForTesting();
-  if (!HasExtensionPageActionVisibilityReachedTarget(location_bar, count)) {
-    content::WindowedNotificationObserver(
-        chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED,
-        base::Bind(&HasExtensionPageActionVisibilityReachedTarget,
-                   location_bar,
-                   count)).Wait();
-  }
-  return HasExtensionPageActionVisibilityReachedTarget(location_bar, count);
+  WaitForConditionWithNotification(
+      base::Bind(
+          &HasExtensionPageActionVisibilityReachedTarget, location_bar, count),
+      chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED);
+  return true;
 }
 
 bool ExtensionTestNotificationObserver::WaitForExtensionViewsToLoad() {
   ExtensionProcessManager* manager =
-    extensions::ExtensionSystem::Get(GetProfile())->process_manager();
-  ExtensionProcessManager::ViewSet all_views = manager->GetAllViews();
-  for (ExtensionProcessManager::ViewSet::const_iterator iter =
-         all_views.begin();
-       iter != all_views.end();) {
-    if (!(*iter)->IsLoading()) {
-      ++iter;
-    } else {
-      // Wait for all the extension render view hosts that exist to finish
-      // loading.
-      content::WindowedNotificationObserver observer(
-          content::NOTIFICATION_LOAD_STOP,
-          content::NotificationService::AllSources());
-      observer.AddNotificationType(
-                                   content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                                   content::NotificationService::AllSources());
-      observer.Wait();
-
-      // Test activity may have modified the set of extension processes during
-      // message processing, so re-start the iteration to catch added/removed
-      // processes.
-      all_views = manager->GetAllViews();
-      iter = all_views.begin();
-    }
-  }
+      extensions::ExtensionSystem::Get(GetProfile())->process_manager();
+  ConditionRunLoop run_loop(
+      base::Bind(&HaveAllExtensionRenderViewHostsFinishedLoading, manager));
+  NotificationCallbackRunner callback_runner(

[Jeffrey Yasskin, 2013/10/24 23:49:23]

I don't really like using NotificationCallbackRunner directly since it makes it
harder to expose a simple interface to other files, and I think we want this as
a general test utility.

+      content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
+      content::NotificationService::AllSources(),
+      base::Bind(&ConditionRunLoop::Check, base::Unretained(&run_loop)));
+  NotificationCallbackRunner callback_runner_2(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources(),
+      base::Bind(&ConditionRunLoop::Check, base::Unretained(&run_loop)));
+  run_loop.Wait();
   return true;
 }