Refactor remote_desktop_browsertest:

1. Adding some helpers to perform common composite tasks.
2. Reorder methods to comply to chromium coding styles.
3. Move some functions defined inline to the .cc file.
4. Better comments and grouping of the class methods.

BUG=134210

Review URL: https://chromiumcodereview.appspot.com/22887025

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

4 files changed, 219 insertions, 168 deletions

diff --git a/remoting/test/auth_browsertest.cc b/remoting/test/auth_browsertest.cc
index b38caf9..fcb3987 100644
--- a/remoting/test/auth_browsertest.cc
+++ b/remoting/test/auth_browsertest.cc
@@ -9,31 +9,14 @@ namespace remoting {
 IN_PROC_BROWSER_TEST_F(RemoteDesktopBrowserTest, MANUAL_Auth) {
   VerifyInternetAccess();
 
-  if (!NoInstall()) {
-    VerifyChromotingLoaded(false);
-    InstallChromotingApp();
-  }
-
-  VerifyChromotingLoaded(true);
+  Install();
 
   LaunchChromotingApp();
 
-  Authorize();
-
-  Authenticate();
-
-  Approve();
-
-  // TODO: Remove this hack by blocking on the appropriate notification.
-  // The browser may still be loading images embedded in the webapp. If we
-  // uinstall it now those load will fail. Navigating away to avoid the load
-  // failures.
-  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
+  // Authorize, Authenticate, and Approve.
+  Auth();
 
-  if (!NoCleanup()) {
-    UninstallChromotingApp();
-    VerifyChromotingLoaded(false);
-  }
+  Cleanup();
 }
 
 }  // namespace remoting
diff --git a/remoting/test/launch_browsertest.cc b/remoting/test/launch_browsertest.cc
index d660119..4c9804e 100644
--- a/remoting/test/launch_browsertest.cc
+++ b/remoting/test/launch_browsertest.cc
@@ -9,25 +9,11 @@ namespace remoting {
 IN_PROC_BROWSER_TEST_F(RemoteDesktopBrowserTest, MANUAL_Launch) {
   VerifyInternetAccess();
 
-  if (!NoInstall()) {
-    VerifyChromotingLoaded(false);
-    InstallChromotingApp();
-  }
-
-  VerifyChromotingLoaded(true);
+  Install();
 
   LaunchChromotingApp();
 
-  // TODO: Remove this hack by blocking on the appropriate notification.
-  // The browser may still be loading images embedded in the webapp. If we
-  // uinstall it now those load will fail. Navigating away to avoid the load
-  // failures.
-  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
-
-  if (!NoCleanup()) {
-    UninstallChromotingApp();
-    VerifyChromotingLoaded(false);
-  }
+  Cleanup();
 }
 
 }  // namespace remoting
diff --git a/remoting/test/remote_desktop_browsertest.cc b/remoting/test/remote_desktop_browsertest.cc
index c8d8bf7..8d91a06 100644
--- a/remoting/test/remote_desktop_browsertest.cc
+++ b/remoting/test/remote_desktop_browsertest.cc
@@ -36,59 +36,6 @@ void RemoteDesktopBrowserTest::TearDownInProcessBrowserTestFixture() {
   DisableDNSLookupForThisTest();
 }
 
