Componentize app_modal_dialog

chrome/browser/lifetime/browser_close_manager_browsertest.cc

Index: chrome/browser/lifetime/browser_close_manager_browsertest.cc
diff --git a/chrome/browser/lifetime/browser_close_manager_browsertest.cc b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
index dd5fae9afc853f984e99b1a9f32b2cac52489eff..3ce47499111b985ad6eebffde9170895981dc9cc 100644
--- a/chrome/browser/lifetime/browser_close_manager_browsertest.cc
+++ b/chrome/browser/lifetime/browser_close_manager_browsertest.cc
@@ -21,8 +21,6 @@
 #include "chrome/browser/prefs/session_startup_pref.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
-#include "chrome/browser/ui/app_modal_dialogs/javascript_app_modal_dialog.h"
-#include "chrome/browser/ui/app_modal_dialogs/native_app_modal_dialog.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_iterator.h"
@@ -32,6 +30,8 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/app_modal_dialogs/javascript_app_modal_dialog.h"
+#include "components/app_modal_dialogs/native_app_modal_dialog.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
@@ -54,9 +54,7 @@ class AppModalDialogObserver {
   AppModalDialogObserver() {}
 
   void Start() {
-    observer_.reset(new content::WindowedNotificationObserver(
-        chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
-        content::NotificationService::AllSources()));
+    waiter_ = ui_test_utils::CreateAppModalDialogWaiter();
   }
 
   void AcceptClose() {
@@ -73,23 +71,18 @@ class AppModalDialogObserver {
 
  private:
   NativeAppModalDialog* GetNextDialog() {
-    DCHECK(observer_);
-    observer_->Wait();
-    if (observer_->source() == content::NotificationService::AllSources())
-      return NULL;
-
-    AppModalDialog* dialog =
-        content::Source<AppModalDialog>(observer_->source()).ptr();
+    DCHECK(waiter_.get());
+    AppModalDialog* dialog = waiter_->Wait();
     EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
     JavaScriptAppModalDialog* js_dialog =
         static_cast<JavaScriptAppModalDialog*>(dialog);
-    observer_.reset(new content::WindowedNotificationObserver(
-        chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
-        content::NotificationService::AllSources()));
+    // Reset before creation because only one instance of the waiter is allowd.

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

nit: "allowed"; for one-liner: s/instance of the waiter/waiter instance/

+    waiter_.reset();
+    waiter_ = ui_test_utils::CreateAppModalDialogWaiter();

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

Why not just have a GetNextDialog-local |waiter|? Better yet, why not just call
ui_test_utils::WaitForAppModalDialog()? You could just scrap
AppModalDialogObserver and have a couple Accept/Cancel local helper functions
like this:

void WaitForAndAcceptDialog() {
  AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
  EXPECT_TRUE(dialog->IsJavaScriptModalDialog());
  dialog->native_dialog()->AcceptAppModalDialog();
}

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

Sure, done.

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

The class isn't needed, but it's still a great improvement, thanks!

[oshima, 2014/10/23 21:44:18]

I see. Removed the class and replaced with function.

[msw, 2014/10/23 22:01:24]

Great, thank you!

     return js_dialog->native_dialog();
   }
 
-  scoped_ptr<content::WindowedNotificationObserver> observer_;
+  scoped_ptr<AppModalDialogWaiter> waiter_;
 
   DISALLOW_COPY_AND_ASSIGN(AppModalDialogObserver);
 };