From b47bc8784483dba0d1c3defa0d0285c2ed9b2e77 Mon Sep 17 00:00:00 2001 From: meacer Date: Mon, 6 Oct 2014 20:40:01 -0700 Subject: [PATCH] Move InterstitialObserver and WaitForInterstitial to browser_test_utils BUG=4086 Review URL: https://codereview.chromium.org/628433002 Cr-Commit-Position: refs/heads/master@{#298353} --- chrome/browser/apps/web_view_browsertest.cc | 37 +------ .../safe_browsing_blocking_page_test.cc | 51 ++------- chrome/browser/ui/browser_browsertest.cc | 114 +++++---------------- chrome/browser/ui/browser_focus_uitest.cc | 20 +--- .../browser/ui/login/login_prompt_browsertest.cc | 64 ++---------- content/public/test/browser_test_utils.cc | 60 ++++++++++- content/public/test/browser_test_utils.h | 13 +++ 7 files changed, 112 insertions(+), 247 deletions(-) diff --git a/chrome/browser/apps/web_view_browsertest.cc b/chrome/browser/apps/web_view_browsertest.cc index 22d91088a551..53da8e35e390 100644 --- a/chrome/browser/apps/web_view_browsertest.cc +++ b/chrome/browser/apps/web_view_browsertest.cc @@ -120,31 +120,6 @@ class WebContentsHiddenObserver : public content::WebContentsObserver { DISALLOW_COPY_AND_ASSIGN(WebContentsHiddenObserver); }; -class InterstitialObserver : public content::WebContentsObserver { - public: - InterstitialObserver(content::WebContents* web_contents, - const base::Closure& attach_callback, - const base::Closure& detach_callback) - : WebContentsObserver(web_contents), - attach_callback_(attach_callback), - detach_callback_(detach_callback) { - } - - virtual void DidAttachInterstitialPage() OVERRIDE { - attach_callback_.Run(); - } - - virtual void DidDetachInterstitialPage() OVERRIDE { - detach_callback_.Run(); - } - - private: - base::Closure attach_callback_; - base::Closure detach_callback_; - - DISALLOW_COPY_AND_ASSIGN(InterstitialObserver); -}; - void ExecuteScriptWaitForTitle(content::WebContents* web_contents, const char* script, const char* title) { @@ -633,16 +608,6 @@ class WebViewTest : public extensions::PlatformAppBrowserTest { ASSERT_TRUE(test_run_listener.WaitUntilSatisfied()); } - void WaitForInterstitial(content::WebContents* web_contents) { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - InterstitialObserver observer(web_contents, - loop_runner->QuitClosure(), - base::Closure()); - if (!content::InterstitialPage::GetInterstitialPage(web_contents)) - loop_runner->Run(); - } - void LoadAppWithGuest(const std::string& app_path) { ExtensionTestMessageListener launched_listener("WebViewTest.LAUNCHED", false); @@ -1255,7 +1220,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, MAYBE_InterstitialTeardown) { content::WebContents* guest_web_contents = GetGuestViewManager()->WaitForGuestCreated(); ASSERT_TRUE(guest_web_contents->GetRenderProcessHost()->IsIsolatedGuest()); - WaitForInterstitial(guest_web_contents); + content::WaitForInterstitialAttach(guest_web_contents); // Now close the app while interstitial page being shown in guest. extensions::AppWindow* window = GetFirstAppWindow(); diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc index 21df9f3751e9..42568705109f 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc @@ -34,6 +34,7 @@ #include "content/public/browser/render_frame_host.h" #include "content/public/browser/render_view_host.h" #include "content/public/browser/web_contents.h" +#include "content/public/test/browser_test_utils.h" #include "content/public/test/test_browser_thread.h" #include "content/public/test/test_utils.h" @@ -48,31 +49,6 @@ const char kEmptyPage[] = "files/empty.html"; const char kMalwarePage[] = "files/safe_browsing/malware.html"; const char kMalwareIframe[] = "files/safe_browsing/malware_iframe.html"; -class InterstitialObserver : public content::WebContentsObserver { - public: - InterstitialObserver(content::WebContents* web_contents, - const base::Closure& attach_callback, - const base::Closure& detach_callback) - : WebContentsObserver(web_contents), - attach_callback_(attach_callback), - detach_callback_(detach_callback) { - } - - virtual void DidAttachInterstitialPage() OVERRIDE { - attach_callback_.Run(); - } - - virtual void DidDetachInterstitialPage() OVERRIDE { - detach_callback_.Run(); - } - - private: - base::Closure attach_callback_; - base::Closure detach_callback_; - - DISALLOW_COPY_AND_ASSIGN(InterstitialObserver); -}; - // A SafeBrowsingDatabaseManager class that allows us to inject the malicious // URLs. class FakeSafeBrowsingDatabaseManager : public SafeBrowsingDatabaseManager { @@ -483,18 +459,6 @@ class SafeBrowsingBlockingPageBrowserTest return interstitial_page != NULL; } - void WaitForInterstitial() { - WebContents* contents = - browser()->tab_strip_model()->GetActiveWebContents(); - scoped_refptr loop_runner( - new content::MessageLoopRunner); - InterstitialObserver observer(contents, - loop_runner->QuitClosure(), - base::Closure()); - if (!InterstitialPage::GetInterstitialPage(contents)) - loop_runner->Run(); - } - void SetReportSentCallback(const base::Closure& callback) { factory_.most_recent_service() ->fake_ui_manager() @@ -520,7 +484,9 @@ class SafeBrowsingBlockingPageBrowserTest GURL("javascript:" + open_function + "()"), CURRENT_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB); - WaitForInterstitial(); + WebContents* contents = + browser()->tab_strip_model()->GetActiveWebContents(); + content::WaitForInterstitialAttach(contents); // Cancel the redirect request while interstitial page is open. browser()->tab_strip_model()->ActivateTabAt(0, true); ui_test_utils::NavigateToURLWithDisposition( @@ -598,15 +564,10 @@ class SafeBrowsingBlockingPageBrowserTest // We wait for interstitial_detached rather than nav_entry_committed, as // going back from a main-frame malware interstitial page will not cause a // nav entry committed event. - scoped_refptr loop_runner( - new content::MessageLoopRunner); - InterstitialObserver observer( - browser()->tab_strip_model()->GetActiveWebContents(), - base::Closure(), - loop_runner->QuitClosure()); if (!Click(node_id)) return false; - loop_runner->Run(); + content::WaitForInterstitialDetach( + browser()->tab_strip_model()->GetActiveWebContents()); return true; } diff --git a/chrome/browser/ui/browser_browsertest.cc b/chrome/browser/ui/browser_browsertest.cc index 67d5d203105c..63a856d9a17d 100644 --- a/chrome/browser/ui/browser_browsertest.cc +++ b/chrome/browser/ui/browser_browsertest.cc @@ -175,31 +175,6 @@ class MockTabStripModelObserver : public TabStripModelObserver { DISALLOW_COPY_AND_ASSIGN(MockTabStripModelObserver); }; -class InterstitialObserver : public content::WebContentsObserver { - public: - InterstitialObserver(content::WebContents* web_contents, - const base::Closure& attach_callback, - const base::Closure& detach_callback) - : WebContentsObserver(web_contents), - attach_callback_(attach_callback), - detach_callback_(detach_callback) { - } - - virtual void DidAttachInterstitialPage() override { - attach_callback_.Run(); - } - - virtual void DidDetachInterstitialPage() override { - detach_callback_.Run(); - } - - private: - base::Closure attach_callback_; - base::Closure detach_callback_; - - DISALLOW_COPY_AND_ASSIGN(InterstitialObserver); -}; - // Causes the browser to swap processes on a redirect to an HTTPS URL. class TransferHttpsRedirectsContentBrowserClient : public chrome::ChromeContentBrowserClient { @@ -1892,17 +1867,9 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialCommandDisable) { WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - TestInterstitialPage* interstitial = NULL; - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - loop_runner->QuitClosure(), - base::Closure()); - interstitial = new TestInterstitialPage(contents, false, GURL()); - loop_runner->Run(); - } + TestInterstitialPage* interstitial = + new TestInterstitialPage(contents, false, GURL()); + content::WaitForInterstitialAttach(contents); EXPECT_TRUE(contents->ShowingInterstitialPage()); @@ -1911,17 +1878,11 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialCommandDisable) { EXPECT_FALSE(command_updater->IsCommandEnabled(IDC_SAVE_PAGE)); EXPECT_FALSE(command_updater->IsCommandEnabled(IDC_ENCODING_MENU)); - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - base::Closure(), - loop_runner->QuitClosure()); - interstitial->Proceed(); - loop_runner->Run(); - // interstitial is deleted now. - } + // Proceed and wait for interstitial to detach. This doesn't destroy + // |contents|. + interstitial->Proceed(); + content::WaitForInterstitialDetach(contents); + // interstitial is deleted now. EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_VIEW_SOURCE)); EXPECT_TRUE(command_updater->IsCommandEnabled(IDC_PRINT)); @@ -1945,33 +1906,19 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialClosesDialogs) { AppModalDialogQueue* dialog_queue = AppModalDialogQueue::GetInstance(); EXPECT_TRUE(dialog_queue->HasActiveDialog()); - TestInterstitialPage* interstitial = NULL; - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - loop_runner->QuitClosure(), - base::Closure()); - interstitial = new TestInterstitialPage(contents, false, GURL()); - loop_runner->Run(); - } + TestInterstitialPage* interstitial = + new TestInterstitialPage(contents, false, GURL()); + content::WaitForInterstitialAttach(contents); // The interstitial should have closed the dialog. EXPECT_TRUE(contents->ShowingInterstitialPage()); EXPECT_FALSE(dialog_queue->HasActiveDialog()); - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - base::Closure(), - loop_runner->QuitClosure()); - interstitial->DontProceed(); - loop_runner->Run(); - // interstitial is deleted now. - } + // Don't proceed and wait for interstitial to detach. This doesn't destroy + // |contents|. + interstitial->DontProceed(); + content::WaitForInterstitialDetach(contents); + // interstitial is deleted now. // Make sure input events still work in the renderer process. EXPECT_FALSE(contents->GetRenderProcessHost()->IgnoreInputEvents()); @@ -1981,31 +1928,16 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialClosesDialogs) { IN_PROC_BROWSER_TEST_F(BrowserTest, InterstitialCloseTab) { WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents(); - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - loop_runner->QuitClosure(), - base::Closure()); - // Interstitial will delete itself when we close the tab. - new TestInterstitialPage(contents, false, GURL()); - loop_runner->Run(); - } + // Interstitial will delete itself when we close the tab. + new TestInterstitialPage(contents, false, GURL()); + content::WaitForInterstitialAttach(contents); EXPECT_TRUE(contents->ShowingInterstitialPage()); - { - scoped_refptr loop_runner( - new content::MessageLoopRunner); - - InterstitialObserver observer(contents, - base::Closure(), - loop_runner->QuitClosure()); - chrome::CloseTab(browser()); - loop_runner->Run(); - // interstitial is deleted now. - } + // Close the tab and wait for interstitial detach. This destroys |contents|. + content::RunTaskAndWaitForInterstitialDetach( + contents, base::Bind(&chrome::CloseTab, browser())); + // interstitial is deleted now. } class MockWebContentsObserver : public WebContentsObserver { diff --git a/chrome/browser/ui/browser_focus_uitest.cc b/chrome/browser/ui/browser_focus_uitest.cc index d13fa7b0a3f0..40f349a8257d 100644 --- a/chrome/browser/ui/browser_focus_uitest.cc +++ b/chrome/browser/ui/browser_focus_uitest.cc @@ -136,23 +136,6 @@ class BrowserFocusTest : public InProcessBrowserTest { } }; -// A helper class that waits for an interstitial page to attach. -class WaitForInterstitial : public content::WebContentsObserver { - public: - explicit WaitForInterstitial(content::WebContents* tab) - : WebContentsObserver(tab), - runner_(new content::MessageLoopRunner) { - runner_->Run(); - } - - virtual void DidAttachInterstitialPage() override { runner_->Quit(); } - virtual void DidDetachInterstitialPage() override { NOTREACHED(); } - - private: - scoped_refptr runner_; - DISALLOW_COPY_AND_ASSIGN(WaitForInterstitial); -}; - // A test interstitial page with typical HTML contents. class TestInterstitialPage : public content::InterstitialPageDelegate { public: @@ -168,7 +151,8 @@ class TestInterstitialPage : public content::InterstitialPageDelegate { // Show the interstitial and delay return until it has attached. interstitial_page_->Show(); - WaitForInterstitial wait(tab); + content::WaitForInterstitialAttach(tab); + EXPECT_TRUE(tab->ShowingInterstitialPage()); } diff --git a/chrome/browser/ui/login/login_prompt_browsertest.cc b/chrome/browser/ui/login/login_prompt_browsertest.cc index c32065395c11..8ef34515cd0d 100644 --- a/chrome/browser/ui/login/login_prompt_browsertest.cc +++ b/chrome/browser/ui/login/login_prompt_browsertest.cc @@ -86,42 +86,6 @@ void LoginPromptBrowserTest::SetAuthFor(LoginHandler* handler) { } } -class InterstitialObserver : public content::WebContentsObserver { - public: - InterstitialObserver(content::WebContents* web_contents, - const base::Closure& attach_callback, - const base::Closure& detach_callback) - : WebContentsObserver(web_contents), - attach_callback_(attach_callback), - detach_callback_(detach_callback) { - } - - virtual void DidAttachInterstitialPage() override { - attach_callback_.Run(); - } - - virtual void DidDetachInterstitialPage() override { - detach_callback_.Run(); - } - - private: - base::Closure attach_callback_; - base::Closure detach_callback_; - - DISALLOW_COPY_AND_ASSIGN(InterstitialObserver); -}; - -void WaitForInterstitialAttach(content::WebContents* web_contents) { - scoped_refptr interstitial_attach_loop_runner( - new content::MessageLoopRunner); - InterstitialObserver observer( - web_contents, - interstitial_attach_loop_runner->QuitClosure(), - base::Closure()); - if (!content::InterstitialPage::GetInterstitialPage(web_contents)) - interstitial_attach_loop_runner->Run(); -} - const char kPrefetchAuthPage[] = "files/login/prefetch.html"; const char kMultiRealmTestPage[] = "files/login/multi_realm.html"; @@ -1202,7 +1166,7 @@ void LoginPromptBrowserTest::TestCrossOriginPrompt( ASSERT_EQ(visit_url.host(), contents->GetVisibleURL().host()); auth_needed_waiter.Wait(); ASSERT_EQ(1u, observer.handlers().size()); - WaitForInterstitialAttach(contents); + content::WaitForInterstitialAttach(contents); // The omnibox should show the correct origin for the new page when the // login prompt is shown. @@ -1211,14 +1175,9 @@ void LoginPromptBrowserTest::TestCrossOriginPrompt( // Cancel and wait for the interstitial to detach. LoginHandler* handler = *observer.handlers().begin(); - scoped_refptr loop_runner( - new content::MessageLoopRunner); - InterstitialObserver interstitial_observer(contents, - base::Closure(), - loop_runner->QuitClosure()); - handler->CancelAuth(); - if (content::InterstitialPage::GetInterstitialPage(contents)) - loop_runner->Run(); + content::RunTaskAndWaitForInterstitialDetach( + contents, base::Bind(&LoginHandler::CancelAuth, handler)); + EXPECT_EQ(auth_host, contents->GetVisibleURL().host()); EXPECT_FALSE(contents->ShowingInterstitialPage()); } @@ -1277,14 +1236,14 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, test_page, Referrer(), CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false)); ASSERT_EQ("127.0.0.1", contents->GetURL().host()); - WaitForInterstitialAttach(contents); + content::WaitForInterstitialAttach(contents); // An overrideable SSL interstitial is now being displayed. Proceed through // the interstitial to see the login prompt. contents->GetInterstitialPage()->Proceed(); auth_needed_waiter.Wait(); ASSERT_EQ(1u, observer.handlers().size()); - WaitForInterstitialAttach(contents); + content::WaitForInterstitialAttach(contents); // The omnibox should show the correct origin while the login prompt is // being displayed. @@ -1294,14 +1253,9 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, // Cancelling the login prompt should detach the interstitial while keeping // the correct origin. LoginHandler* handler = *observer.handlers().begin(); - scoped_refptr loop_runner( - new content::MessageLoopRunner); - InterstitialObserver interstitial_observer(contents, - base::Closure(), - loop_runner->QuitClosure()); - handler->CancelAuth(); - if (content::InterstitialPage::GetInterstitialPage(contents)) - loop_runner->Run(); + content::RunTaskAndWaitForInterstitialDetach( + contents, base::Bind(&LoginHandler::CancelAuth, handler)); + EXPECT_EQ("127.0.0.1", contents->GetVisibleURL().host()); EXPECT_FALSE(contents->ShowingInterstitialPage()); } diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc index c261a1e501e9..1017c75f7774 100644 --- a/content/public/test/browser_test_utils.cc +++ b/content/public/test/browser_test_utils.cc @@ -67,7 +67,7 @@ class DOMOperationObserver : public NotificationObserver, virtual void Observe(int type, const NotificationSource& source, - const NotificationDetails& details) OVERRIDE { + const NotificationDetails& details) override { DCHECK(type == NOTIFICATION_DOM_OPERATION_RESPONSE); Details dom_op_details(details); response_ = dom_op_details->json; @@ -76,7 +76,7 @@ class DOMOperationObserver : public NotificationObserver, } // Overridden from WebContentsObserver: - virtual void RenderProcessGone(base::TerminationStatus status) OVERRIDE { + virtual void RenderProcessGone(base::TerminationStatus status) override { message_loop_runner_->Quit(); } @@ -95,6 +95,32 @@ class DOMOperationObserver : public NotificationObserver, DISALLOW_COPY_AND_ASSIGN(DOMOperationObserver); }; +class InterstitialObserver : public content::WebContentsObserver { + public: + InterstitialObserver(content::WebContents* web_contents, + const base::Closure& attach_callback, + const base::Closure& detach_callback) + : WebContentsObserver(web_contents), + attach_callback_(attach_callback), + detach_callback_(detach_callback) { + } + virtual ~InterstitialObserver() {} + + // WebContentsObserver methods: + virtual void DidAttachInterstitialPage() OVERRIDE { + attach_callback_.Run(); + } + virtual void DidDetachInterstitialPage() OVERRIDE { + detach_callback_.Run(); + } + + private: + base::Closure attach_callback_; + base::Closure detach_callback_; + + DISALLOW_COPY_AND_ASSIGN(InterstitialObserver); +}; + // Specifying a prototype so that we can add the WARN_UNUSED_RESULT attribute. bool ExecuteScriptHelper( RenderFrameHost* render_frame_host, @@ -636,6 +662,36 @@ void SetupCrossSiteRedirector( embedded_test_server->base_url())); } +void WaitForInterstitialAttach(content::WebContents* web_contents) { + if (web_contents->ShowingInterstitialPage()) + return; + scoped_refptr loop_runner( + new content::MessageLoopRunner); + InterstitialObserver observer(web_contents, + loop_runner->QuitClosure(), + base::Closure()); + loop_runner->Run(); +} + +void WaitForInterstitialDetach(content::WebContents* web_contents) { + RunTaskAndWaitForInterstitialDetach(web_contents, base::Closure()); +} + +void RunTaskAndWaitForInterstitialDetach(content::WebContents* web_contents, + const base::Closure& task) { + if (!web_contents || !web_contents->ShowingInterstitialPage()) + return; + scoped_refptr loop_runner( + new content::MessageLoopRunner); + InterstitialObserver observer(web_contents, + base::Closure(), + loop_runner->QuitClosure()); + if (!task.is_null()) + task.Run(); + // At this point, web_contents may have been deleted. + loop_runner->Run(); +} + TitleWatcher::TitleWatcher(WebContents* web_contents, const base::string16& expected_title) : WebContentsObserver(web_contents), diff --git a/content/public/test/browser_test_utils.h b/content/public/test/browser_test_utils.h index b19ab4977e4e..c6b17cd90d9a 100644 --- a/content/public/test/browser_test_utils.h +++ b/content/public/test/browser_test_utils.h @@ -206,6 +206,19 @@ void FetchHistogramsFromChildProcesses(); void SetupCrossSiteRedirector( net::test_server::EmbeddedTestServer* embedded_test_server); +// Waits for an interstitial page to attach to given web contents. +void WaitForInterstitialAttach(content::WebContents* web_contents); + +// Waits for an interstitial page to detach from given web contents. +void WaitForInterstitialDetach(content::WebContents* web_contents); + +// Runs task and waits for an interstitial page to detach from given web +// contents. Prefer this over WaitForInterstitialDetach if web_contents may be +// destroyed by the time WaitForInterstitialDetach is called (e.g. when waiting +// for an interstitial detach after closing a tab). +void RunTaskAndWaitForInterstitialDetach(content::WebContents* web_contents, + const base::Closure& task); + // Watches title changes on a WebContents, blocking until an expected title is // set. class TitleWatcher : public WebContentsObserver { -- 2.11.4.GIT