[GTTF] Various ui tests cleanups:

- unify timeout handling
- remove unneccessary assertions that clutter the code

They were mostly there because earlier the automation
framework was less reliable. Now the same checks (or
equivalent) are always done in the framework itself.

TEST=none
BUG=none

Review URL: http://codereview.chromium.org/1377001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42767 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 63 insertions, 114 deletions

diff --git a/chrome/browser/browser_encoding_uitest.cc b/chrome/browser/browser_encoding_uitest.cc
index c5c3eb7..5ed496c 100644
--- a/chrome/browser/browser_encoding_uitest.cc
+++ b/chrome/browser/browser_encoding_uitest.cc
@@ -92,18 +92,16 @@ TEST_F(BrowserEncodingTest, TestEncodingAliasMapping) {
   };
   const char* const kAliasTestDir = "alias_mapping";
 
+  scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
+  ASSERT_TRUE(tab_proxy.get());
+
   FilePath test_dir_path = FilePath(kTestDir).AppendASCII(kAliasTestDir);
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kEncodingTestDatas); ++i) {
     FilePath test_file_path(test_dir_path);
     test_file_path = test_file_path.AppendASCII(
         kEncodingTestDatas[i].file_name);
-    GURL url =
-        URLRequestMockHTTPJob::GetMockUrl(test_file_path);
 
-    scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
-    ASSERT_TRUE(tab_proxy.get());
-    ASSERT_TRUE(tab_proxy->NavigateToURL(url));
-    WaitUntilTabCount(1);
+    NavigateToURL(URLRequestMockHTTPJob::GetMockUrl(test_file_path));
 
     std::string encoding;
     EXPECT_TRUE(tab_proxy->GetPageCurrentEncoding(&encoding));
@@ -253,7 +251,6 @@ TEST_F(BrowserEncodingTest, TestEncodingAutoDetect) {
     GURL url =
         URLRequestMockHTTPJob::GetMockUrl(test_file_path);
     ASSERT_TRUE(tab->NavigateToURL(url));
-    WaitUntilTabCount(1);
 
     // Disable auto detect if it is on.
     EXPECT_TRUE(
diff --git a/chrome/browser/browser_uitest.cc b/chrome/browser/browser_uitest.cc
index 08e311a..25b8e75 100644
--- a/chrome/browser/browser_uitest.cc
+++ b/chrome/browser/browser_uitest.cc
@@ -25,49 +25,7 @@
 
 namespace {
 
-// Delay to let the browser shut down before trying more brutal methods.
-static const int kWaitForTerminateMsec = 30000;
-
 class BrowserTest : public UITest {
-
- protected:
-  void TerminateBrowser() {
-#if defined(OS_WIN)
-    scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
-    ASSERT_TRUE(browser.get());
-    ASSERT_TRUE(browser->TerminateSession());
-#elif defined(OS_POSIX)
-    // There's nothing to do here if the browser is not running.
-    if (IsBrowserRunning()) {
-      EXPECT_TRUE(automation()->SetFilteredInet(false));
-
-      int window_count = 0;
-      EXPECT_TRUE(automation()->GetBrowserWindowCount(&window_count));
-
-      // Now, drop the automation IPC channel so that the automation provider in
-      // the browser notices and drops its reference to the browser process.
-      automation()->Disconnect();
-
-      EXPECT_EQ(kill(process_, SIGTERM), 0);
-
-      // Wait for the browser process to quit. It should have quit when it got
-      // SIGTERM.
-      int timeout = kWaitForTerminateMsec;
-#ifdef WAIT_FOR_DEBUGGER_ON_OPEN
-      timeout = 500000;
-#endif
-      if (!base::WaitForSingleProcess(process_, timeout)) {
-        // We need to force the browser to quit because it didn't quit fast
-        // enough. Take no chance and kill every chrome processes.
-        CleanupAppProcesses();
-      }
-
-      // Don't forget to close the handle
-      base::CloseProcessHandle(process_);
-      process_ = NULL;
-    }
-#endif  // OS_POSIX
-  }
 };
 
 class VisibleBrowserTest : public UITest {
@@ -84,16 +42,11 @@ TEST_F(BrowserTest, WindowsSessionEnd) {
 // The browser should quit gracefully and quickly if it receives a SIGTERM.
 TEST_F(BrowserTest, PosixSessionEnd) {
 #endif
-#if defined(OS_WIN) || defined(OS_POSIX)
   FilePath test_file(test_data_directory_);
   test_file = test_file.AppendASCII("title1.html");
 
   NavigateToURL(net::FilePathToFileURL(test_file));
-  PlatformThread::Sleep(action_timeout_ms());
-
   TerminateBrowser();
-
-  PlatformThread::Sleep(action_timeout_ms());
   ASSERT_FALSE(IsBrowserRunning());
 
   // Make sure the UMA metrics say we didn't crash.
@@ -117,7 +70,6 @@ TEST_F(BrowserTest, PosixSessionEnd) {
                                         &exited_cleanly));
   ASSERT_TRUE(exited_cleanly);
 }
-#endif  // OS_WIN || OS_POSIX
 
 // Test that scripts can fork a new renderer process for a tab in a particular
 // case (which matches following a link in Gmail).  The script must open a new
@@ -164,7 +116,7 @@ TEST_F(BrowserTest, NullOpenerRedirectForksProcess) {
   ASSERT_TRUE(window->GetTabCount(&new_tab_count));
   ASSERT_EQ(orig_tab_count + 1, new_tab_count);
 }
-#endif
+#endif  // CHROME_V8
 
 // This test fails on ChromeOS (it has never been known to work on it).
 // http://crbug.com/32799
@@ -265,9 +217,6 @@ TEST_F(ShowModalDialogTest, FLAKY_BasicTest) {
 }
 
 class SecurityTest : public UITest {
- protected:
-  static const int kTestIntervalMs = 250;
-  static const int kTestWaitTimeoutMs = 60 * 1000;
 };
 
 TEST_F(SecurityTest, DisallowFileUrlUniversalAccessTest) {
@@ -281,7 +230,7 @@ TEST_F(SecurityTest, DisallowFileUrlUniversalAccessTest) {
   ASSERT_TRUE(tab->NavigateToURL(url));
 
   std::string value = WaitUntilCookieNonEmpty(tab.get(), url,
-        "status", kTestIntervalMs, kTestWaitTimeoutMs);
+        "status", sleep_timeout_ms(), action_max_timeout_ms());
   ASSERT_STREQ("Disallowed", value.c_str());
 }
 
@@ -309,7 +258,7 @@ TEST_F(KioskModeTest, EnableKioskModeTest) {
   ASSERT_TRUE(browser->IsFullscreenBubbleVisible(&is_visible));
   EXPECT_FALSE(is_visible);
 }
-#endif
+#endif  // !defined(OS_MACOSX)
 
 #if defined(OS_WIN)
 // This test verifies that Chrome can be launched with a user-data-dir path
@@ -349,6 +298,6 @@ TEST_F(LaunchBrowserWithNonAsciiUserDatadir, TestNonAsciiUserDataDir) {
   scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
   ASSERT_TRUE(browser.get());
 }
-#endif
+#endif  // defined(OS_WIN)
 
 }  // namespace
diff --git a/chrome/browser/dom_ui/new_tab_ui_uitest.cc b/chrome/browser/dom_ui/new_tab_ui_uitest.cc
index 21d7a20..7d7531e 100644
--- a/chrome/browser/dom_ui/new_tab_ui_uitest.cc
+++ b/chrome/browser/dom_ui/new_tab_ui_uitest.cc
@@ -33,14 +33,8 @@ TEST_F(NewTabUITest, NTPHasThumbnails) {
   scoped_refptr<BrowserProxy> window(automation()->GetBrowserWindow(0));
   ASSERT_TRUE(window.get());
 
-  int tab_count = -1;
-  ASSERT_TRUE(window->GetTabCount(&tab_count));
-  ASSERT_EQ(1, tab_count);
-
   // Bring up a new tab page.
   ASSERT_TRUE(window->RunCommand(IDC_NEW_TAB));
-  ASSERT_TRUE(window->GetTabCount(&tab_count));
-  ASSERT_EQ(2, tab_count);
   int load_time;
   ASSERT_TRUE(automation()->WaitForInitialNewTabUILoad(&load_time));
 
@@ -50,7 +44,6 @@ TEST_F(NewTabUITest, NTPHasThumbnails) {
   ASSERT_TRUE(tab.get());
 
   int filler_thumbnails_count = -1;
-  const int kWaitDuration = 100;
   int wait_time = action_max_timeout_ms();
   while (wait_time > 0) {
     ASSERT_TRUE(tab->ExecuteAndExtractInt(L"",
@@ -59,8 +52,8 @@ TEST_F(NewTabUITest, NTPHasThumbnails) {
         &filler_thumbnails_count));
     if (filler_thumbnails_count == 0)
       break;
-    PlatformThread::Sleep(kWaitDuration);
-    wait_time -= kWaitDuration;
+    PlatformThread::Sleep(sleep_timeout_ms());
+    wait_time -= sleep_timeout_ms();
   }
   EXPECT_EQ(0, filler_thumbnails_count);
 }
@@ -69,10 +62,6 @@ TEST_F(NewTabUITest, ChromeInternalLoadsNTP) {
   scoped_refptr<BrowserProxy> window(automation()->GetBrowserWindow(0));
   ASSERT_TRUE(window.get());
 
-  int tab_count = -1;
-  ASSERT_TRUE(window->GetTabCount(&tab_count));
-  ASSERT_EQ(1, tab_count);
-
   // Go to the "new tab page" using its old url, rather than chrome://newtab.
   scoped_refptr<TabProxy> tab = window->GetTab(0);
   ASSERT_TRUE(tab.get());
@@ -118,14 +107,8 @@ TEST_F(NewTabUITest, HomePageLink) {
   ASSERT_TRUE(
       browser->SetBooleanPreference(prefs::kHomePageIsNewTabPage, false));
 
-  int tab_count = -1;
-  ASSERT_TRUE(browser->GetTabCount(&tab_count));
-  ASSERT_EQ(1, tab_count);
-
   // Bring up a new tab page.
   ASSERT_TRUE(browser->RunCommand(IDC_NEW_TAB));
-  ASSERT_TRUE(browser->GetTabCount(&tab_count));
-  ASSERT_EQ(2, tab_count);
   int load_time;
   ASSERT_TRUE(automation()->WaitForInitialNewTabUILoad(&load_time));
 
diff --git a/chrome/browser/errorpage_uitest.cc b/chrome/browser/errorpage_uitest.cc
index 6c1f34f..efd7135 100644
--- a/chrome/browser/errorpage_uitest.cc
+++ b/chrome/browser/errorpage_uitest.cc
@@ -12,10 +12,10 @@
 class ErrorPageTest : public UITest {
  protected:
   bool WaitForTitleMatching(const std::wstring& title) {
-    for (int i = 0; i < 100; ++i) {
+    for (int i = 0; i < 10; ++i) {
       if (GetActiveTabTitle() == title)
         return true;
-      PlatformThread::Sleep(sleep_timeout_ms() / 10);
+      PlatformThread::Sleep(sleep_timeout_ms());
     }
     EXPECT_EQ(title, GetActiveTabTitle());
     return false;
diff --git a/chrome/browser/extensions/extension_uitest.cc b/chrome/browser/extensions/extension_uitest.cc
index b2dea64..ebfef1f 100644
--- a/chrome/browser/extensions/extension_uitest.cc
+++ b/chrome/browser/extensions/extension_uitest.cc
@@ -283,7 +283,7 @@ TEST_F(ExtensionTestRoundtripApiCall, RunTest) {
 
   // CheckAndSendResponse (called by OnForwardMessageToExternalHost)
   // will end the loop once it has received both of our expected messages.
-  loop_.RunFor(2 * action_max_timeout_ms());
+  loop_.RunFor(action_max_timeout_ms());
 }
 
 class ExtensionTestBrowserEvents : public ExtensionUITest {
@@ -470,7 +470,7 @@ TEST_F(ExtensionTestBrowserEvents, RunTest) {
 
   // HandleMessageFromChrome (called by OnForwardMessageToExternalHost) ends
   // the loop when we've received the number of response messages we expect.
-  loop_.RunFor(2 * action_max_timeout_ms());
+  loop_.RunFor(action_max_timeout_ms());
 
   // If this assert hits and the actual size is 0 then you need to look at:
   // src\chrome\test\data\extensions\uitest\event_sink\test.html and see if
diff --git a/chrome/browser/history/redirect_uitest.cc b/chrome/browser/history/redirect_uitest.cc
index 976d91b..c2df04f 100644
--- a/chrome/browser/history/redirect_uitest.cc
+++ b/chrome/browser/history/redirect_uitest.cc
@@ -20,11 +20,7 @@ namespace {
 
 const wchar_t kDocRoot[] = L"chrome/test/data";
 
-class RedirectTest : public UITest {
- protected:
-  RedirectTest() : UITest() {
-  }
-};
+typedef UITest RedirectTest;
 
 // Tests a single server redirect
 TEST_F(RedirectTest, Server) {
diff --git a/chrome/browser/iframe_uitest.cc b/chrome/browser/iframe_uitest.cc
index 15a1b11..d255344 100644
--- a/chrome/browser/iframe_uitest.cc
+++ b/chrome/browser/iframe_uitest.cc
@@ -4,7 +4,6 @@
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
-#include "base/platform_thread.h"
 #include "build/build_config.h"
 #include "chrome/test/ui/ui_test.h"
 #include "net/base/net_util.h"
@@ -16,13 +15,7 @@ class IFrameTest : public UITest {
     test_file = test_file.AppendASCII(url);
 
     NavigateToURL(net::FilePathToFileURL(test_file));
-    // The browser lazily updates the title.
-    PlatformThread::Sleep(sleep_timeout_ms());
-
-    // Make sure the navigation succeeded.
     EXPECT_EQ(std::wstring(page_title), GetActiveTabTitle());
-
-    // UITest will check if this crashed.
   }
 };
 
diff --git a/chrome/browser/images_uitest.cc b/chrome/browser/images_uitest.cc
index 054ca6e..2c67437 100644
--- a/chrome/browser/images_uitest.cc
+++ b/chrome/browser/images_uitest.cc
@@ -5,30 +5,21 @@
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/platform_thread.h"
+#include "net/base/net_util.h"
 #include "chrome/test/ui/ui_test.h"
 
-class ImagesTest : public UITest {
- protected:
-  ImagesTest() : UITest() {
-    FilePath path(test_data_directory_);
-    path = path.AppendASCII("animated-gifs.html");
-    launch_arguments_ = CommandLine(CommandLine::ARGUMENTS_ONLY);
-    launch_arguments_.AppendLooseValue(path.ToWStringHack());
-  }
-};
+typedef UITest ImagesTest;
 
 TEST_F(ImagesTest, AnimatedGIFs) {
-  std::wstring page_title = L"animated gif test";
+  FilePath test_file(test_data_directory_);
+  test_file = test_file.AppendASCII("animated-gifs.html");
+  NavigateToURL(net::FilePathToFileURL(test_file));
 
   // Let the GIFs fully animate.
-  for (int i = 0; i < 10; ++i) {
-    PlatformThread::Sleep(sleep_timeout_ms());
-    if (page_title == GetActiveTabTitle())
-      break;
-  }
+  PlatformThread::Sleep(sleep_timeout_ms());
 
-  // Make sure the navigation succeeded.
+  std::wstring page_title = L"animated gif test";
   EXPECT_EQ(page_title, GetActiveTabTitle());
 
-  // Tau will check if this crashed.
+  // UI test framework will check if this crashed.
 }
diff --git a/chrome/test/ui/ui_test.cc b/chrome/test/ui/ui_test.cc
index b4ae546..f41c1dd 100644
--- a/chrome/test/ui/ui_test.cc
+++ b/chrome/test/ui/ui_test.cc
@@ -467,6 +467,43 @@ void UITestBase::QuitBrowser() {
   process_id_ = -1;
 }
 
+void UITestBase::TerminateBrowser() {
+  // There's nothing to do here if the browser is not running.
+  if (!IsBrowserRunning())
+    return;
+
+  EXPECT_TRUE(automation()->SetFilteredInet(false));
+#if defined(OS_WIN)
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  ASSERT_TRUE(browser->TerminateSession());
+#endif  // defined(OS_WIN)
+
+  // Now, drop the automation IPC channel so that the automation provider in
+  // the browser notices and drops its reference to the browser process.
+  automation()->Disconnect();
+
+#if defined(OS_POSIX)
+  EXPECT_EQ(kill(process_, SIGTERM), 0);
+#endif  // OS_POSIX
+
+  // Wait for the browser process to quit.
+  int timeout = terminate_timeout_ms_;
+#ifdef WAIT_FOR_DEBUGGER_ON_OPEN
+  timeout = 500000;
+#endif
+  if (!base::WaitForSingleProcess(process_, timeout)) {
+    // We need to force the browser to quit because it didn't quit fast
+    // enough. Take no chance and kill every chrome processes.
+    CleanupAppProcesses();
+  }
+
+  // Don't forget to close the handle
+  base::CloseProcessHandle(process_);
+  process_ = base::kNullProcessHandle;
+  process_id_ = -1;
+}
+
 void UITestBase::AssertAppNotRunning(const std::wstring& error_message) {
   std::wstring final_error_message(error_message);
 
diff --git a/chrome/test/ui/ui_test.h b/chrome/test/ui/ui_test.h
index 8405028..ab5d2d8 100644
--- a/chrome/test/ui/ui_test.h
+++ b/chrome/test/ui/ui_test.h
@@ -91,6 +91,9 @@ class UITestBase {
   // Exits out browser instance.
   void QuitBrowser();
 
+  // Terminates the browser, simulates end of session.
+  void TerminateBrowser();
+
   // Tells the browser to navigato to the givne URL in the active tab
   // of the first app window.
   // Does not wait for the navigation to complete to return.