[GTTF] Miscellanous UI tests cleanups:

- use built-in timeouts instead of "inventing" them in each test case
- avoid unneeded checks and operations
- use automation calls more effectively
- use FLAKY mark instead of DISABLED to maintain test coverage
- split some tests to make the above possible

TEST=UI test based
BUG=39785

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

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

9 files changed, 50 insertions, 70 deletions

diff --git a/chrome/browser/metrics/metrics_service_uitest.cc b/chrome/browser/metrics/metrics_service_uitest.cc
index 7465e17..5dfe9d1 100644
--- a/chrome/browser/metrics/metrics_service_uitest.cc
+++ b/chrome/browser/metrics/metrics_service_uitest.cc
@@ -102,7 +102,7 @@ TEST_F(MetricsServiceTest, CrashRenderers) {
   }
 
   // Give the browser a chance to notice the crashed tab.
-  PlatformThread::Sleep(1000);
+  PlatformThread::Sleep(sleep_timeout_ms());
 
   QuitBrowser();
 
diff --git a/chrome/browser/printing/printing_layout_uitest.cc b/chrome/browser/printing/printing_layout_uitest.cc
index 5fb1b3f..d205f7b 100644
--- a/chrome/browser/printing/printing_layout_uitest.cc
+++ b/chrome/browser/printing/printing_layout_uitest.cc
@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/simple_thread.h"
+#include "base/test/test_file_util.h"
 #include "base/win_util.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/automation/browser_proxy.h"