-void RemoteDesktopBrowserTest::ParseCommandLine() {
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-
-  // The test framework overrides any command line user-data-dir
-  // argument with a /tmp/.org.chromium.Chromium.XXXXXX directory.
-  // That happens in the ChromeTestLauncherDelegate, and affects
-  // all unit tests (no opt out available). It intentionally erases
-  // any --user-data-dir switch if present and appends a new one.
-  // Re-override the default data dir if override-user-data-dir
-  // is specified.
-  if (command_line->HasSwitch(kOverrideUserDataDir)) {
-    const base::FilePath& override_user_data_dir =
-        command_line->GetSwitchValuePath(kOverrideUserDataDir);
-
-    ASSERT_FALSE(override_user_data_dir.empty());
-
-    command_line->AppendSwitchPath(switches::kUserDataDir,
-                                   override_user_data_dir);
-  }
-
-  username_ = command_line->GetSwitchValueASCII(kUsername);
-  password_ = command_line->GetSwitchValueASCII(kkPassword);
-
-  no_cleanup_ = command_line->HasSwitch(kNoCleanup);
-  no_install_ = command_line->HasSwitch(kNoInstall);
-
-  if (!no_install_) {
-    webapp_crx_ = command_line->GetSwitchValuePath(kWebAppCrx);
-    ASSERT_FALSE(webapp_crx_.empty());
-  }
-}
-
-void RemoteDesktopBrowserTest::EnableDNSLookupForThisTest(
-    net::RuleBasedHostResolverProc* host_resolver) {
-  // mock_host_resolver_override_ takes ownership of the resolver.
-  scoped_refptr<net::RuleBasedHostResolverProc> resolver =
-      new net::RuleBasedHostResolverProc(host_resolver);
-  resolver->AllowDirectLookup("*.google.com");
-  // On Linux, we use Chromium's NSS implementation which uses the following
-  // hosts for certificate verification. Without these overrides, running the
-  // integration tests on Linux causes errors as we make external DNS lookups.
-  resolver->AllowDirectLookup("*.thawte.com");
-  resolver->AllowDirectLookup("*.geotrust.com");
-  resolver->AllowDirectLookup("*.gstatic.com");
-  resolver->AllowDirectLookup("*.googleapis.com");
-  mock_host_resolver_override_.reset(
-      new net::ScopedDefaultHostResolverProc(resolver.get()));
-}
-
-void RemoteDesktopBrowserTest::DisableDNSLookupForThisTest() {
-  mock_host_resolver_override_.reset();
-}
-
 void RemoteDesktopBrowserTest::VerifyInternetAccess() {
   GURL google_url("http://www.google.com");
   NavigateToURLAndWait(google_url);
@@ -109,15 +56,6 @@ void RemoteDesktopBrowserTest::UninstallChromotingApp() {
   chromoting_id_.clear();
 }
 
-void RemoteDesktopBrowserTest::LaunchChromotingApp() {
-  ASSERT_FALSE(ChromotingID().empty());
-
-  const GURL chromoting_main = Chromoting_Main_URL();
-  NavigateToURLAndWait(chromoting_main);
-
-  EXPECT_EQ(GetCurrentURL(), chromoting_main);
-}
-
 void RemoteDesktopBrowserTest::VerifyChromotingLoaded(bool expected) {
   const ExtensionSet* extensions = extension_service()->extensions();
   scoped_refptr<const extensions::Extension> extension;
@@ -147,6 +85,15 @@ void RemoteDesktopBrowserTest::VerifyChromotingLoaded(bool expected) {
   EXPECT_EQ(installed, expected);
 }
 
+void RemoteDesktopBrowserTest::LaunchChromotingApp() {
+  ASSERT_FALSE(ChromotingID().empty());
+
+  const GURL chromoting_main = Chromoting_Main_URL();
+  NavigateToURLAndWait(chromoting_main);
+
+  EXPECT_EQ(GetCurrentURL(), chromoting_main);
+}
+
 void RemoteDesktopBrowserTest::Authorize() {
   // The chromoting extension should be installed.
   ASSERT_FALSE(ChromotingID().empty());
@@ -210,4 +157,151 @@ void RemoteDesktopBrowserTest::Approve() {
       "remoting.OAuth2.prototype.isAuthenticated()"));
 }
 
+void RemoteDesktopBrowserTest::Install() {
+  // TODO: add support for installing unpacked extension (the v2 app needs it).
+  if (!NoInstall()) {
+    VerifyChromotingLoaded(false);
+    InstallChromotingApp();
+  }
+
+  VerifyChromotingLoaded(true);
+}
+
+void RemoteDesktopBrowserTest::Cleanup() {
+  // TODO: Remove this hack by blocking on the appropriate notification.
+  // The browser may still be loading images embedded in the webapp. If we
+  // uinstall it now those load will fail. Navigating away to avoid the load
+  // failures.
+  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
+
+  if (!NoCleanup()) {
+    UninstallChromotingApp();
+    VerifyChromotingLoaded(false);
+  }
+}
+
+void RemoteDesktopBrowserTest::Auth() {
+  Authorize();
+  Authenticate();
+  Approve();
+}
+
+void RemoteDesktopBrowserTest::EnableDNSLookupForThisTest(
+    net::RuleBasedHostResolverProc* host_resolver) {
+  // mock_host_resolver_override_ takes ownership of the resolver.
+  scoped_refptr<net::RuleBasedHostResolverProc> resolver =
+      new net::RuleBasedHostResolverProc(host_resolver);
+  resolver->AllowDirectLookup("*.google.com");
+  // On Linux, we use Chromium's NSS implementation which uses the following
+  // hosts for certificate verification. Without these overrides, running the
+  // integration tests on Linux causes errors as we make external DNS lookups.
+  resolver->AllowDirectLookup("*.thawte.com");
+  resolver->AllowDirectLookup("*.geotrust.com");
+  resolver->AllowDirectLookup("*.gstatic.com");
+  resolver->AllowDirectLookup("*.googleapis.com");
+  mock_host_resolver_override_.reset(
+      new net::ScopedDefaultHostResolverProc(resolver.get()));
+}
+
+void RemoteDesktopBrowserTest::DisableDNSLookupForThisTest() {
+  mock_host_resolver_override_.reset();
+}
+
+void RemoteDesktopBrowserTest::ParseCommandLine() {
+  CommandLine* command_line = CommandLine::ForCurrentProcess();
+
+  // The test framework overrides any command line user-data-dir
+  // argument with a /tmp/.org.chromium.Chromium.XXXXXX directory.
+  // That happens in the ChromeTestLauncherDelegate, and affects
+  // all unit tests (no opt out available). It intentionally erases
+  // any --user-data-dir switch if present and appends a new one.
+  // Re-override the default data dir if override-user-data-dir
+  // is specified.
+  if (command_line->HasSwitch(kOverrideUserDataDir)) {
+    const base::FilePath& override_user_data_dir =
+        command_line->GetSwitchValuePath(kOverrideUserDataDir);
+
+    ASSERT_FALSE(override_user_data_dir.empty());
+
+    command_line->AppendSwitchPath(switches::kUserDataDir,
+                                   override_user_data_dir);
+  }
+
+  username_ = command_line->GetSwitchValueASCII(kUsername);
+  password_ = command_line->GetSwitchValueASCII(kkPassword);
+
+  no_cleanup_ = command_line->HasSwitch(kNoCleanup);
+  no_install_ = command_line->HasSwitch(kNoInstall);
+
+  if (!no_install_) {
+    webapp_crx_ = command_line->GetSwitchValuePath(kWebAppCrx);
+    ASSERT_FALSE(webapp_crx_.empty());
+  }
+}
+
+void RemoteDesktopBrowserTest::ExecuteScript(const std::string& script) {
+  ASSERT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents(), script));
+}
+
+void RemoteDesktopBrowserTest::ExecuteScriptAndWait(const std::string& script) {
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<content::NavigationController>(
+          &browser()->tab_strip_model()->GetActiveWebContents()->
+              GetController()));
+
+  ExecuteScript(script);
+
+  observer.Wait();
+}
+
+void RemoteDesktopBrowserTest::ExecuteScriptAndWaitUntil(
+    const std::string& script,
+    const GURL& target) {
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<content::NavigationController>(
+          &browser()->tab_strip_model()->GetActiveWebContents()->
+              GetController()));
+
+  ExecuteScript(script);
+
+  observer.Wait();
+
+  // TODO: is there a better way to wait for all the redirections to complete?
+  while (GetCurrentURL() != target) {
+    content::WindowedNotificationObserver(
+        content::NOTIFICATION_LOAD_STOP,
+        content::Source<content::NavigationController>(
+            &browser()->tab_strip_model()->GetActiveWebContents()->
+                GetController())).Wait();
+  }
+}
+
+bool RemoteDesktopBrowserTest::ExecuteScriptAndExtractBool(
+    const std::string& script) {
+  bool result;
+  // Using a private assert function because ASSERT_TRUE can only be used in
+  // void returning functions.
+  _ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "window.domAutomationController.send(" + script + ");",
+      &result));
+
+  return result;
+}
+
+// Helper to navigate to a given url.
+void RemoteDesktopBrowserTest::NavigateToURLAndWait(const GURL& url) {
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::Source<content::NavigationController>(
+          &browser()->tab_strip_model()->GetActiveWebContents()->
+              GetController()));
+
+  ui_test_utils::NavigateToURL(browser(), url);
+  observer.Wait();
+}
+
 }  // namespace remoting
