Refactoring the synchronization code in the chromoting browser_tests

BUG=134210


Details:
1. Implement Wait* routines using nested message loop.
2. Replace all the busy wait code with the Wait* routines.
3. Refactor NavigateToURLAndWaitForPageLoad: Use a callback to check the currently loaded page in WindowsNotificationObserver.
4. Some other minor refactorings.
5. Move Me2MeBrowserTest in a separate test class. The class is empty right now but me2me specific test routines will be added to it in subsequent CLs.

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

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

5 files changed, 262 insertions, 70 deletions

diff --git a/remoting/test/me2me_browsertest.cc b/remoting/test/me2me_browsertest.cc
index 115bd93..bd1f7a2 100644
--- a/remoting/test/me2me_browsertest.cc
+++ b/remoting/test/me2me_browsertest.cc
@@ -6,7 +6,10 @@
 
 namespace remoting {
 
-IN_PROC_BROWSER_TEST_F(RemoteDesktopBrowserTest,
+class Me2MeBrowserTest : public RemoteDesktopBrowserTest {
+};
+
+IN_PROC_BROWSER_TEST_F(Me2MeBrowserTest,
                        MANUAL_Me2Me_Connect_Localhost) {
   VerifyInternetAccess();
 
@@ -21,8 +24,6 @@ IN_PROC_BROWSER_TEST_F(RemoteDesktopBrowserTest,
 
   ConnectToLocalHost();
 
-  EnterPin(me2me_pin());
-
   Cleanup();
 }
 
diff --git a/remoting/test/remote_desktop_browsertest.cc b/remoting/test/remote_desktop_browsertest.cc
index d389788..d7303e4 100644
--- a/remoting/test/remote_desktop_browsertest.cc
+++ b/remoting/test/remote_desktop_browsertest.cc
@@ -12,6 +12,7 @@
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/test/test_utils.h"
+#include "remoting/test/waiter.h"
 
 using extensions::Extension;
 
@@ -40,7 +41,7 @@ void RemoteDesktopBrowserTest::TearDownInProcessBrowserTestFixture() {
 
 void RemoteDesktopBrowserTest::VerifyInternetAccess() {
   GURL google_url("http://www.google.com");
-  NavigateToURLAndWait(google_url);
+  NavigateToURLAndWaitForPageLoad(google_url);
 
   EXPECT_EQ(GetCurrentURL().host(), "www.google.com");
 }
@@ -111,7 +112,7 @@ void RemoteDesktopBrowserTest::LaunchChromotingApp() {
   ASSERT_FALSE(ChromotingID().empty());
 
   const GURL chromoting_main = Chromoting_Main_URL();
-  NavigateToURLAndWait(chromoting_main);
+  NavigateToURLAndWaitForPageLoad(chromoting_main);
 
   EXPECT_EQ(GetCurrentURL(), chromoting_main);
 }
@@ -126,7 +127,8 @@ void RemoteDesktopBrowserTest::Authorize() {
   ASSERT_FALSE(ExecuteScriptAndExtractBool(
       "remoting.OAuth2.prototype.isAuthenticated()"));
 
-  ExecuteScriptAndWait("remoting.OAuth2.prototype.doAuthRedirect();");
+  ExecuteScriptAndWaitForAnyPageLoad(
+      "remoting.OAuth2.prototype.doAuthRedirect();");
 
   // Verify the active tab is at the "Google Accounts" login page.
   EXPECT_EQ(GetCurrentURL().host(), "accounts.google.com");
@@ -144,14 +146,14 @@ void RemoteDesktopBrowserTest::Authenticate() {
   ASSERT_TRUE(HtmlElementExists("Passwd"));
 
   // Now log in using the username and password passed in from the command line.
-  ExecuteScriptAndWait(
+  ExecuteScriptAndWaitForAnyPageLoad(
       "document.getElementById(\"Email\").value = \"" + username_ + "\";" +
       "document.getElementById(\"Passwd\").value = \"" + password_ +"\";" +
       "document.forms[\"gaia_loginform\"].submit();");
 
   EXPECT_EQ(GetCurrentURL().host(), "accounts.google.com");
 
-  // TODO: Is there a better way to verify we are on the
+  // TODO(weitaosu): Is there a better way to verify we are on the
   // "Request for Permission" page?
   EXPECT_TRUE(HtmlElementExists("submit_approve_access"));
 }
@@ -168,7 +170,7 @@ void RemoteDesktopBrowserTest::Approve() {
   ASSERT_TRUE(HtmlElementExists("submit_approve_access"));
 
   const GURL chromoting_main = Chromoting_Main_URL();
-  ExecuteScriptAndWaitUntil(
+  ExecuteScriptAndWaitForPageLoad(
       "lso.approveButtonAction();"
       "document.forms[\"connect-approve\"].submit();",
       chromoting_main);
@@ -198,6 +200,19 @@ void RemoteDesktopBrowserTest::StartMe2Me() {
 
   EXPECT_TRUE(HtmlElementVisible("me2me-content"));
   EXPECT_FALSE(HtmlElementVisible("me2me-first-run"));
+
+  // Wait until localHost is initialized. This can take a while.
+  ConditionalTimeoutWaiter waiter(
+      base::TimeDelta::FromSeconds(5),
+      base::TimeDelta::FromSeconds(1),
+      base::Bind(&RemoteDesktopBrowserTest::IsLocalHostReady, this));
+  EXPECT_TRUE(waiter.Wait());
+
+  EXPECT_TRUE(ExecuteScriptAndExtractBool(
+      "remoting.hostList.localHost_.hostName && "
+      "remoting.hostList.localHost_.hostId && "
+      "remoting.hostList.localHost_.status && "
+      "remoting.hostList.localHost_.status == 'ONLINE'"));
 }
 
 void RemoteDesktopBrowserTest::SimulateKeyPressWithCode(
@@ -224,7 +239,7 @@ void RemoteDesktopBrowserTest::SimulateKeyPressWithCode(
 }
 
 void RemoteDesktopBrowserTest::Install() {
-  // TODO: add support for installing unpacked extension (the v2 app needs it).
+  // TODO(weitaosu): add support for unpacked extension (the v2 app needs it).
   if (!NoInstall()) {
     VerifyChromotingLoaded(false);
     InstallChromotingApp();
@@ -234,7 +249,8 @@ void RemoteDesktopBrowserTest::Install() {
 }
 
 void RemoteDesktopBrowserTest::Cleanup() {
-  // TODO: Remove this hack by blocking on the appropriate notification.
+  // TODO(weitaosu): 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.
@@ -253,21 +269,20 @@ void RemoteDesktopBrowserTest::Auth() {
 }
 
 void RemoteDesktopBrowserTest::ConnectToLocalHost() {
-  // Wait until remoting.hostList.localHost_ is initialized.
-  // This can take a while.
-  // TODO: Instead of polling, can we register a callback to
-  // remoting.hostList.setLocalHost_?
-  while (ExecuteScriptAndExtractBool(
-      "remoting.hostList.localHost_ == null")) {
-  }
-
+  // Verify that the local host is online.
   ASSERT_TRUE(ExecuteScriptAndExtractBool(
       "remoting.hostList.localHost_.hostName && "
       "remoting.hostList.localHost_.hostId && "
       "remoting.hostList.localHost_.status && "
       "remoting.hostList.localHost_.status == 'ONLINE'"));
 
+  // Connect.
   ClickOnControl("this-host-connect");
+
+  // Enter the pin # passed in from the command line.
+  EnterPin(me2me_pin());
+
+  WaitForConnection();
 }
 
 void RemoteDesktopBrowserTest::EnableDNSLookupForThisTest(
@@ -329,7 +344,8 @@ void RemoteDesktopBrowserTest::ExecuteScript(const std::string& script) {
       browser()->tab_strip_model()->GetActiveWebContents(), script));
 }
 
-void RemoteDesktopBrowserTest::ExecuteScriptAndWait(const std::string& script) {
+void RemoteDesktopBrowserTest::ExecuteScriptAndWaitForAnyPageLoad(
+    const std::string& script) {
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_LOAD_STOP,
       content::Source<content::NavigationController>(
@@ -341,27 +357,16 @@ void RemoteDesktopBrowserTest::ExecuteScriptAndWait(const std::string& script) {
   observer.Wait();
 }
 
-void RemoteDesktopBrowserTest::ExecuteScriptAndWaitUntil(
+void RemoteDesktopBrowserTest::ExecuteScriptAndWaitForPageLoad(
     const std::string& script,
     const GURL& target) {
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_LOAD_STOP,
-      content::Source<content::NavigationController>(
-          &browser()->tab_strip_model()->GetActiveWebContents()->
-              GetController()));
+      base::Bind(&RemoteDesktopBrowserTest::IsURLLoaded, this, target));
 
   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(
@@ -398,7 +403,8 @@ std::string RemoteDesktopBrowserTest::ExecuteScriptAndExtractString(
 }
 
 // Helper to navigate to a given url.
-void RemoteDesktopBrowserTest::NavigateToURLAndWait(const GURL& url) {
+void RemoteDesktopBrowserTest::NavigateToURLAndWaitForPageLoad(
+    const GURL& url) {
   content::WindowedNotificationObserver observer(
       content::NOTIFICATION_LOAD_STOP,
       content::Source<content::NavigationController>(
@@ -419,33 +425,62 @@ void RemoteDesktopBrowserTest::ClickOnControl(const std::string& name) {
 void RemoteDesktopBrowserTest::EnterPin(const std::string& pin) {
   // Wait for the pin-form to be displayed. This can take a while.
   // We also need to dismiss the host-needs-update dialog if it comes up.
-  // TODO 1: Instead of polling, can we register a callback to be called
-  // when the pin-form is ready?
-  // TODO 2: Instead of blindly dismiss the host-needs-update dialog,
+  // TODO(weitaosu) 1: Instead of polling, can we register a callback to be
+  // called when the pin-form is ready?
+  // TODO(weitaosu) 2: Instead of blindly dismiss the host-needs-update dialog,
   // we should verify that it only pops up at the right circumstance. That
   // probably belongs in a separate test case though.
-  do {
-    if (HtmlElementVisible("host-needs-update-connect-button")) {
-      ClickOnControl("host-needs-update-connect-button");
-    }
-  } while (!HtmlElementVisible("pin-form"));
+  ConditionalTimeoutWaiter waiter(
+      base::TimeDelta::FromSeconds(3),
+      base::TimeDelta::FromSeconds(1),
+      base::Bind(&RemoteDesktopBrowserTest::IsPinFormVisible, this));
+  EXPECT_TRUE(waiter.Wait());
 
   ExecuteScript(
       "document.getElementById(\"pin-entry\").value = \"" + pin + "\";");
 
   ClickOnControl("pin-connect-button");
-
-  WaitForConnection();
 }
 
 void RemoteDesktopBrowserTest::WaitForConnection() {
   // Wait until the client has connected to the server.
   // This can take a while.
-  // TODO: Instead of polling, can we register a callback to
+  // TODO(weitaosu): Instead of polling, can we register a callback to
   // remoting.clientSession.onStageChange_?
-  while (ExecuteScriptAndExtractBool(
-      "remoting.clientSession == null")) {
-  }
+  ConditionalTimeoutWaiter waiter(
+      base::TimeDelta::FromSeconds(8),
+      base::TimeDelta::FromSeconds(1),
+      base::Bind(&RemoteDesktopBrowserTest::IsSessionConnected, this));
+  EXPECT_TRUE(waiter.Wait());
+
+  // The client is not yet ready to take input when the session state becomes
+  // CONNECTED. Wait for 3 seconds for the client to become ready.
+  // TODO(weitaosu): Find a way to detect when the client is truly ready.
+  TimeoutWaiter(base::TimeDelta::FromSeconds(3)).Wait();
+}
+
+bool RemoteDesktopBrowserTest::IsLocalHostReady() {
+  // TODO(weitaosu): Instead of polling, can we register a callback to
+  // remoting.hostList.setLocalHost_?
+  return ExecuteScriptAndExtractBool("remoting.hostList.localHost_ != null");
+}
+
+bool RemoteDesktopBrowserTest::IsSessionConnected() {
+  return ExecuteScriptAndExtractBool(
+      "remoting.clientSession != null && "
+      "remoting.clientSession.getState() == "
+      "remoting.ClientSession.State.CONNECTED");
+}
+
+bool RemoteDesktopBrowserTest::IsPinFormVisible() {
+  if (HtmlElementVisible("host-needs-update-connect-button"))
+    ClickOnControl("host-needs-update-connect-button");
+
+  return HtmlElementVisible("pin-form");
+}
+
+bool RemoteDesktopBrowserTest::IsURLLoaded(const GURL& url) {
+  return GetCurrentURL() == url;
 }
 
 }  // namespace remoting
diff --git a/remoting/test/remote_desktop_browsertest.h b/remoting/test/remote_desktop_browsertest.h
index a9922b8..81e73e9 100644
--- a/remoting/test/remote_desktop_browsertest.h
+++ b/remoting/test/remote_desktop_browsertest.h
@@ -49,10 +49,7 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   // InProcessBrowserTest Overrides
   virtual void TearDownInProcessBrowserTestFixture() OVERRIDE;
 
-
-  /*                                                    */
-  /* The following helpers each perform a simple task.  */
-  /*                                                    */
+  // The following helpers each perform a simple task.
 
   // Verify the test has access to the internet (specifically google.com)
   void VerifyInternetAccess();
@@ -91,10 +88,7 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
                                 bool alt,
                                 bool command);
 
-
-  /*                                                      */
-  /* The following helpers each perform a composite task. */
-  /*                                                      */
+  // The following helpers each perform a composite task.
 
   // Install the chromoting extension
   void Install();
@@ -127,10 +121,7 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
 
   void ParseCommandLine();
 
-
-  /*                    */
-  /* Accessor methods.  */
-  /*                    */
+  // Accessor methods.
 
   // Helper to get the path to the crx file of the webapp to be tested.
   base::FilePath WebAppCrxPath() { return webapp_crx_; }
@@ -156,22 +147,20 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
     return browser()->tab_strip_model()->GetActiveWebContents()->GetURL();
   }
 
-
-  /*                                                    */
-  /* Helpers to execute javascript code on a web page.  */
-  /*                                                    */
+  // 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);
 
   // Helper to execute a javascript code snippet on the current page and
   // wait for page load to complete.
-  void ExecuteScriptAndWait(const std::string& script);
+  void ExecuteScriptAndWaitForAnyPageLoad(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);
+  void ExecuteScriptAndWaitForPageLoad(const std::string& script,
+                                       const GURL& target);
 
   // Helper to execute a javascript code snippet on the current page and
   // extract the boolean result.
@@ -186,7 +175,7 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   std::string ExecuteScriptAndExtractString(const std::string& script);
 
   // Helper to navigate to a given url.
-  void NavigateToURLAndWait(const GURL& url);
+  void NavigateToURLAndWaitForPageLoad(const GURL& url);
 
   // Helper to check whether an html element with the given name exists on
   // the current page.
@@ -204,10 +193,22 @@ class RemoteDesktopBrowserTest : public ExtensionBrowserTest {
   // Wait for the me2me connection to be established.
   void WaitForConnection();
 
+  // Checking whether the localHost has been initialized.
+  bool IsLocalHostReady();
+
+  // Callback used by EnterPin to check whether the pin form is visible
+  // and to dismiss the host-needs-update dialog.
+  bool IsPinFormVisible();
+
+  // Callback used by WaitForConnection to check whether the connection
+  // has been established.
+  bool IsSessionConnected();
+
+  // Callback used by ExecuteScriptAndWaitForPageLoad to check whether
+  // the given page is currently loaded.
+  bool IsURLLoaded(const GURL& url);
 
-  /*        */
-  /* Fields */
-  /*        */
+  // 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
diff --git a/remoting/test/waiter.cc b/remoting/test/waiter.cc
new file mode 100644
index 0000000..adc21c0
--- /dev/null
+++ b/remoting/test/waiter.cc
@@ -0,0 +1,81 @@
+// 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 "remoting/test/waiter.h"
+
+#include "content/public/test/test_utils.h"
+
+namespace remoting {
+
+TimeoutWaiter::TimeoutWaiter(base::TimeDelta timeout)
+    : timeout_(timeout) {
+  DCHECK(timeout > base::TimeDelta::FromSeconds(0));
+}
+
+TimeoutWaiter::~TimeoutWaiter() {}
+
+bool TimeoutWaiter::Wait() {
+  DCHECK(!timeout_timer_.IsRunning());
+
+  timeout_timer_.Start(
+      FROM_HERE,
+      timeout_,
+      base::Bind(&TimeoutWaiter::CancelWaitCallback, base::Unretained(this)));
+
+  message_loop_runner_ = new content::MessageLoopRunner;
+  message_loop_runner_->Run();
+
+  return true;
+}
+
+void TimeoutWaiter::CancelWait() {
+  message_loop_runner_->Quit();
+}
+
+void TimeoutWaiter::CancelWaitCallback() {
+  CancelWait();
+}
+
+ConditionalTimeoutWaiter::ConditionalTimeoutWaiter(base::TimeDelta timeout,
+                                                   base::TimeDelta interval,
+                                                   const Predicate& callback)
+    : TimeoutWaiter(timeout),
+      interval_(interval),
+      callback_(callback),
+      success_(false) {
+  DCHECK(timeout > interval);
+}
+
+ConditionalTimeoutWaiter::~ConditionalTimeoutWaiter() {}
+
+bool ConditionalTimeoutWaiter::Wait() {
+  DCHECK(!condition_timer_.IsRunning());
+
+  condition_timer_.Start(
+      FROM_HERE,
+      interval_,
+      base::Bind(&ConditionalTimeoutWaiter::CancelWaitCallback,
+                 base::Unretained(this)));
+
+  // Also call the base class Wait() to start the timeout timer.
+  TimeoutWaiter::Wait();
+
+  return success_;
+}
+
+void ConditionalTimeoutWaiter::CancelWait() {
+  condition_timer_.Stop();
+
+  // Also call the base class CancelWait() to stop the timeout timer.
+  TimeoutWaiter::CancelWait();
+}
+
+void ConditionalTimeoutWaiter::CancelWaitCallback() {
+  if (callback_.Run()) {
+    CancelWait();
+    success_ = true;
+  }
+}
+
+}  // namespace remoting
diff --git a/remoting/test/waiter.h b/remoting/test/waiter.h
new file mode 100644
index 0000000..71e64a8
--- /dev/null
+++ b/remoting/test/waiter.h
@@ -0,0 +1,74 @@
+// 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.
+
+#ifndef REMOTING_TEST_WAITER_H_
+#define REMOTING_TEST_WAITER_H_
+
+#include "base/timer/timer.h"
+
+namespace content {
+class MessageLoopRunner;
+}
+
+namespace remoting {
+
+// Block the execution of the test code for the specified number of
+// milliseconds while keeping the message loop running. The browser instance
+// will be responsive during the wait and test actions initiated before
+// the wait will keep running.
+class TimeoutWaiter {
+ public:
+  TimeoutWaiter(base::TimeDelta timeout);
+  virtual ~TimeoutWaiter();
+
+  // Returns true in case of success.
+  // For TimeoutWaiter it should always be true.
+  virtual bool Wait();
+
+ protected:
+  virtual void CancelWait();
+
+ private:
+  // Callback used to cancel the TimeoutWaiter::Wait.
+  void CancelWaitCallback();
+
+  base::OneShotTimer<TimeoutWaiter> timeout_timer_;
+  base::TimeDelta timeout_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(TimeoutWaiter);
+};
+
+// With a message loop running, keep calling callback in the specified
+// interval until true is returned.
+class ConditionalTimeoutWaiter : public TimeoutWaiter {
+ public:
+  typedef base::Callback<bool(void)> Predicate;
+
+  ConditionalTimeoutWaiter(base::TimeDelta timeout,
+                           base::TimeDelta interval,
+                           const Predicate& callback);
+  virtual ~ConditionalTimeoutWaiter();
+
+  // Returns true if |callback_| returned true and false in case of timeout.
+  virtual bool Wait() OVERRIDE;
+
+ protected:
+  virtual void CancelWait() OVERRIDE;
+
+ private:
+  // Callback used to cancel the ConditionalTimeoutWaiter::Wait.
+  void CancelWaitCallback();
+
+  base::TimeDelta interval_;
+  Predicate callback_;
+  base::RepeatingTimer<ConditionalTimeoutWaiter> condition_timer_;
+  bool success_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConditionalTimeoutWaiter);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_TEST_WAITER_H_