@@ -49,16 +50,13 @@ class PrintingLayoutTest : public PrintingTest<UITest> {
   void PrintNowTab() {
     scoped_refptr<TabProxy> tab_proxy(GetActiveTab());
     ASSERT_TRUE(tab_proxy.get());
-    if (!tab_proxy.get())
-      return;
-
     ASSERT_TRUE(tab_proxy->PrintNow());
   }
 
   // Finds the dump for the last print job and compares it to the data named
   // |verification_name|. Compares the saved printed job pixels with the test
   // data pixels and returns the percentage of different pixels; 0 for success,
-  // ]0, 100] for failure.
+  // [0, 100] for failure.
   double CompareWithResult(const std::wstring& verification_name) {
     FilePath test_result(ScanFiles(verification_name));
     if (test_result.value().empty()) {
@@ -118,21 +116,8 @@ class PrintingLayoutTest : public PrintingTest<UITest> {
 
   // Makes sure the directory exists and is empty.
   void CleanupDumpDirectory() {
-    // Tries to delete the dumping directory for around 10 seconds.
-    for (int i = 0; i < 100 && file_util::PathExists(emf_path()); ++i) {
-      // It's fine fail sometimes because of opened left over .PRN file.
-      // Explanation:
-      // When calling PrintNowTab(), it makes sure the page is rendered and
-      // sent to the spooler. It does *not* wait for the spooler to flush the
-      // job. It is completely unnecessary to wait for that. So the printer
-      // may write the file too late. Since the printer holds an exclusive
-      // access to the file, it can't be deleted until the printer is done.
-      if (file_util::Delete(emf_path(), true)) {
-        break;
-      }
-      PlatformThread::Sleep(100);
-    }
-    file_util::CreateDirectory(emf_path());
+    EXPECT_TRUE(file_util::DieFileDie(emf_path(), true));
+    EXPECT_TRUE(file_util::CreateDirectory(emf_path()));
   }
 
   // Returns if Clear Type is currently enabled.
diff --git a/chrome/browser/process_singleton_linux_uitest.cc b/chrome/browser/process_singleton_linux_uitest.cc
index 8bef102..d0ba750 100644
--- a/chrome/browser/process_singleton_linux_uitest.cc
+++ b/chrome/browser/process_singleton_linux_uitest.cc
@@ -32,7 +32,8 @@ typedef UITest ProcessSingletonLinuxTest;
 // A helper method to call ProcessSingleton::NotifyOtherProcess().
 // |url| will be added to CommandLine for current process, so that it can be
 // sent to browser process by ProcessSingleton::NotifyOtherProcess().
-ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url) {
+ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url,
+                                                  int timeout_ms) {
   FilePath user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
 
@@ -49,10 +50,8 @@ ProcessSingleton::NotifyResult NotifyOtherProcess(const std::string& url) {
 
   ProcessSingleton process_singleton(user_data_dir);
 
-  // Use a short timeout to keep tests fast.
-  const int kTimeoutSeconds = 3;
-  return process_singleton.NotifyOtherProcessWithTimeout(new_cmd_line,
-                                                         kTimeoutSeconds);
+  return process_singleton.NotifyOtherProcessWithTimeout(
+      new_cmd_line, timeout_ms / 1000);
 }
 
 }  // namespace
@@ -95,7 +94,8 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessSuccess) {
   std::string url("about:blank");
   int original_tab_count = GetTabCount();
 
-  EXPECT_EQ(ProcessSingleton::PROCESS_NOTIFIED, NotifyOtherProcess(url));
+  EXPECT_EQ(ProcessSingleton::PROCESS_NOTIFIED,
+            NotifyOtherProcess(url, action_timeout_ms()));
   EXPECT_EQ(original_tab_count + 1, GetTabCount());
   EXPECT_EQ(url, GetActiveTabURL().spec());
 }
@@ -115,10 +115,11 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessFailure) {
   HANDLE_EINTR(waitpid(pid, 0, WUNTRACED));
 
   std::string url("about:blank");
-  EXPECT_EQ(ProcessSingleton::PROCESS_NONE, NotifyOtherProcess(url));
+  EXPECT_EQ(ProcessSingleton::PROCESS_NONE,
+            NotifyOtherProcess(url, action_timeout_ms()));
 
   // Wait for a while to make sure the browser process is actually killed.
-  EXPECT_FALSE(CrashAwareSleep(1000));
+  EXPECT_FALSE(CrashAwareSleep(sleep_timeout_ms()));
 }
 
 // Test that we can still notify a process on the same host even after the
@@ -131,7 +132,8 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessHostChanged) {
   int original_tab_count = GetTabCount();
 
   std::string url("about:blank");
-  EXPECT_EQ(ProcessSingleton::PROCESS_NOTIFIED, NotifyOtherProcess(url));
+  EXPECT_EQ(ProcessSingleton::PROCESS_NOTIFIED,
+            NotifyOtherProcess(url, action_timeout_ms()));
   EXPECT_EQ(original_tab_count + 1, GetTabCount());
   EXPECT_EQ(url, GetActiveTabURL().spec());
 }
@@ -146,12 +148,13 @@ TEST_F(ProcessSingletonLinuxTest, NotifyOtherProcessDifferingHost) {
   // Kill the browser process, so that it does not respond on the socket.
   kill(pid, SIGKILL);
   // Wait for a while to make sure the browser process is actually killed.
-  EXPECT_FALSE(CrashAwareSleep(1000));
+  EXPECT_FALSE(CrashAwareSleep(sleep_timeout_ms()));
 
   FilePath lock_path = user_data_dir().Append(chrome::kSingletonLockFilename);
   EXPECT_EQ(0, unlink(lock_path.value().c_str()));
   EXPECT_EQ(0, symlink("FAKEFOOHOST-1234", lock_path.value().c_str()));
 
   std::string url("about:blank");
-  EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE, NotifyOtherProcess(url));
+  EXPECT_EQ(ProcessSingleton::PROFILE_IN_USE,
+            NotifyOtherProcess(url, action_timeout_ms()));
 }