diff --git a/remoting/test/remote_desktop_browsertest.h b/remoting/test/remote_desktop_browsertest.h
index 0c41f64..96a5b5e 100644
--- a/remoting/test/remote_desktop_browsertest.h
+++ b/remoting/test/remote_desktop_browsertest.h
@@ -37,12 +37,24 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   RemoteDesktopBrowserTest();
   virtual ~RemoteDesktopBrowserTest();
 
+  // InProcessBrowserTest Overrides
   virtual void SetUp() OVERRIDE;
 
  protected:
+  // InProcessBrowserTest Overrides
   virtual void SetUpInProcessBrowserTestFixture() OVERRIDE;
+
+  // InProcessBrowserTest Overrides
   virtual void TearDownInProcessBrowserTestFixture() OVERRIDE;
 
+
+  /*                                                    */
+  /* The following helpers each perform a simple task.  */
+  /*                                                    */
+
+  // Verify the test has access to the internet (specifically google.com)
+  void VerifyInternetAccess();
+
   // Install the chromoting extension from a crx file.
   void InstallChromotingApp();
 
@@ -55,9 +67,6 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   // Launch the chromoting app.
   void LaunchChromotingApp();
 
-  // Verify the test has access to the internet (specifically google.com)
-  void VerifyInternetAccess();
-
   // Authorize: grant extended access permission to the user's computer.
   void Authorize();
 
