Componentize app_modal_dialog

chrome/test/base/ui_test_utils.cc

Index: chrome/test/base/ui_test_utils.cc
diff --git a/chrome/test/base/ui_test_utils.cc b/chrome/test/base/ui_test_utils.cc
index e017345010e36ad4766e7c5acc50ac8c2b4ae139..f03e705b664c2087046c1a903e327fd41d822f47 100644
--- a/chrome/test/base/ui_test_utils.cc
+++ b/chrome/test/base/ui_test_utils.cc
@@ -30,8 +30,6 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/app_modal_dialogs/app_modal_dialog.h"
-#include "chrome/browser/ui/app_modal_dialogs/app_modal_dialog_queue.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -48,6 +46,8 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/find_in_page_observer.h"
+#include "components/app_modal_dialogs/app_modal_dialog.h"
+#include "components/app_modal_dialogs/app_modal_dialog_queue.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/search_engines/template_url_service.h"
 #include "content/public/browser/dom_operation_notification_details.h"
@@ -107,6 +107,43 @@ Browser* WaitForBrowserNotInSet(std::set<Browser*> excluded_browsers) {
   return new_browser;
 }
 
+class AppModalDialogWaiterImpl : public AppModalDialogWaiter {

[msw, 2014/10/22 22:53:32]

Why did you scrap the notification, which seem more canonical and re-usable?
While pushing this to test code seems nice, having a one-off impl seems worse
than using a notification like before.

[oshima, 2014/10/23 00:44:12]

Notification is considered deprecated and we're eliminating them as we
componentize the code
whereever possible.

[msw, 2014/10/23 18:41:21]

Acknowledged.

+ public:
+  AppModalDialogWaiterImpl()
+      : dialog_(NULL),
+        running_(false) {
+  }
+  virtual ~AppModalDialogWaiterImpl() {
+  }
+
+  // AppModalDialogWaiter:
+  virtual AppModalDialog* Wait() override {
+    if (dialog_)
+      return dialog_;
+    running_ = true;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+    EXPECT_TRUE(dialog_);
+    return dialog_;
+  }
+
+  virtual void Notify(AppModalDialog* dialog) override {
+    DCHECK(!dialog_);
+    dialog_ = dialog;
+    if (!running_)
+      return;
+    message_loop_runner_->Quit();
+    running_ = false;
+  }
+
+ private:
+  AppModalDialog* dialog_;
+  bool running_;

[msw, 2014/10/22 22:53:31]

Why not just use content::MessageLoopRunner::loop_running() and remove this
local member?

[oshima, 2014/10/23 00:44:12]

Done.

+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppModalDialogWaiterImpl);
+};
+
 }  // namespace
 
 bool GetCurrentTabTitle(const Browser* browser, base::string16* title) {
@@ -292,16 +329,15 @@ bool GetRelativeBuildDirectory(base::FilePath* build_dir) {
   return true;
 }
 
+scoped_ptr<AppModalDialogWaiter> CreateAppModalDialogWaiter() {
+  return make_scoped_ptr(new AppModalDialogWaiterImpl());
+}
+
 AppModalDialog* WaitForAppModalDialog() {
   AppModalDialogQueue* dialog_queue = AppModalDialogQueue::GetInstance();
   if (dialog_queue->HasActiveDialog())
     return dialog_queue->active_dialog();
-
-  content::WindowedNotificationObserver observer(
-      chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
-      content::NotificationService::AllSources());
-  observer.Wait();
-  return content::Source<AppModalDialog>(observer.source()).ptr();
+  return CreateAppModalDialogWaiter()->Wait();
 }
 
 int FindInPage(WebContents* tab,