Modifying extension automation so that it is done through a particular

tab for all extension views.  Previously, API routing to the
automation client would only work for extension views that were
contained in the particular ExternalTab instance being automated. This
meant you couldn't test API calls from e.g. background pages.

Also using async functions instead of the previous RVH-based hack.

Updating one of the UI tests to make the API calls from a background
page, to provide an end-to-end test of the new routing.

This makes AutomationProvider::SetEnableAutomationExtension
Windows-only, but it effectively always was since it was already
dependent on ExternalTabContainer (indirectly, to provide a non-empty
implementation of TabContentsDelegate::ForwardMessageToExternalHost).

BUG=none
TEST=ui_tests.exe, chrome_frame_tests.exe, chrome_frame_unittests.exe

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

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

8 files changed, 186 insertions, 67 deletions

diff --git a/chrome/browser/automation/automation_extension_function.cc b/chrome/browser/automation/automation_extension_function.cc
index 98ca3c9..6f3bb73 100644
--- a/chrome/browser/automation/automation_extension_function.cc
+++ b/chrome/browser/automation/automation_extension_function.cc
@@ -12,26 +12,33 @@
 #include "chrome/browser/extensions/extension_function_dispatcher.h"
 #include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/renderer_host/render_view_host_delegate.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
+#include "chrome/browser/tab_contents/tab_contents_delegate.h"
 
-bool AutomationExtensionFunction::enabled_ = false;
+TabContents* AutomationExtensionFunction::api_handler_tab_ = NULL;
+AutomationExtensionFunction::PendingFunctionsMap
+    AutomationExtensionFunction::pending_functions_;
 
 void AutomationExtensionFunction::SetArgs(const Value* args) {
+  // Need to JSON-encode for sending over the wire to the automation user.
   base::JSONWriter::Write(args, false, &args_);
 }
 
 const std::string AutomationExtensionFunction::GetResult() {
-  // Our API result passing is done through InterceptMessageFromExternalHost
-  return "";
+  // Already JSON-encoded, so override the base class's implementation.
+  return json_result_;
 }
 
