This check in is the initial step to try improve UI

automation framework. Currently we are not consistent in
UI automation. After receiving the automation message,
the automation provider in browser does not always
execute UI commands synchronously. In many cases, it
simply sends back an acknowledgement and dispatches
the command. On the test client side, it waits and
polls the result after sending the message.

I think this causes lots of UI test flakeyness and makes
the test slow. I plan to convert all asynchronous
execution to synchronous. It may take some time to get
them all done. I CC'ed a few people so they are aware of
this ongoing work. Feel free to comment on whether and
how to address the issue.

This check in adds an UI automation message
AutomationMsg_WindowExecuteCommandSync that executes
accelerators synchronously. The existing automation
message AutomationMsg_WindowExecuteCommand only
dispatches the accelerator.

There are many UI accelerators using the existing async
version. In this check in I only made the conversion
for IDC_NEW_TAB to try out the new mechanism.


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

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

7 files changed, 116 insertions, 20 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index ccdb32d..954db43 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -451,6 +451,42 @@ class BrowserClosedNotificationObserver : public NotificationObserver {
   IPC::Message* reply_message_;
 };
 
+class ExecuteBrowserCommandObserver : public NotificationObserver {
+ public:
+  ExecuteBrowserCommandObserver(
+      AutomationProvider* automation,
+      NotificationType::Type notification_type,
+      IPC::Message* reply_message)
+      : automation_(automation),
+        notification_type_(notification_type),
+        reply_message_(reply_message) {
+    registrar_.Add(this, notification_type,
+                    NotificationService::AllSources());
+  }
+
+  ~ExecuteBrowserCommandObserver() {
+  }
+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    if (type == notification_type_) {
+      AutomationMsg_WindowExecuteCommandSync::WriteReplyParams(reply_message_,
+                                                               true);
+      automation_->Send(reply_message_);
+      delete this;
+    } else {
+      NOTREACHED();
+    }
+  }
+
+ private:
+  AutomationProvider* automation_;
+  NotificationType::Type notification_type_;
+  IPC::Message* reply_message_;
+  NotificationRegistrar registrar_;
+};
+
 class FindInPageNotificationObserver : public NotificationObserver {
  public:
   FindInPageNotificationObserver(AutomationProvider* automation,
@@ -803,6 +839,8 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
 #endif  // defined(OS_WIN)
     IPC_MESSAGE_HANDLER(AutomationMsg_WindowExecuteCommand,
                         ExecuteBrowserCommand)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_WindowExecuteCommandSync,
+                        ExecuteBrowserCommandWithNotification)
     IPC_MESSAGE_HANDLER(AutomationMsg_WindowViewBounds,
                         WindowGetViewBounds)
     IPC_MESSAGE_HANDLER(AutomationMsg_SetWindowVisible,
@@ -1283,6 +1321,25 @@ void AutomationProvider::ExecuteBrowserCommand(int handle, int command,
   }
 }
 
+void AutomationProvider::ExecuteBrowserCommandWithNotification(
+    int handle, int command, IPC::Message* reply_message) {
+  if (browser_tracker_->ContainsHandle(handle)) {
+    Browser* browser = browser_tracker_->GetResource(handle);
+    if (browser->command_updater()->SupportsCommand(command) &&
+        browser->command_updater()->IsCommandEnabled(command)) {
+      // TODO(huanr): mapping command to notification type.
+      // For now only IDC_NEW_TAB uses this code path.
+      NotificationType::Type type = NotificationType::TAB_PARENTED;
+      new ExecuteBrowserCommandObserver(this, type, reply_message);
+      browser->ExecuteCommand(command);
+      return;
+    }
+  }
+  AutomationMsg_WindowExecuteCommandSync::WriteReplyParams(reply_message,
+                                                           false);
+  Send(reply_message);
+}
+
 void AutomationProvider::WindowGetViewBounds(int handle, int view_id,
                                              bool screen_coordinates,
                                              bool* success,
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index c2bdba3..38008d0 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -143,6 +143,8 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
   void GetWindowHWND(int handle, HWND* win32_handle);
 #endif  // defined(OS_WIN)
   void ExecuteBrowserCommand(int handle, int command, bool* success);
+  void ExecuteBrowserCommandWithNotification(int handle, int command,
+                                             IPC::Message* reply_message);
   void WindowGetViewBounds(int handle, int view_id, bool screen_coordinates,
                            bool* success, gfx::Rect* bounds);
 #if defined(OS_WIN)
diff --git a/chrome/test/automated_ui_tests/automated_ui_tests.cc b/chrome/test/automated_ui_tests/automated_ui_tests.cc
index 65e03c2..0567d26 100644
--- a/chrome/test/automated_ui_tests/automated_ui_tests.cc
+++ b/chrome/test/automated_ui_tests/automated_ui_tests.cc
@@ -547,22 +547,9 @@ bool AutomatedUITest::NewTab() {
     AddErrorAttribute("browser_window_not_found");
     return false;
   }
-  int old_tab_count;
-  int new_tab_count;
-  bool is_timeout;
-  browser->GetTabCountWithTimeout(&old_tab_count,
-                                  action_max_timeout_ms(),
-                                  &is_timeout);
   // Apply accelerator and wait for a new tab to open, if either
   // fails, return false. Apply Accelerator takes care of logging its failure.
-  bool return_value = RunCommand(IDC_NEW_TAB);
-  if (!browser->WaitForTabCountToChange(old_tab_count,
-                                        &new_tab_count,
-                                        action_max_timeout_ms())) {
-    AddWarningAttribute("tab_count_failed_to_change");
-    return false;
-  }
-  return return_value;
+  return RunCommandSync(IDC_NEW_TAB);
 }
 
 bool AutomatedUITest::OpenAboutDialog() {
@@ -857,6 +844,20 @@ bool AutomatedUITest::RunCommand(int browser_command) {
   return true;
 }
 
+bool AutomatedUITest::RunCommandSync(int browser_command) {
+  scoped_ptr<BrowserProxy> browser(automation()->GetLastActiveBrowserWindow());
+  if (browser.get() == NULL) {
+    AddErrorAttribute("browser_window_not_found");
+    return false;
+  }
+  if (!browser->RunCommandSync(browser_command)) {
+    AddWarningAttribute("failure_running_browser_command");
+    return false;
+  }
+  return true;
+}
+
+
 bool AutomatedUITest::SimulateKeyPressInActiveWindow(wchar_t key, int flags) {
   scoped_ptr<WindowProxy> window(automation()->GetActiveWindow());
   if (window.get() == NULL) {
diff --git a/chrome/test/automated_ui_tests/automated_ui_tests.h b/chrome/test/automated_ui_tests/automated_ui_tests.h
index fdee28c..dcff882 100644
--- a/chrome/test/automated_ui_tests/automated_ui_tests.h
+++ b/chrome/test/automated_ui_tests/automated_ui_tests.h
@@ -390,11 +390,20 @@ class AutomatedUITest : public UITest {
 
   // Runs the specified browser command in the current active browser.
   // See browser_commands.cc for the list of commands.
-  // Returns true if the call is successful.
-  // Returns false if the active window is not a browser window or if the
-  // message to apply the accelerator fails.
+  // Returns true if the call is successfully dispatched.
+  // Possible failures include the active window is not a browser window or
+  // the message to apply the accelerator fails.
   bool RunCommand(int browser_command);
 
+  // Runs the specified browser command in the current active browser, wait
+  // and return until the command has finished executing.
+  // See browser_commands.cc for the list of commands.
+  // Returns true if the call is successfully dispatched and executed.
+  // Possible failures include the active window is not a browser window, or
+  // the message to apply the accelerator fails, or the command execution
+  // fails.
+  bool RunCommandSync(int browser_command);
+
   // Calls SimulateOSKeyPress on the active window. Simulates a key press at
   // the OS level. |key| is the key pressed  and |flags| specifies which
   // modifiers keys are also pressed (as defined in chrome/views/event.h).
diff --git a/chrome/test/automation/automation_messages_internal.h b/chrome/test/automation/automation_messages_internal.h
index 15daa28..c0f3f9b 100644
--- a/chrome/test/automation/automation_messages_internal.h
+++ b/chrome/test/automation/automation_messages_internal.h
@@ -710,13 +710,22 @@ IPC_BEGIN_MESSAGES(Automation)
 
   // This message requests the execution of a browser command in the browser
   // for which the handle is specified.
-  // The return value contains a boolean, whether the command execution was
-  // successful.
+  // The return value contains a boolean, whether the command was dispatched.
   IPC_SYNC_MESSAGE_ROUTED2_1(AutomationMsg_WindowExecuteCommand,
                              int /* automation handle */,
                              int /* browser command */,
                              bool /* success flag */)
 
+  // This message requests the execution of a browser command in the browser
+  // for which the handle is specified.
+  // The return value contains a boolean, whether the command was dispatched
+  // and successful executed.
+  IPC_SYNC_MESSAGE_ROUTED2_1(AutomationMsg_WindowExecuteCommandSync,
+                             int /* automation handle */,
+                             int /* browser command */,
+                             bool /* success flag */)
+
+
   // This message opens the Find window within a tab corresponding to the
   // supplied tab handle.
   IPC_MESSAGE_ROUTED1(AutomationMsg_OpenFindInPage,
diff --git a/chrome/test/automation/browser_proxy.cc b/chrome/test/automation/browser_proxy.cc
index 942919d..9340a61 100644
--- a/chrome/test/automation/browser_proxy.cc
+++ b/chrome/test/automation/browser_proxy.cc
@@ -312,6 +312,19 @@ bool BrowserProxy::RunCommand(int browser_command) const {
   return result;
 }
 
+bool BrowserProxy::RunCommandSync(int browser_command) const {
+  if (!is_valid())
+    return false;
+
+  bool result = false;
+
+  sender_->Send(new AutomationMsg_WindowExecuteCommandSync(0, handle_,
+                                                           browser_command,
+                                                           &result));
+
+  return result;
+}
+
 bool BrowserProxy::GetBookmarkBarVisibility(bool* is_visible,
                                             bool* is_animating) {
   if (!is_valid())
diff --git a/chrome/test/automation/browser_proxy.h b/chrome/test/automation/browser_proxy.h
index 2b3a54e..dffcae1 100644
--- a/chrome/test/automation/browser_proxy.h
+++ b/chrome/test/automation/browser_proxy.h
@@ -179,9 +179,14 @@ class BrowserProxy : public AutomationResourceProxy {
 
   // Run the specified command in the browser (see browser_commands.cc for the
   // list of supported commands).  Returns true if the command was successfully
-  // executed, false otherwise.
+  // dispatched, false otherwise.
   bool RunCommand(int browser_command) const;
 
+  // Run the specified command in the browser (see browser_commands.cc for the
+  // list of supported commands).  Returns true if the command was successfully
+  // dispatched and executed, false otherwise.
+  bool RunCommandSync(int browser_command) const;
+
   // Returns whether the Bookmark bar is visible and whether we are animating
   // it into position. Returns false on failure.
   bool GetBookmarkBarVisibility(bool* is_visible, bool* is_animating);