Componentize app_modal_dialog

chrome/browser/lifetime/browser_close_manager_browsertest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/files/scoped_temp_dir.h"
 #include "chrome/browser/background/background_mode_manager.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/lifetime/browser_close_manager.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #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"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #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"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/download_test_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/test/net/url_request_slow_download_job.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/chromeos_switches.h"
 #endif
 
 namespace {
 
 class AppModalDialogObserver {
  public:
   AppModalDialogObserver() {}
 
   void Start() {
-    observer_.reset(new content::WindowedNotificationObserver(
-        chrome::NOTIFICATION_APP_MODAL_DIALOG_SHOWN,
-        content::NotificationService::AllSources()));
+    waiter_ = ui_test_utils::CreateAppModalDialogWaiter();
   }
 
   void AcceptClose() {
     NativeAppModalDialog* dialog = GetNextDialog();
     ASSERT_TRUE(dialog);
     dialog->AcceptAppModalDialog();
   }
 
   void CancelClose() {
     NativeAppModalDialog* dialog = GetNextDialog();
     ASSERT_TRUE(dialog);
     dialog->CancelAppModalDialog();
   }
 
  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);
 };
 
 class RepeatedNotificationObserver : public content::NotificationObserver {
  public:
   explicit RepeatedNotificationObserver(int type, int count)
       : num_outstanding_(count), running_(false) {
     registrar_.Add(this, type, content::NotificationService::AllSources());
   }
@@ skipping 873 matching lines @@
 
   chrome::CloseAllBrowsers();
   EXPECT_FALSE(browser_shutdown::IsTryingToQuit());
   EXPECT_TRUE(chrome::BrowserIterator().done());
   EXPECT_TRUE(IsBackgroundModeSuspended());
 }
 
 INSTANTIATE_TEST_CASE_P(BrowserCloseManagerWithBackgroundModeBrowserTest,
                         BrowserCloseManagerWithBackgroundModeBrowserTest,
                         testing::Bool());