diff --git a/chrome/browser/process_singleton_win_uitest.cc b/chrome/browser/process_singleton_win_uitest.cc
index 7611a83..929e271b2 100644
--- a/chrome/browser/process_singleton_win_uitest.cc
+++ b/chrome/browser/process_singleton_win_uitest.cc
@@ -33,11 +33,12 @@ namespace {
 // NewRunnableMethod class to run the StartChrome methods in many threads.
 class ChromeStarter : public base::RefCountedThreadSafe<ChromeStarter> {
  public:
-  ChromeStarter()
+  explicit ChromeStarter(int timeout_ms)
       : ready_event_(false /* manual */, false /* signaled */),
         done_event_(false /* manual */, false /* signaled */),
         process_handle_(NULL),
-        process_terminated_(false) {
+        process_terminated_(false),
+        timeout_ms_(timeout_ms) {
   }
 
   // We must reset some data members since we reuse the same ChromeStarter
@@ -75,9 +76,8 @@ class ChromeStarter : public base::RefCountedThreadSafe<ChromeStarter> {
     // We can wait on the handle here, we should get stuck on one and only
     // one process. The test below will take care of killing that process
     // to unstuck us once it confirms there is only one.
-    static const int64 kWaitForProcessDeath = 5000;
     process_terminated_ = base::WaitForSingleProcess(process_handle_,
-                                                     kWaitForProcessDeath);
+                                                     timeout_ms_);
     // Let the test know we are done.
     done_event_.Signal();
   }
@@ -90,10 +90,14 @@ class ChromeStarter : public base::RefCountedThreadSafe<ChromeStarter> {
 
  private:
   friend class base::RefCountedThreadSafe<ChromeStarter>;
+
   ~ChromeStarter() {
     if (process_handle_ != NULL)
       base::CloseProcessHandle(process_handle_);
   }
+
+  int timeout_ms_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeStarter);
 };
 
@@ -111,7 +115,7 @@ class ProcessSingletonWinTest : public UITest {
     for (size_t i = 0; i < kNbThreads; ++i) {
       chrome_starter_threads_[i].reset(new base::Thread("ChromeStarter"));
       ASSERT_TRUE(chrome_starter_threads_[i]->Start());
-      chrome_starters_[i] = new ChromeStarter;
+      chrome_starters_[i] = new ChromeStarter(action_max_timeout_ms());
     }
   }
 
diff --git a/chrome/browser/repost_form_warning_uitest.cc b/chrome/browser/repost_form_warning_uitest.cc
index 755ae85..b17e588 100644
--- a/chrome/browser/repost_form_warning_uitest.cc
+++ b/chrome/browser/repost_form_warning_uitest.cc
@@ -13,26 +13,23 @@
 #include "chrome/test/ui/ui_test.h"
 #include "net/url_request/url_request_unittest.h"
 
-using std::wstring;
-
 namespace {
 
 const wchar_t kDocRoot[] = L"chrome/test/data";
 
 }  // namespace
 
-class RepostFormWarningTest : public UITest {
-};
-
+typedef UITest RepostFormWarningTest;
 
 TEST_F(RepostFormWarningTest, TestDoubleReload) {
   scoped_refptr<HTTPTestServer> server =
       HTTPTestServer::CreateServer(kDocRoot, NULL);
   ASSERT_TRUE(NULL != server.get());
   scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
-  EXPECT_TRUE(browser.get());
+  ASSERT_TRUE(browser.get());
 
   scoped_refptr<TabProxy> tab(browser->GetTab(0));
+  ASSERT_TRUE(tab.get());
 
   // Load a form.
   ASSERT_TRUE(tab->NavigateToURL(server->TestServerPageW(L"files/form.html")));
@@ -56,6 +53,7 @@ TEST_F(RepostFormWarningTest, TestLoginAfterRepost) {
   ASSERT_TRUE(browser.get());
 
   scoped_refptr<TabProxy> tab(browser->GetTab(0));
+  ASSERT_TRUE(tab.get());
 
   // Load a form.
   ASSERT_TRUE(tab->NavigateToURL(server->TestServerPageW(L"files/form.html")));
diff --git a/chrome/browser/session_history_uitest.cc b/chrome/browser/session_history_uitest.cc
index c293a9d..830f9be 100644
--- a/chrome/browser/session_history_uitest.cc
+++ b/chrome/browser/session_history_uitest.cc
@@ -14,8 +14,6 @@
 #include "net/base/net_util.h"
 #include "net/url_request/url_request_unittest.h"
 
-using std::wstring;
-
 namespace {
 
 const wchar_t kDocRoot[] = L"chrome/test/data";
@@ -32,9 +30,7 @@ class SessionHistoryTest : public UITest {
     window_ = automation()->GetBrowserWindow(0);
     ASSERT_TRUE(window_.get());
 
-    int active_tab_index = -1;
-    ASSERT_TRUE(window_->GetActiveTabIndex(&active_tab_index));
-    tab_ = window_->GetTab(active_tab_index);
+    tab_ = window_->GetActiveTab();
     ASSERT_TRUE(tab_.get());
   }
 
@@ -67,8 +63,8 @@ class SessionHistoryTest : public UITest {
     ASSERT_TRUE(tab_->NavigateToURL(url));
   }
 
-  wstring GetTabTitle() {
-    wstring title;
+  std::wstring GetTabTitle() {
+    std::wstring title;
     EXPECT_TRUE(tab_->GetTabTitle(&title));
     return title;
   }
diff --git a/chrome/browser/sessions/session_restore_uitest.cc b/chrome/browser/sessions/session_restore_uitest.cc
index aefa6aa..fe4ffeb 100644
--- a/chrome/browser/sessions/session_restore_uitest.cc
+++ b/chrome/browser/sessions/session_restore_uitest.cc
@@ -73,7 +73,7 @@ class SessionRestoreUITest : public UITest {
 
     scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetActiveTab());
     ASSERT_TRUE(tab_proxy.get());
-    ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+    ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
     ASSERT_TRUE(tab_proxy->GetCurrentURL(url));
   }
@@ -100,7 +100,7 @@ TEST_F(SessionRestoreUITest, Basic) {
   scoped_refptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
   ASSERT_TRUE(browser_proxy.get());
   scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetTab(0));
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
   ASSERT_EQ(url2_, GetActiveTabURL());
   ASSERT_EQ(AUTOMATION_MSG_NAVIGATION_SUCCESS, tab_proxy->GoBack());
@@ -126,7 +126,7 @@ TEST_F(SessionRestoreUITest, RestoresForwardAndBackwardNavs) {
   scoped_refptr<BrowserProxy> browser_proxy(automation()->GetBrowserWindow(0));
   ASSERT_TRUE(browser_proxy.get());
   scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetTab(0));
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
   ASSERT_TRUE(GetActiveTabURL() == url2_);
   ASSERT_TRUE(tab_proxy->GoForward());
@@ -220,16 +220,16 @@ TEST_F(SessionRestoreUITest, TwoTabsSecondSelected) {
 
   scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetActiveTab());
   ASSERT_TRUE(tab_proxy.get());
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
-  ASSERT_TRUE(GetActiveTabURL() == url2_);
+  ASSERT_EQ(url2_, GetActiveTabURL());
 
   ASSERT_TRUE(browser_proxy->ActivateTab(0));
   tab_proxy = browser_proxy->GetActiveTab();
   ASSERT_TRUE(tab_proxy.get());
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
-  ASSERT_TRUE(GetActiveTabURL() == url1_);
+  ASSERT_EQ(url1_, GetActiveTabURL());
 }
 
 // Creates two tabs, closes one, quits and makes sure only one tab is restored.
@@ -258,7 +258,7 @@ TEST_F(SessionRestoreUITest, ClosedTabStaysClosed) {
 
   AssertOneWindowWithOneTab();
 
-  ASSERT_TRUE(GetActiveTabURL() == url1_);
+  ASSERT_EQ(url1_, GetActiveTabURL());
 }
 
 // Creates a tabbed browser and popup and makes sure we restore both.
@@ -324,7 +324,6 @@ TEST_F(SessionRestoreUITest, NormalAndPopup) {
   }
 }
 
-#if defined(OS_WIN)
 // Creates a browser, goes incognito, closes browser, launches and make sure
 // we don't restore.
 //
@@ -366,7 +365,7 @@ TEST_F(SessionRestoreUITest, FLAKY_DontRestoreWhileIncognito) {
   ASSERT_TRUE(browser_proxy.get());
   scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetTab(0));
   ASSERT_TRUE(tab_proxy.get());
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
   GURL url;
   ASSERT_TRUE(tab_proxy->GetCurrentURL(&url));
   ASSERT_TRUE(url != url1_);
@@ -396,7 +395,7 @@ TEST_F(SessionRestoreUITest, TwoWindowsCloseOneRestoreOnlyOne) {
 
   AssertOneWindowWithOneTab();
 
-  ASSERT_TRUE(GetActiveTabURL() == url1_);
+  ASSERT_EQ(url1_, GetActiveTabURL());
 }
 
 // Launches an app window, closes tabbed browser, launches and makes sure
@@ -430,7 +429,7 @@ TEST_F(SessionRestoreUITest,
 
   AssertOneWindowWithOneTab();
 
-  ASSERT_TRUE(GetActiveTabURL() == url1_);
+  ASSERT_EQ(url1_, GetActiveTabURL());
 }
 
 // Make sure after a restore the number of processes matches that of the number
@@ -473,13 +472,12 @@ TEST_F(SessionRestoreUITest, ShareProcessesOnRestore) {
 
   scoped_refptr<TabProxy> tab_proxy(browser_proxy->GetTab(tab_count - 2));
   ASSERT_TRUE(tab_proxy.get() != NULL);
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
   tab_proxy = browser_proxy->GetTab(tab_count - 1);
   ASSERT_TRUE(tab_proxy.get() != NULL);
-  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_timeout_ms()));
+  ASSERT_TRUE(tab_proxy->WaitForTabToBeRestored(action_max_timeout_ms()));
 
   ASSERT_EQ(expected_process_count, GetBrowserProcessCount());
 }