@@ -67,17 +76,23 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   // Approve: grant the chromoting app necessary permissions.
   void Approve();
 
-  // Whether to perform the cleanup tasks (uninstalling chromoting, etc).
-  // This is useful for diagnostic purposes.
-  bool NoCleanup() { return no_cleanup_; }
 
-  // Whether to install the chromoting extension before running the test cases.
-  // This is useful for diagnostic purposes.
-  bool NoInstall() { return no_install_; }
+  /*                                                      */
+  /* The following helpers each perform a composite task. */
+  /*                                                      */
 
- private:
-  void ParseCommandLine();
+  // Install the chromoting extension
+  void Install();
+
+  // Clean up after the test.
+  void Cleanup();
 
+  // Perform all the auth steps: authorization, authenticattion, etc.
+  // It starts from the chromoting main page unauthenticated and ends up back
+  // on the chromoting main page authenticated and ready to go.
+  void Auth();
+
+ private:
   // Change behavior of the default host resolver to allow DNS lookup
   // to proceed instead of being blocked by the test infrastructure.
   void EnableDNSLookupForThisTest(
@@ -86,12 +101,27 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   // We need to reset the DNS lookup when we finish, or the test will fail.
   void DisableDNSLookupForThisTest();
 
+  void ParseCommandLine();
+
+
+  /*                    */
+  /* Accessor methods.  */
+  /*                    */
+
   // Helper to get the path to the crx file of the webapp to be tested.
   base::FilePath WebAppCrxPath() { return webapp_crx_; }
 
   // Helper to get the extension ID of the installed chromoting webapp.
   std::string ChromotingID() { return chromoting_id_; }
 
+  // Whether to perform the cleanup tasks (uninstalling chromoting, etc).
+  // This is useful for diagnostic purposes.
+  bool NoCleanup() { return no_cleanup_; }
+
+  // Whether to install the chromoting extension before running the test cases.
+  // This is useful for diagnostic purposes.
+  bool NoInstall() { return no_install_; }
+
   // Helper to construct the starting URL of the installed chromoting webapp.
   GURL Chromoting_Main_URL() {
     return GURL("chrome-extension://" + ChromotingID() + "/main.html");
@@ -102,64 +132,29 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
     return browser()->tab_strip_model()->GetActiveWebContents()->GetURL();
   }
 
+
+  /*                                                    */
+  /* Helpers to execute javascript code on a web page.  */
+  /*                                                    */
+
   // Helper to execute a javascript code snippet on the current page.
-  void ExecuteScript(const std::string& script) {
-    ASSERT_TRUE(content::ExecuteScript(
-        browser()->tab_strip_model()->GetActiveWebContents(), script));
-  }
+  void ExecuteScript(const std::string& script);
 
   // Helper to execute a javascript code snippet on the current page and
   // wait for page load to complete.
-  void ExecuteScriptAndWait(const std::string& script) {
-    content::WindowedNotificationObserver observer(
-        content::NOTIFICATION_LOAD_STOP,
-        content::Source<content::NavigationController>(
-            &browser()->tab_strip_model()->GetActiveWebContents()->
-                GetController()));
-
-    ExecuteScript(script);
-
-    observer.Wait();
-  }
+  void ExecuteScriptAndWait(const std::string& script);
 
   // Helper to execute a javascript code snippet on the current page and
   // wait until the target url is loaded. This is used when the target page
   // is loaded after multiple redirections.
-  void ExecuteScriptAndWaitUntil(
-      const std::string& script, const GURL& target) {
-    content::WindowedNotificationObserver observer(
-        content::NOTIFICATION_LOAD_STOP,
-        content::Source<content::NavigationController>(
-            &browser()->tab_strip_model()->GetActiveWebContents()->
-                GetController()));
-
-    ExecuteScript(script);
-
-    observer.Wait();
-
-    // TODO: is there a better way to wait for all the redirections to complete?
-    while (GetCurrentURL() != target) {
-      content::WindowedNotificationObserver(
-          content::NOTIFICATION_LOAD_STOP,
-          content::Source<content::NavigationController>(
-              &browser()->tab_strip_model()->GetActiveWebContents()->
-                  GetController())).Wait();
-    }
-  }
+  void ExecuteScriptAndWaitUntil(const std::string& script, const GURL& target);
 
   // Helper to execute a javascript code snippet on the current page and
   // extract the boolean result.
-  bool ExecuteScriptAndExtractBool(const std::string& script) {
-    bool result;
-    // Using a private assert function because ASSERT_TRUE can only be used in
-    // void returning functions.
-    _ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
-        browser()->tab_strip_model()->GetActiveWebContents(),
-        "window.domAutomationController.send(" + script + ");",
-        &result));
-
-    return result;
-  }
+  bool ExecuteScriptAndExtractBool(const std::string& script);
+
+  // Helper to navigate to a given url.
+  void NavigateToURLAndWait(const GURL& url);
 
   // Helper to check whether a html element with the given name exists on
   // the current page.
@@ -168,17 +163,10 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
         "document.getElementById(\"" + name + "\") != null");
   }
 
-  // Helper to navigate to a given url.
-  void NavigateToURLAndWait(const GURL& url) {
-    content::WindowedNotificationObserver observer(
-        content::NOTIFICATION_LOAD_STOP,
-        content::Source<content::NavigationController>(
-            &browser()->tab_strip_model()->GetActiveWebContents()->
-                GetController()));
-
-    ui_test_utils::NavigateToURL(browser(), url);
-    observer.Wait();
-  }
+
+  /*        */
+  /* Fields */
+  /*        */
 
   // This test needs to make live DNS requests for access to
   // GAIA and sync server URLs under google.com. We use a scoped version