-const std::string AutomationExtensionFunction::GetError() {
-  // Our API result passing is done through InterceptMessageFromExternalHost
-  return "";
-}
-
-void AutomationExtensionFunction::Run() {
+bool AutomationExtensionFunction::RunImpl() {
   namespace keys = extension_automation_constants;
 
+  DCHECK(api_handler_tab_) <<
+      "Why is this function still enabled if no target tab?";
+  if (!api_handler_tab_) {
+    error_ = "No longer automating functions.";
+    return false;
+  }
+
   // We are being driven through automation, so we send the extension API
   // request over to the automation host. We do this before decoding the
   // 'args' JSON, otherwise we'd be decoding it only to encode it again.
@@ -43,39 +50,61 @@ void AutomationExtensionFunction::Run() {
 
   std::string message;
   base::JSONWriter::Write(&message_to_host, false, &message);
-  dispatcher()->render_view_host_->delegate()->ProcessExternalHostMessage(
-      message, keys::kAutomationOrigin, keys::kAutomationRequestTarget);
+  if (api_handler_tab_->delegate()) {
+    api_handler_tab_->delegate()->ForwardMessageToExternalHost(
+        message, keys::kAutomationOrigin, keys::kAutomationRequestTarget);
+  } else {
+    NOTREACHED() << "ExternalTabContainer is supposed to correctly manage "
+        "lifetime of api_handler_tab_.";
+  }
+
+  // Automation APIs are asynchronous so we need to stick around until
+  // our response comes back.  Add ourselves to a static hash map keyed
+  // by request ID.  The hash map keeps a reference count on us.
+  DCHECK(pending_functions_.find(request_id_) == pending_functions_.end());
+  pending_functions_[request_id_] = this;
+
+  return true;
 }
 
 ExtensionFunction* AutomationExtensionFunction::Factory() {
   return new AutomationExtensionFunction();
 }
 
-void AutomationExtensionFunction::SetEnabled(
+void AutomationExtensionFunction::Enable(
+    TabContents* api_handler_tab,
     const std::vector<std::string>& functions_enabled) {
-  if (functions_enabled.size() > 0) {
-    std::vector<std::string> function_names;
-    if (functions_enabled.size() == 1 && functions_enabled[0] == "*") {
-      ExtensionFunctionDispatcher::GetAllFunctionNames(&function_names);
-    } else {
-      function_names = functions_enabled;
-    }
+  DCHECK(api_handler_tab);
+  if (api_handler_tab_ && api_handler_tab != api_handler_tab_) {
+    NOTREACHED() << "Don't call with different API handler.";
+    return;
+  }
+  api_handler_tab_ = api_handler_tab;
 
-    for (std::vector<std::string>::iterator it = function_names.begin();
-         it != function_names.end(); it++) {
-      // TODO(joi) Could make this a per-profile change rather than a global
-      // change. Could e.g. have the AutomationExtensionFunction store the
-      // profile pointer and dispatch to the original ExtensionFunction when the
-      // current profile is not that.
-      bool result = ExtensionFunctionDispatcher::OverrideFunction(
-          *it, AutomationExtensionFunction::Factory);
-      LOG_IF(WARNING, !result) << "Failed to override API function: " << *it;
-    }
+  std::vector<std::string> function_names;
+  if (functions_enabled.size() == 1 && functions_enabled[0] == "*") {
+    ExtensionFunctionDispatcher::GetAllFunctionNames(&function_names);
   } else {
-    ExtensionFunctionDispatcher::ResetFunctions();
+    function_names = functions_enabled;
+  }
+
+  for (std::vector<std::string>::iterator it = function_names.begin();
+       it != function_names.end(); it++) {
+    // TODO(joi) Could make this a per-profile change rather than a global
+    // change. Could e.g. have the AutomationExtensionFunction store the
+    // profile pointer and dispatch to the original ExtensionFunction when the
+    // current profile is not that.
+    bool result = ExtensionFunctionDispatcher::OverrideFunction(
+        *it, AutomationExtensionFunction::Factory);
+    LOG_IF(WARNING, !result) << "Failed to override API function: " << *it;
   }
 }
 
+void AutomationExtensionFunction::Disable() {
+   api_handler_tab_ = NULL;
+   ExtensionFunctionDispatcher::ResetFunctions();
+}
+
 bool AutomationExtensionFunction::InterceptMessageFromExternalHost(
     RenderViewHost* view_host,
     const std::string& message,
@@ -83,7 +112,10 @@ bool AutomationExtensionFunction::InterceptMessageFromExternalHost(
     const std::string& target) {
   namespace keys = extension_automation_constants;
 
-  if (origin == keys::kAutomationOrigin &&
+  // We want only specially-tagged messages passed via the conduit tab.
+  if (api_handler_tab_ &&
+      view_host == api_handler_tab_->render_view_host() &&
+      origin == keys::kAutomationOrigin &&
       target == keys::kAutomationResponseTarget) {
     // This is an extension API response being sent back via postMessage,
     // so redirect it.
@@ -107,10 +139,23 @@ bool AutomationExtensionFunction::InterceptMessageFromExternalHost(
                                             &response);
         DCHECK(!success || got_value);
 
-        // TODO(joi) Once ExtensionFunctionDispatcher supports asynchronous
-        // functions, we should use that instead.
-        view_host->SendExtensionResponse(request_id, success,
-                                         response, error);
+        PendingFunctionsMap::iterator it = pending_functions_.find(request_id);
+        DCHECK(it != pending_functions_.end());
+
+        if (it != pending_functions_.end()) {
+          scoped_refptr<AutomationExtensionFunction> func = it->second;
+          pending_functions_.erase(it);
+
+          // Our local ref should be the last remaining.
+          DCHECK(func && func->HasOneRef());
+
+          if (func) {
+            func->json_result_ = response;
+            func->error_ = error;
+
+            func->SendResponse(success);
+          }
+        }
         return true;
       }
     }
diff --git a/chrome/browser/automation/automation_extension_function.h b/chrome/browser/automation/automation_extension_function.h
index ed57ca1..faf125e 100644
--- a/chrome/browser/automation/automation_extension_function.h
+++ b/chrome/browser/automation/automation_extension_function.h
@@ -8,38 +8,49 @@
 #define CHROME_BROWSER_AUTOMATION_AUTOMATION_EXTENSION_FUNCTION_H_
 
 #include <string>
+#include <map>
 
 #include "chrome/browser/extensions/extension_function.h"
 
 class RenderViewHost;
+class TabContents;
 
 // An extension function that pipes the extension API call through the
 // automation interface, so that extensions can be tested using UITests.
-class AutomationExtensionFunction : public ExtensionFunction {
+class AutomationExtensionFunction : public AsyncExtensionFunction {
  public:
   AutomationExtensionFunction() { }
 
   // ExtensionFunction implementation.
   virtual void SetArgs(const Value* args);
   virtual const std::string GetResult();
-  virtual const std::string GetError();
-  virtual void Run();
+  virtual bool RunImpl();
 
   static ExtensionFunction* Factory();
 
+  // Enable API automation of selected APIs.  Overridden extension API messages
+  // will be routed to the automation client attached to |api_handler_tab|.
+  //
   // If the list of enabled functions is non-empty, we enable according to the
   // list ("*" means enable all, otherwise we enable individual named
-  // functions). If empty, we restore the initial functions.
+  // functions).  An empty list makes this function a no-op.
+  //
+  // Note that all calls to this function are additive.  Functions previously
+  // enabled will remain enabled until you call Disable().
   //
-  // Note that all calls to this function, except a call with the empty list,
-  // are additive.  Functions previously enabled will remain enabled until
-  // you clear all function forwarding by specifying the empty list.
-  static void SetEnabled(const std::vector<std::string>& functions_enabled);
+  // Calling this function after enabling one or more functions with a
+  // tab other than the one previously used is an error.
+  static void Enable(TabContents* api_handler_tab,
+                     const std::vector<std::string>& functions_enabled);
+
+  // Restore the default API function implementations and reset the stored
+  // API handler.
+  static void Disable();
 
   // Intercepts messages sent from the external host to check if they
   // are actually responses to extension API calls.  If they are, redirects
-  // the message to view_host->SendExtensionResponse and returns true,
-  // otherwise returns false to indicate the message was not intercepted.
+  // the message to respond to the pending asynchronous API call and returns
+  // true, otherwise returns false to indicate the message was not intercepted.
   static bool InterceptMessageFromExternalHost(RenderViewHost* view_host,
                                                const std::string& message,
                                                const std::string& origin,
@@ -48,8 +59,17 @@ class AutomationExtensionFunction : public ExtensionFunction {
  private:
   ~AutomationExtensionFunction() {}
 
-  static bool enabled_;
+  // Weak reference, lifetime managed by the ExternalTabContainer instance
+  // owning the TabContents in question.
+  static TabContents* api_handler_tab_;
+
+  typedef std::map<int, scoped_refptr<AutomationExtensionFunction> >
+      PendingFunctionsMap;
+  static PendingFunctionsMap pending_functions_;
+
   std::string args_;
+  std::string json_result_;
+
   DISALLOW_COPY_AND_ASSIGN(AutomationExtensionFunction);
 };
 
diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index 64b0133..537cc6d 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -418,8 +418,11 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(AutomationMsg_SavePackageShouldPromptUser,
                         SavePackageShouldPromptUser)
     IPC_MESSAGE_HANDLER(AutomationMsg_WindowTitle, GetWindowTitle)
+#if defined(OS_WIN)
+    // Depends on ExternalTabContainer, so Windows-only
     IPC_MESSAGE_HANDLER(AutomationMsg_SetEnableExtensionAutomation,
                         SetEnableExtensionAutomation)
+#endif
     IPC_MESSAGE_HANDLER(AutomationMsg_SetShelfVisibility, SetShelfVisibility)
     IPC_MESSAGE_HANDLER(AutomationMsg_BlockedPopupCount, GetBlockedPopupCount)
     IPC_MESSAGE_HANDLER(AutomationMsg_SelectAll, SelectAll)
@@ -1930,11 +1933,6 @@ void AutomationProvider::SavePackageShouldPromptUser(bool should_prompt) {
   SavePackage::SetShouldPromptUser(should_prompt);
 }
 
-void AutomationProvider::SetEnableExtensionAutomation(
-    const std::vector<std::string>& functions_enabled) {
-  AutomationExtensionFunction::SetEnabled(functions_enabled);
-}
-
 void AutomationProvider::GetWindowTitle(int handle, string16* text) {
   gfx::NativeWindow window = window_tracker_->GetResource(handle);
   text->assign(platform_util::GetWindowTitle(window));
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index 418c804..82a2d72 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -467,6 +467,7 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
 
   // Enables extension automation (for e.g. UITests).
   void SetEnableExtensionAutomation(
+      int tab_handle,
       const std::vector<std::string>& functions_enabled);
 
   void GetWindowTitle(int handle, string16* text);
diff --git a/chrome/browser/automation/automation_provider_win.cc b/chrome/browser/automation/automation_provider_win.cc
index cfadf91..7b750ca 100644
--- a/chrome/browser/automation/automation_provider_win.cc
+++ b/chrome/browser/automation/automation_provider_win.cc
@@ -489,3 +489,18 @@ void AutomationProvider::TerminateSession(int handle, bool* success) {
     *success = (::PostMessageW(window, WM_ENDSESSION, 0, 0) == TRUE);
   }
 }
+
+void AutomationProvider::SetEnableExtensionAutomation(
+    int tab_handle,
+    const std::vector<std::string>& functions_enabled) {
+  ExternalTabContainer* external_tab = GetExternalTabForHandle(tab_handle);
+  if (external_tab) {
+    external_tab->SetEnableExtensionAutomation(functions_enabled);
+  } else {
+    // Tab must exist, and must be an external tab so that its
+    // delegate has an on-empty
+    // implementation of ForwardMessageToExternalHost.
+    DLOG(WARNING) <<
+      "SetEnableExtensionAutomation called with invalid tab handle.";
+  }
+}
diff --git a/chrome/browser/extensions/extension_uitest.cc b/chrome/browser/extensions/extension_uitest.cc
index ac4504c..2cfcd7c 100644
--- a/chrome/browser/extensions/extension_uitest.cc
+++ b/chrome/browser/extensions/extension_uitest.cc
@@ -55,38 +55,46 @@ class ExtensionUITest : public ParentTestType {
 
   void SetUp() {
     ParentTestType::SetUp();
-    automation()->SetEnableExtensionAutomation(functions_enabled_);
+
+    AutomationProxyForExternalTab* proxy =
+        static_cast<AutomationProxyForExternalTab*>(automation());
+    HWND external_tab_container = NULL;
+    HWND tab_wnd = NULL;
+    tab_ = proxy->CreateTabWithHostWindow(false,
+        GURL(), &external_tab_container, &tab_wnd);
+
+    tab_->SetEnableExtensionAutomation(functions_enabled_);
   }
 
   void TearDown() {
-    automation()->SetEnableExtensionAutomation(std::vector<std::string>());
+    tab_->SetEnableExtensionAutomation(std::vector<std::string>());
+
+    AutomationProxyForExternalTab* proxy =
+        static_cast<AutomationProxyForExternalTab*>(automation());
+    proxy->DestroyHostWindow();
+    proxy->WaitForTabCleanup(tab_, action_max_timeout_ms());
+    EXPECT_FALSE(tab_->is_valid());
+    tab_.release();
+
     ParentTestType::TearDown();
   }
 
   void TestWithURL(const GURL& url) {
-    AutomationProxyForExternalTab* proxy =
+    EXPECT_TRUE(tab_->is_valid());
+    if (tab_) {
+      AutomationProxyForExternalTab* proxy =
         static_cast<AutomationProxyForExternalTab*>(automation());
-    HWND external_tab_container = NULL;
-    HWND tab_wnd = NULL;
-    scoped_refptr<TabProxy> tab(proxy->CreateTabWithHostWindow(false,
-        GURL(), &external_tab_container, &tab_wnd));
 
-    EXPECT_TRUE(tab->is_valid());
-    if (tab) {
       // Enter a message loop to allow the tab to be created
       proxy->WaitForNavigation(2000);
-      DoAdditionalPreNavigateSetup(tab.get());
+      DoAdditionalPreNavigateSetup(tab_.get());
 
       // We explicitly do not make this a toolstrip in the extension manifest,
       // so that the test can control when it gets loaded, and so that we test
       // the intended behavior that tabs should be able to show extension pages
       // (useful for development etc.)
-      tab->NavigateInExternalTab(url, GURL());
+      tab_->NavigateInExternalTab(url, GURL());
       EXPECT_TRUE(proxy->WaitForMessage(action_max_timeout_ms()));
-
-      proxy->DestroyHostWindow();
-      proxy->WaitForTabCleanup(tab, action_max_timeout_ms());
-      EXPECT_FALSE(tab->is_valid());
     }
   }
 
@@ -97,6 +105,7 @@ class ExtensionUITest : public ParentTestType {
  protected:
   // Extension API functions that we want to take over.  Defaults to all.
   std::vector<std::string> functions_enabled_;
+  scoped_refptr<TabProxy> tab_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionUITest);
diff --git a/chrome/browser/external_tab_container.cc b/chrome/browser/external_tab_container.cc
index d200891..4440294 100644
--- a/chrome/browser/external_tab_container.cc
+++ b/chrome/browser/external_tab_container.cc
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/win_util.h"
 #include "chrome/browser/automation/automation_provider.h"
+#include "chrome/browser/automation/automation_extension_function.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/debugger/devtools_manager.h"
 #include "chrome/browser/load_notification_details.h"
@@ -43,7 +44,8 @@ ExternalTabContainer::ExternalTabContainer(
       automation_resource_message_filter_(filter),
       load_requests_via_automation_(false),
       handle_top_level_requests_(false),
-      external_method_factory_(this) {
+      external_method_factory_(this),
+      enabled_extension_automation_(false) {
 }
 
 ExternalTabContainer::~ExternalTabContainer() {
@@ -161,6 +163,10 @@ bool ExternalTabContainer::Init(Profile* profile,
 }
 
 void ExternalTabContainer::Uninitialize() {
+  if (enabled_extension_automation_) {
+    AutomationExtensionFunction::Disable();
+  }
+
   registrar_.RemoveAll();
   if (tab_contents_) {
     RenderViewHost* rvh = tab_contents_->render_view_host();
@@ -659,6 +665,22 @@ ExternalTabContainer* ExternalTabContainer::RemovePendingTab(intptr_t cookie) {
   return NULL;
 }
 
+void ExternalTabContainer::SetEnableExtensionAutomation(
+    const std::vector<std::string>& functions_enabled) {
+  if (functions_enabled.size() > 0) {
+    if (!tab_contents_) {
+      NOTREACHED() << "Being invoked via tab so should have TabContents";
+      return;
+    }
+
+    AutomationExtensionFunction::Enable(tab_contents_, functions_enabled);
+    enabled_extension_automation_ = true;
+  } else {
+    AutomationExtensionFunction::Disable();
+    enabled_extension_automation_ = false;
+  }
+}
+
 void ExternalTabContainer::Navigate(const GURL& url, const GURL& referrer) {
   if (!tab_contents_) {
     NOTREACHED();
diff --git a/chrome/browser/external_tab_container.h b/chrome/browser/external_tab_container.h
index 3c97233..95ab846 100644
--- a/chrome/browser/external_tab_container.h
+++ b/chrome/browser/external_tab_container.h
@@ -157,6 +157,12 @@ class ExternalTabContainer : public TabContentsDelegate,
   // Returns NULL if we fail to find the cookie in the map.
   static ExternalTabContainer* RemovePendingTab(intptr_t cookie);
 
+  // Enables extension automation (for e.g. UITests), with the current tab
+  // used as a conduit for the extension API messages being handled by the
+  // automation client.
+  void SetEnableExtensionAutomation(
+      const std::vector<std::string>& functions_enabled);
+
  protected:
   // Overridden from views::WidgetWin:
   virtual LRESULT OnCreate(LPCREATESTRUCT create_struct);
@@ -216,6 +222,9 @@ class ExternalTabContainer : public TabContentsDelegate,
   // Contains ExternalTabContainers that have not been connected to as yet.
   static PendingTabs pending_tabs_;
 
+  // True if this tab is currently the conduit for extension API automation.
+  bool enabled_extension_automation_;
+
   // Allows us to run tasks on the ExternalTabContainer instance which are
   // bound by its lifetime.
   ScopedRunnableMethodFactory<ExternalTabContainer> external_method_factory_;