-#endif
 
 }  // namespace
diff --git a/chrome/browser/tab_restore_uitest.cc b/chrome/browser/tab_restore_uitest.cc
index 56ff39f..ebc6f09 100644
--- a/chrome/browser/tab_restore_uitest.cc
+++ b/chrome/browser/tab_restore_uitest.cc
@@ -98,14 +98,10 @@ class TabRestoreUITest : public UITest {
   // the final number of tabs.
   int AddSomeTabs(BrowserProxy* browser, int how_many) {
     int starting_tab_count = -1;
-    // Use EXPECT instead of ASSERT throughout to avoid trying to return void.
     EXPECT_TRUE(browser->GetTabCount(&starting_tab_count));
 
     for (int i = 0; i < how_many; ++i) {
       EXPECT_TRUE(browser->AppendTab(url1_));
-      int current_tab_count;
-      EXPECT_TRUE(browser->GetTabCount(&current_tab_count));
-      EXPECT_EQ(starting_tab_count + i + 1, current_tab_count);
     }
     int tab_count;
     EXPECT_TRUE(browser->GetTabCount(&tab_count));
diff --git a/chrome/browser/unload_uitest.cc b/chrome/browser/unload_uitest.cc
index 5ee30b7..62072f9 100644
--- a/chrome/browser/unload_uitest.cc
+++ b/chrome/browser/unload_uitest.cc
@@ -105,7 +105,7 @@ class UnloadTest : public UITest {
 
   void WaitForBrowserClosed() {
     const int kCheckDelayMs = 100;
-    int max_wait_time = 5000;
+    int max_wait_time = action_max_timeout_ms();
     while (max_wait_time > 0) {
       max_wait_time -= kCheckDelayMs;
       PlatformThread::Sleep(kCheckDelayMs);
@@ -116,7 +116,7 @@ class UnloadTest : public UITest {
 
   void CheckTitle(const std::wstring& expected_title) {
     const int kCheckDelayMs = 100;
-    int max_wait_time = 5000;
+    int max_wait_time = action_max_timeout_ms();
     while (max_wait_time > 0) {
       max_wait_time -= kCheckDelayMs;
       PlatformThread::Sleep(kCheckDelayMs);